Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete unnecessary seek from FileStream.SetLengthInternal on Unix #44097

Merged
merged 1 commit into from
Nov 1, 2020

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Oct 31, 2020

No description provided.

@jkotas jkotas changed the title Delete unnecessary seek from FIleStream.SetLengthInternal on Unix Delete unnecessary seek from FileStream.SetLengthInternal on Unix Oct 31, 2020
@jkotas jkotas requested a review from jozkee October 31, 2020 02:30
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me once CI is green. I'm trying to remember why all this was deemed necessary and coming up empty.

@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2020

I suspect that it was just copy&paste from Windows where we can avoid it as well - #43863 (comment).

@stephentoub
Copy link
Member

    System.IO.Tests.FileStream_SetLength.InvalidLengths [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.ArgumentOutOfRangeException)
      Actual:   (No exception was thrown)
      Stack Trace:
        /_/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs(69,0): at System.AssertExtensions.Throws[T](String expectedParamName, Action action)
        /_/src/libraries/System.IO.FileSystem/tests/FileStream/SetLength.cs(19,0): at System.IO.Tests.FileStream_SetLength.InvalidLengths()
    System.IO.MemoryMappedFiles.Tests.MemoryMappedFileTests_CreateNew.TooLargeCapacity_Unix [FAIL]
      Expected one of: (System.IO.IOException, System.ArgumentException) -> Actual: (System.ArgumentOutOfRangeException)
      Stack Trace:
        /_/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs(212,0): at System.AssertExtensions.ThrowsAnyInternal(Action action, Type[] exceptionTypes)
        /_/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs(222,0): at System.AssertExtensions.ThrowsAny[IOException,ArgumentException](Action action)
        /_/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateNew.Tests.cs(115,0): at System.IO.MemoryMappedFiles.Tests.MemoryMappedFileTests_CreateNew.TooLargeCapacity_Unix()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(378,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -112,7 +112,7 @@ public void TooLargeCapacity_Unix()
// due to differences in OS behaviors and Unix not actually having a notion of
// a view separate from a map. It could also come from CreateNew, depending
// on what backing store is being used.
Assert.Throws<IOException>(() =>
AssertExtensions.ThrowsAny<IOException, ArgumentException>(() =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this method is poorly named, in that whereas Assert.ThrowsAny<T> allows for the exception to be T or anything derived from it, AssertExtensions.ThrowsAny<T1, T2> is requiring the concrete type of T1 or T2:

public static void ThrowsAny(Type firstExceptionType, Type secondExceptionType, Action action)
{
ThrowsAnyInternal(action, firstExceptionType, secondExceptionType);
}
private static void ThrowsAnyInternal(Action action, params Type[] exceptionTypes)
{
try
{
action();
}
catch (Exception e)
{
Type exceptionType = e.GetType();
if (exceptionTypes.Any(t => t.Equals(exceptionType)))
return;
throw new XunitException($"Expected one of: ({string.Join<Type>(", ", exceptionTypes)}) -> Actual: ({exceptionType})");
}
throw new XunitException($"Expected one of: ({string.Join<Type>(", ", exceptionTypes)}) -> Actual: No exception thrown");
}

which is then failing if the exception is actually an ArgumentOutOfRangeException.

@jkotas jkotas force-pushed the SetLengthInternal branch from ae6acc1 to 5901a0c Compare October 31, 2020 16:49
Also, improve test coverage for FileStream.SetLength
@jkotas jkotas force-pushed the SetLengthInternal branch from ec6d800 to 4ae2738 Compare November 1, 2020 04:48
@jkotas jkotas merged commit c03edca into dotnet:master Nov 1, 2020
@jkotas jkotas deleted the SetLengthInternal branch November 1, 2020 14:08
@jozkee jozkee added this to the 6.0.0 milestone Nov 5, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants