Tweak path related tests#22407
Conversation
Needed to update a few tests for unix whitespace behavior.
Also add another assert extension for three exceptions.
|
These are hitting in #22368 |
|
|
||
| namespace CoreFx.Private.TestUtilities.Tests | ||
| { | ||
| public class AssertExtensionTests |
There was a problem hiding this comment.
Are these unit tests for the functionality of our test helpers? That shouldn't be necessary, and if it is then our helpers are too complex.
There was a problem hiding this comment.
Test helpers should always be tested- it is part of our infrastructure. What you don't need to do is test test methods.
There was a problem hiding this comment.
To be completely clear, where plausible and particularly in shared helpers.
There was a problem hiding this comment.
The regression risk should also be considered. In these extensions it is plausible for an error in logic to not be found immediately, potentially masking test failures for extended periods of time. We have a history of our test infrastructure doing just that.
|
|
||
| [Theory, | ||
| InlineData(" "), | ||
| InlineData("\r\n")] |
There was a problem hiding this comment.
I prefer having separate attributes, but it seems we aren't consisetnt...
| { | ||
| AssertExtensions.Throws<ArgumentException>("path", null, () => Path.GetDirectoryName(path)); | ||
| Action action = () => Path.GetDirectoryName(path); | ||
| if (PlatformDetection.IsWindows) |
There was a problem hiding this comment.
This should be using PlatformSpecific attributes instead.
Also the test name has "Windows" in it but runs on Unix.
There was a problem hiding this comment.
I don't agree in this case. This particular case is saying "throws on Windows but not Unix". Same input data. I could make the test name longer, but I think the fact that it isn't Windows only makes it implicit that it is "WindowsAndNotUnix".
I could split this into two entry points and shared test data property, but that seems like over-kill to me in this case. Same goes for duplicating the method. I think it is more coherent to have both sides of the coin together here.
* Tweak whitespace related tests Needed to update a few tests for unix whitespace behavior. * Fix more test issues Also add another assert extension for three exceptions.
|
Refer to #8655 for background on changes. |
|
@JeremyKuhne FYI: Fixed the milestone to 2.1. The change didn't make it into 2.0 branch. |
* Tweak whitespace related tests Needed to update a few tests for unix whitespace behavior. * Fix more test issues Also add another assert extension for three exceptions. Commit migrated from dotnet/corefx@449e350
7 participants
Needed to update a few tests for unix whitespace behavior.