Skip to content

Delete unnecessary seek from FileStream.SetLengthInternal on Unix #44097

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.

Sign up for GitHub

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

Jump to bottom
Merged
jkotas merged 1 commit into from
Nov 1, 2020
Merged

Delete unnecessary seek from FileStream.SetLengthInternal on Unix #44097

jkotas 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
stephentoub
stephentoub approved these changes Oct 31, 2020
View reviewed changes
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 jkotas mentioned this pull request Oct 31, 2020
Closed
@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

stephentoub commented Oct 31, 2020 

    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)

stephentoub
stephentoub reviewed Oct 31, 2020
View reviewed changes
src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateNew.Tests.cs Outdated
Copy link
Member

@stephentoub stephentoub Oct 31, 2020

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:

runtime/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs

Lines 195 to 216 in 22c3105

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
@jkotas
Delete unnecessary seek from SetLengthInternal on Unix
4ae2738 
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 mentioned this pull request Nov 4, 2020
Closed
@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.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@jozkee jozkee Awaiting requested review from jozkee

Assignees

No one assigned

Labels

area-System.IO

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jkotas @stephentoub @jozkee @Dotnet-GitSync-Bot