Repository navigation
Fix/disable tests that fail on Unix - #294
Conversation
jpreese
left a comment
jpreese
left a comment
There was a problem hiding this comment.
Thanks for the PR! I went ahead and added a couple comments.
| [TestCase(" ")] | ||
| public void MockDirectory_GetAccessControl_ShouldThrowArgumentExceptionIfPathContainsOnlyWhitespaces(string path) | ||
| { | ||
| if (MockUnixSupport.IsUnixPlatform()) |
There was a problem hiding this comment.
Instead of this block of code throughout the test project, what do we think about categorizing Windows tests?
I think this would declutter the test runner a bit more (no need for inconclusives) and would also clearly show that the feature we're testing is intended for Windows.
| yield return new object[] { XFS.Path(@"c:\temp//\\///folder"), XFS.Path(@"c:\temp\folder") }; | ||
| yield return new object[] { XFS.Path(@"\\unc\folder"), XFS.Path(@"\\unc\folder") }; | ||
| yield return new object[] { XFS.Path(@"\\unc/folder\\foo"), XFS.Path(@"\\unc\folder\foo") }; | ||
| if (!MockUnixSupport.IsUnixPlatform()) |
There was a problem hiding this comment.
I think this would be a little better if we just removed these values from the test data, and then made a new Windows test that checked just the two unc paths. This way we're testing drive paths and unc paths in two different tests. What do you think?
| fileSystem.AddFile(@"C:\LOUD\file.txt", "foo"); | ||
| fileSystem.AddDirectory(@"C:\test\SUBDirectory"); | ||
| fileSystem.AddDirectory(@"C:\LOUD\SUBDirectory"); | ||
| fileSystem.AddFile(MockUnixSupport.Path(@"C:\test\file.txt"), "foo"); |
There was a problem hiding this comment.
Should we, for consistency, use XFS as the namespace instead of MockUnixSupport? My first gut reaction was what is the difference between MockUnitSupport and XFS? If we chose one and stuck with it, I think the experience would be a little better.
| yield return @"..\"; | ||
| yield return @"aaa\vv..\"; | ||
|
|
||
| if (!MockUnixSupport.IsUnixPlatform()) |
There was a problem hiding this comment.
This is a little confusing to me. If we're not on a Unix platform, go ahead and add these values to the test data.. but the comment says that these aren't a problem on Unix, so why would we exclude these? If they do need to be excluded, I would go with a similar approach as outlined above where we'd just go ahead and remove these and leverage a Windows test.
|
In order to increase our velocity, I'm just going to go ahead and merge in this PR since it will immensely help moving forward with PRs and validating against Linux. I think everything will work, I just have some opinions as to how things may have been done a little cleaner. Regardless if we do agree with the above comments, we can revisit in a future PR. |
|
Thanks - busy days for me. Will take care of the follow-ups! |
|
Sounds good! As stated above, they were mostly just suggestions. If you feel differently, I'd love to hear your counter thoughts. |
This MR makes the Travis build green, by either