-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Tweak path related tests #22407
Tweak path related tests #22407
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using Xunit; | ||
| using Xunit.Sdk; | ||
|
|
||
| namespace CoreFx.Private.TestUtilities.Tests | ||
| { | ||
| public class AssertExtensionTests | ||
| { | ||
| public class OneException : Exception { } | ||
|
|
||
| public class TwoException : Exception { } | ||
|
|
||
| public class ThreeException : Exception { } | ||
|
|
||
| public class NotMyException : Exception { } | ||
|
|
||
| [Fact] | ||
| public void ThrowsAny_Two_ThrowsExpected() | ||
| { | ||
| Action throwExpected = () => { throw new OneException(); }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException>(throwExpected); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Assert.False(true, $"Should not have thrown for first, threw {e}"); | ||
| } | ||
|
|
||
| throwExpected = () => { throw new TwoException(); }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException>(throwExpected); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Assert.False(true, $"Should not have thrown for second, threw {e}"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ThrowsAny_Two_ThrowsUnexpected() | ||
| { | ||
| Action throwUnexpected = () => { throw new NotMyException(); }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException>(throwUnexpected); | ||
| } | ||
| catch (XunitException e) | ||
| { | ||
| Assert.Equal( | ||
| "Expected one of: (CoreFx.Private.TestUtilities.Tests.AssertExtensionTests+OneException, CoreFx.Private.TestUtilities.Tests.AssertExtensionTests+TwoException) -> Actual: (CoreFx.Private.TestUtilities.Tests.AssertExtensionTests+NotMyException)", | ||
| e.Message); | ||
| return; | ||
| } | ||
|
|
||
| Assert.False(true, "Should have thrown"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ThrowsAny_Two_DoesNotThrow() | ||
| { | ||
| Action doNothing = () => { }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException>(doNothing); | ||
| } | ||
| catch (XunitException e) | ||
| { | ||
| Assert.Equal( | ||
| "Expected one of: (CoreFx.Private.TestUtilities.Tests.AssertExtensionTests+OneException, CoreFx.Private.TestUtilities.Tests.AssertExtensionTests+TwoException) -> Actual: No exception thrown", | ||
| e.Message); | ||
| return; | ||
| } | ||
|
|
||
| Assert.False(true, "Should have thrown"); | ||
| } | ||
|
|
||
|
|
||
| [Fact] | ||
| public void ThrowsAny_Three_ThrowsExpected() | ||
| { | ||
| Action throwExpected = () => { throw new ThreeException(); }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException, ThreeException>(throwExpected); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Assert.False(true, $"Should not have thrown for first, threw {e}"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ThrowsAny_Three_ThrowsUnexpected() | ||
| { | ||
| Action throwUnexpected = () => { throw new NotMyException(); }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException, ThreeException>(throwUnexpected); | ||
| } | ||
| catch (XunitException) | ||
| { | ||
| // We're ok | ||
| return; | ||
| } | ||
|
|
||
| Assert.False(true, "Should have thrown"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ThrowsAny_Three_DoesNotThrow() | ||
| { | ||
| Action doNothing = () => { }; | ||
| try | ||
| { | ||
| AssertExtensions.ThrowsAny<OneException, TwoException, ThreeException>(doNothing); | ||
| } | ||
| catch (XunitException) | ||
| { | ||
| // We're ok | ||
| return; | ||
| } | ||
|
|
||
| Assert.False(true, "Should have thrown"); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,13 +35,28 @@ public static void ChangeExtension(string path, string newExtension, string expe | |
| Assert.Equal(expected, Path.ChangeExtension(path, newExtension)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("")] | ||
| [InlineData(" ")] | ||
| [InlineData("\r\n")] | ||
| public static void GetDirectoryName_EmptyOrWhitespace_Throws(string path) | ||
| [Fact] | ||
| public static void GetDirectoryName_EmptyThrows() | ||
| { | ||
| AssertExtensions.Throws<ArgumentException>("path", null, () => Path.GetDirectoryName(string.Empty)); | ||
| } | ||
|
|
||
| [Theory, | ||
| InlineData(" "), | ||
| InlineData("\r\n")] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer having separate attributes, but it seems we aren't consisetnt...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
| [ActiveIssue(21358)] // Pending CoreCLR behavior change | ||
| public static void GetDirectoryName_SpaceOrControlCharsThrowOnWindows(string path) | ||
| { | ||
| AssertExtensions.Throws<ArgumentException>("path", null, () => Path.GetDirectoryName(path)); | ||
| Action action = () => Path.GetDirectoryName(path); | ||
| if (PlatformDetection.IsWindows) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be using PlatformSpecific attributes instead. Also the test name has "Windows" in it but runs on Unix.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| { | ||
| AssertExtensions.Throws<ArgumentException>("path", null, () => Path.GetDirectoryName(path)); | ||
| } | ||
| else | ||
| { | ||
| // These are valid paths on Unix | ||
| action(); | ||
| } | ||
| } | ||
|
|
||
| [Theory] | ||
|
|
@@ -201,12 +216,20 @@ public static void GetFileNameWithoutExtension(string path, string expected) | |
| } | ||
|
|
||
| [Fact] | ||
| public static void GetPathRoot() | ||
| public static void GetPathRoot_NullReturnsNull() | ||
| { | ||
| Assert.Null(Path.GetPathRoot(null)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void GetPathRoot_EmptyThrows() | ||
| { | ||
| AssertExtensions.Throws<ArgumentException>("path", null, () => Path.GetPathRoot(string.Empty)); | ||
| AssertExtensions.Throws<ArgumentException>("path", null, () => Path.GetPathRoot("\r\n")); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void GetPathRoot_Basic() | ||
| { | ||
| string cwd = Directory.GetCurrentDirectory(); | ||
| Assert.Equal(cwd.Substring(0, cwd.IndexOf(Path.DirectorySeparatorChar) + 1), Path.GetPathRoot(cwd)); | ||
| Assert.True(Path.IsPathRooted(cwd)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be completely clear, where plausible and particularly in shared helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.