[release/2.1] Fix Granularity for copy operation on unix #31046

Anipik · 2018-07-13T05:06:35Z

Fixes https://github.com/dotnet/corefx/issues/12403
This PR ports both of below PRs
#30996
#31164

stephentoub · 2018-07-13T12:42:15Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+        [Fact]
+        public void CopyToMillisecondPresent()
+        {
+            FileInfo input = new FileInfo(Path.Combine(TestDirectory, GetTestFileName()));


This can use GetTestFilePath(): it does the Path.Combine of the TestDirectory with GetTestFileName().

stephentoub · 2018-07-13T12:43:33Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+            input.Create().Dispose();
+
+            string driveFormat = new DriveInfo(input.DirectoryName).DriveFormat;
+            if (!driveFormat.Equals(HFS, StringComparison.InvariantCultureIgnoreCase))


If we're going to skip this test on HFS, it'd be better to make this a ConditionalFact that's only executed when the drive format != HFS. (And if for some reason we choose not to do that, the above file creation should be moved to within this if block... there's no benefit to creating the file if we're not going to use it.)

will this be okay

private static bool isHFS = new DriveInfo(Path.GetTempPath()).DriveFormat.Equals(HFS, StringComparison.InvariantCultureIgnoreCase);

stephentoub · 2018-07-13T12:43:57Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+                    // not support millisecond granularity.
+
+                    // If it's 1/1000, or low granularity, this may help:
+                    Thread.Sleep(1234);


Why not just Thread.Sleep(1)?

I wasn't sure what the lowest granularity was for all the file systems this might run on. It only happens 1/1000 times, of course.

stephentoub · 2018-07-13T12:45:39Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+                    // If it's 1/1000, or low granularity, this may help:
+                    Thread.Sleep(1234);
+                    input = new FileInfo(Path.Combine(TestDirectory, GetTestFileName()));
+                    input.Create().Dispose();


Why not just call input.Create().Dispose() again on the original input FileInfo?

stephentoub · 2018-07-13T12:45:52Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+                    input.Create().Dispose();
+                }
+
+                FileInfo output = new FileInfo(Path.Combine(TestDirectory, GetTestFileName(), input.Name));


Same GetTestFilePath comment.

stephentoub · 2018-07-13T12:47:26Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+                output = input.CopyTo(output.FullName, true);
+
+                Assert.NotEqual(0, input.LastWriteTime.Millisecond);
+                Assert.NotEqual(0, output.LastWriteTime.Millisecond);


Wouldn't it be better to check that Assert.Equal(input.LastWriteTime, output.LastWriteTime)?

I didn't go for that in order for the test fail if for some reason the millisecond granularity is not working. if we go go for equality. the test will still pass if they are zero

stephentoub · 2018-07-13T12:48:19Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

+            FileInfo output = new FileInfo(Path.Combine(TestDirectory, GetTestFileName(), input.Name));
+
+            string driveFormat = new DriveInfo(input.DirectoryName).DriveFormat;
+            if (driveFormat.Equals(HFS, StringComparison.InvariantCultureIgnoreCase))


Similar ConditionalFact comment. Or if you don't want to use a ConditionalFact, you can just combine the two tests, with one if branch for if it's on HFS, and an else for if it's not.

Anipik · 2018-07-13T19:39:07Z

@dotnet-bot test NETFX x86 Release Build

Anipik · 2018-07-17T04:23:04Z

@danmosemsft when can we merge this one ?

danmoseley · 2018-07-17T18:53:38Z

We are going to sit on this until we see customer impact.

Anipik · 2018-07-19T18:57:13Z

@danmosemsft can you take a look at the formatting done to the test so that we can merge after that (As it has been approved)

danmoseley · 2018-07-19T19:46:08Z

src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs

 {
    public class FileInfo_GetSetTimes : InfoGetSetTimes<FileInfo>
    {
+        private static bool isHFS => new DriveInfo(Path.GetTempPath()).DriveFormat.Equals(HFS, StringComparison.InvariantCultureIgnoreCase);


I noticed DriveFormat does not work in AppContainer. I was going to put up a PR to fix that
https://github.com/dotnet/corefx/compare/master...danmosemsft:fix.for.uap?expand=1

I suggest waiting until that's in then pulling that into this PR. The branch isn't open yet anyway

…31164) * Feedback Addressed * Fixing original tests and for appContainers * comment corrected

Anipik · 2018-07-25T04:27:43Z

@dotnet-bot test OSX x64 Debug Build

Fix Granualarity on unix

493dacf

Anipik requested review from danmoseley and stephentoub July 13, 2018 05:06

danmoseley approved these changes Jul 13, 2018

View reviewed changes

stephentoub changed the title ~~Fix Granularity for copy operation on unix~~ [release/2.1] Fix Granularity for copy operation on unix Jul 13, 2018

stephentoub reviewed Jul 13, 2018

View reviewed changes

danmoseley added this to the 2.1.x milestone Jul 17, 2018

danmoseley added the Servicing-consider Issue for next servicing release review label Jul 17, 2018

danmoseley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 17, 2018

Anipik mentioned this pull request Jul 18, 2018

Maintaining the consistency of tests among master and release branch #31164

Merged

danmoseley reviewed Jul 19, 2018

View reviewed changes

Maintaining the consistency of tests among master and release branch (#…

4436c64

…31164) * Feedback Addressed * Fixing original tests and for appContainers * comment corrected

danmoseley removed the Servicing-consider Issue for next servicing release review label Jul 25, 2018

This was referenced Aug 8, 2018

Cherry-picking test fix for System.IO.FileSystem from master to release2.2 #31657

Merged

[release/2.1] SetLastAccessTimeUtc and SetLastModifiedTimeUtc corrected to set Milliseconds #31569

Closed

danmoseley added the Servicing-approved-2.1.5 label Aug 14, 2018

karelz added the area-System.IO label Aug 15, 2018

karelz assigned Anipik Aug 15, 2018

danmoseley removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 28, 2018

danmoseley merged commit 83ac6fa into dotnet:release/2.1 Aug 28, 2018

danmoseley modified the milestones: 2.1.x, 2.1.5 Sep 13, 2018

danmoseley added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release Servicing-approved-2.1.5 labels Sep 13, 2018

Anipik deleted the releaseFixTest branch February 26, 2019 19:30

[release/2.1] Fix Granularity for copy operation on unix #31046

[release/2.1] Fix Granularity for copy operation on unix #31046

Uh oh!

Conversation

Anipik commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jul 13, 2018

Uh oh!

Anipik commented Jul 17, 2018

Uh oh!

danmoseley commented Jul 17, 2018

Uh oh!

Anipik commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Anipik commented Jul 13, 2018 •

edited

Loading

Anipik Jul 13, 2018 •

edited

Loading