Uh oh!
There was an error while loading. Please reload this page.
Test fix: FileStream DeleteOnClose run on OuterLoop with longer timeout - #60149
Conversation
ghost
commented
Oct 7, 2021
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFixes #60147 A new unit test was added in #55327 to fix a Unix-only bug. This PR ensures the test is limited to only run on Unix, in both Pending confirmation from @tmds that this unit test was only intended to be run on Unix.
|
Uh oh!
There was an error while loading. Please reload this page.
krwq
commented
Oct 11, 2021
Hey @carlossanlop what's the status? Can this be merged? |
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, but a simpler solution exists:
- [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsFileLockingEnabled), nameof(PlatformDetection.IsThreadingSupported))]+ [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsFileLockingEnabled), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsNotWindows))]tmds
commented
Oct 11, 2021
See discussion on #60147. Alternative to skipping the test on Windows is to increase the timeout, and move it to OuterLoop if the local run on Windows takes too long. (On Linux, the local run takes a couple of seconds). |
carlossanlop
commented
Oct 11, 2021
Ok, @tmds. Since we expect this test to pass on both Windows and Linux, I'll apply your suggestion:
cc @adamsitnik who already gave me a sign-off; don't merge it yet. |
There was a problem hiding this comment.
@tmds I'd like to assume this is the timeout you wanted me to extend? I ask because there are 3 Task.Delay calls above. Should I extend those as well? I wonder if 1 ms delays are long enough.
There was a problem hiding this comment.
This is the one.
For some reason when I read it I hear:
carlossanlop
commented
Oct 13, 2021
@kunalspathak can I ignore this build failure? It says you manually cancelled it. |
kunalspathak
commented
Oct 13, 2021
Yes, the pipeline is still under development and I forgot to have proper setting to not trigger it on every PR. |
Fixes#60147
A new unit test was added in #55327 to fix a Unix-only bug. This PR ensures the test is limited to only run on Unix, in both
System.IO.FileSystem.Tests.csprojand inSystem.IO.FileSystem.Net5Compat.Tests.csproj.Pending confirmation from @tmds that this unit test was only intended to be run on Unix.