Introduce WindowsOnlyAttribute and use it to simplify tests #315
Conversation
|
Awesome! I think I like this approach better than the categories, a lot easier to run. My only big comment would be, if I recall correctly we have some tests that still check IsUnixPlatform() and add/remove test data based on the environment. I think it would be a lot better if we werent modifying test data within the test (avoiding logic in tests), and just splitting those out into new tests that we can then just flat out skip for Linux. Does that make sense? |
| // Assert | ||
| Assert.IsNotNull(result); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is one example, I think there was at least one more.
// These are no problems on Unix platforms
yield return @"..\";
yield return @"aaa\vv..\";
yield return @"a..\b"; I think I'd rather see this in another test and we could add this skip attribute
There was a problem hiding this comment.
6d58566 splits this to separate tests. I'm not sure (but also not very emotional) on what I like better 🤣
There was a problem hiding this comment.
haha I think the idea is better, anyway. You typically want to avoid any sort of logic within a test.
For the approach, I don't think I'd have a test that calls another test with different input. I was more thinking just explicitly put the input in the test and run the test, even if the arrange/act are similar or exact.
So you'd have something like
[Skip on Linux]
public void MockDirectoryInfo_FullName_UNCPaths_ShouldReturnNormalizedPath()
{
var input = object array of uncpaths {} // arrange
// act
// assert
}There really isn't a reason, that I can see anyway, to have that test data outside of the test itself (especially public ❤️ )
There was a problem hiding this comment.
If that's still a little bit confusing, let me know and I could whip up an example. I just think its better to be a little be repetitive in tests vs. trying to avoid repeating yourself. When it comes to tests, I think it's generally better to show as much intent/be expressive as possible.
There was a problem hiding this comment.
Yeah, that's what I also didn't like. b6a744c tries to improve on this.
There was a problem hiding this comment.
Woo! I just saw one more, and then I think we're good. Thanks for this! 🎉
| { | ||
| internal static class SkipReason | ||
| { | ||
| public const string NoDrivesOnUnix = "Unix does not have the concept of Drives"; |
There was a problem hiding this comment.
I think there are a couple tests that are skipped due to Illegal characters
There was a problem hiding this comment.
Yes, added a new reason for that
|
|
||
| namespace System.IO.Abstractions.TestingHelpers.Tests | ||
| { | ||
| internal static class SkipReason |
There was a problem hiding this comment.
I'd probably put this in its own class. You?
|
Looking great! This will be so much better than that if(linux) block everywhere. |
|
Most minor of quibbles, but I would call it Yes, I feel rhyming grants this critique a little extra legitimacy. |
|
That’s actually a good idea @rkoeninger - makes it much clearer IMO! Will change |
fgreinacher
changed the title
|
|
||
| [TestCaseSource("MockDirectoryInfo_FullName_Data_WindowsOnly")] | ||
| [WindowsOnly(WindowsSpecifics.UNCPaths)] | ||
| public void MockDirectoryInfo_FullName_ShouldReturnNormalizedPath_WindowsOnly(string directoryPath, string expectedFullName) |
There was a problem hiding this comment.
Another case of a test calling a test.
….Abstractions into chore/skip-on-unix
|
I'll merge this when the build starts passing. I'm excited for this one! |
|
Whenever I defeat Git 😛 |
|
Woo! Awesome 🎉 |
|
Done 🎉 Please squash this crappy commits when merging 🤖 |
3 participants
@jpreese this addresses some of your comments on my fix PR. Do you think this goes in the right direction? I'm also very open for feedback on the naming.