Updates Unix FileSystem tests that check millisecond granularity #30763
Conversation
| Assert.All(TimeFunctions(), (function) => | ||
| { | ||
| DateTime time = function.Getter(item); | ||
| Assert.NotEqual(0, time.Millisecond); |
There was a problem hiding this comment.
Can you explain this more? Why are we verifying it's equal on hfs and not equal on everything else?
There was a problem hiding this comment.
You can't rely on milliseconds != 0 -- it will fail 1 in 1000 times. It needs to use the same approach as TimesIncludeMillisecondPart_Windows.
There was a problem hiding this comment.
Is it that every other file system other than hfs on macOS support milliseconds times?
If so,
a) seems like a potential reliability issue to assert NotEqual, as 0 is a valid number of milliseconds, so this test would theoretically fail 1/1000 times, which will make this very flaky in CI where the test potentially runs hundreds of times a day.
b) seems like the test should really be refactored, e.g. so that https://github.com/dotnet/corefx/pull/30763/files#diff-170a22c34cffbe331bbbc9364e76aa75L74 is used when on non-hfs and this test remains for hfs.
| // if this ever changes so we can enable the actual test | ||
| [PlatformSpecific(TestPlatforms.OSX)] | ||
| public void TimesIncludeMillisecondPart_OSX() | ||
| { |
There was a problem hiding this comment.
comment above is now outdated
| Assert.Equal(0, time.Millisecond); | ||
| }); | ||
| } | ||
| else |
There was a problem hiding this comment.
I actually think it's not worth bothering doing anything on hfs. Just make the test skip if driveFormat != apfs.
- non any linux or - OSX non-HFS driver formats Fixes dotnet#30472
maryamariyan
force-pushed
the
fix-filesystem
branch
from
de96dec to
4a41c53
Compare
maryamariyan
changed the title
maryamariyan
changed the title
maryamariyan
changed the title
|
The windows test failure in System.IO.Pipelines.Tests is unrelated to this PR. |
| string driveFormat = new DriveInfo(GetItemPath(item)).DriveFormat; | ||
|
|
||
| Assert.All(TimeFunctions(), (function) => | ||
| if (!PlatformDetection.IsOSX || |
There was a problem hiding this comment.
Style nit, sometimes !(A && B) is easier to read than !A || !B. In this case I read "if it's not Mac or it's not HFS" whereas what a human would say is "if it's not HFS on Mac" so I believe
!(PlatformDetection.IsOSX &&
driveFormat.Equals(HFS, StringComparison.InvariantCultureIgnoreCase)may be easier to read.
There was a problem hiding this comment.
No need to go back to fix, just general observation
|
thanks! |
- non any linux or - OSX non-HFS driver formats Fixes dotnet#30472
- non any linux or - OSX non-HFS driver formats Fixes dotnet/corefx#30472 Commit migrated from dotnet/corefx@a399e51
4 participants
Fixing osx filesystem test customizing assert per driver format.
Switching assert for different driver formats
Fixes #30742