Path changes resulting in failing DriveInfo test

[stephentoub]

dotnet/corefx#27339 (comment)
https://mc.dot.net/#/user/dotnet-maestro-bot/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/e91038db253e351822d63ed2cf875f94cb0cabc7/workItem/System.IO.FileSystem.DriveInfo.Tests/analysis/xunit/System.IO.FileSystem.DriveInfoTests.DriveInfoWindowsTests~2FCtor_InvalidPath_ThrowsArgumentException(driveName:%20%5C%22%5C%22,%20paramName:%20%5C%22path%5C%22)

Unhandled Exception of Type Xunit.Sdk.EqualException
Message :
Assert.Equal() Failure
Expected: path
Actual:   (null)
Stack Trace :
   at System.AssertExtensions.Throws[T](String netCoreParamName, String netFxParamName, Action action) in D:\j\workspace\windows-TGrou---f8ac6754\src\CoreFx.Private.TestUtilities\src\System\AssertExtensions.cs:line 39
   at System.IO.FileSystem.DriveInfoTests.DriveInfoWindowsTests.Ctor_InvalidPath_ThrowsArgumentException(String driveName, String paramName) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.IO.FileSystem.DriveInfo\tests\DriveInfo.Windows.Tests.cs:line 35

[danmoseley]

@Anipik maybe you can fix this real quick?

[stephentoub]

Do we know why the the argument name is no longer being set? Presumably there was some coreclr change that removed the argument name, and if so, why?

[danmoseley]

Likely a minor error in the Path changes recently. eg

C:\git\coreclr\src\mscorlib>git show 1709ce9e53590adb5d649ddf99fcddf652b2fbc4 |  \t\grep "nameof(path)"
-                throw new ArgumentException(SR.Arg_PathEmpty, nameof(path));
-                throw new ArgumentException(SR.Arg_PathEmpty, nameof(path));
-                throw new ArgumentException(SR.Arg_PathEmpty, nameof(path));
                 throw new ArgumentNullException(nameof(path));

[danmoseley]

Uh, the above doesn't prove anything except there were relevant changes

[danmoseley]

BTW, there's a lot here that never had a param, and ideally would:

        [InlineData(":", null)]
        [InlineData("://", null)]
        [InlineData(@":\", null)]
        [InlineData(":/", null)]
        [InlineData(@":\\", null)]
        [InlineData("Az", null)]
        [InlineData("1", null)]
        [InlineData("a1", null)]
        [InlineData(@"\\share", null)]
        [InlineData(@"\\", null)]
        [InlineData("c ", null)]
        [InlineData("", "path")]
        [InlineData(" c", null)]
        public void Ctor_InvalidPath_ThrowsArgumentException(string driveName, string paramName)

[Anipik]

@danmosemsft do i need to find the cause why the paramter name is no longer thrown or just change the assert statement to not check for paramter name ?

[Anipik]

@stephentoub @danmosemsft It was changed by @JeremyKuhne in dotnet/coreclr@1709ce9
He has mentioned the reason in the description.

[danmoseley]

@Anipik I don't see that. In that change he removed some places that throw, which is OK. However if an ArgumentException (or any of its derived types eg ArgumentNullException) is thrown, ideally it has its parameter name field set. Sometimes that isn't possible, usually because it's deep in the callstack so it doesn't have an argument name that's useful to the user. I'm guessing that the list above, though, could have parameters on their exceptions.

[Anipik]

Earlier the exception getting thrown occured at this place dotnet/coreclr@1709ce9#diff-6836e42e6acde3dd212e4e6e012906ccL203
where it passes as the "path" as argument name whereas now it is returning null so exception is thrown at
https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem.DriveInfo/src/System/IO/DriveInfo.Windows.cs#L27

if we want to include the argument name in the exception i suggest changing this line to

if (name == null || name.Length == 0 || name.StartsWith("\\\\", StringComparison.Ordinal))
         throw new ArgumentException(SR.Arg_MustBeDriveLetterOrRootDir, nameof(driveName));