Invalidate FileInfo and DirectoryInfo upon create and delete #34229

Jlalond · 2020-03-28T02:12:38Z

This is the change discussed in #31425

jkotas · 2020-03-28T02:54:46Z

src/libraries/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs

@@ -50,6 +50,12 @@ private void Init(string originalPath, string? fullPath = null, string? fileName
            _isNormalized = isNormalized;
        }

+        private void DeleteAndInvalidate(bool recurisve)


Typo: recurisve

And here I was thinking I could spell

jkotas · 2020-03-28T02:57:51Z

src/libraries/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs

@@ -235,8 +241,8 @@ public void MoveTo(string destDirName)
            Invalidate();
        }

-        public override void Delete() => FileSystem.RemoveDirectory(FullPath, recursive: false);
+        public override void Delete() => DeleteAndInvalidate(false);


Suggested change

public override void Delete() => DeleteAndInvalidate(false);

public override void Delete() => Delete(false);

You do not really need a new method for this.

Agreed. Changed

jkotas · 2020-03-28T02:58:57Z

src/libraries/System.IO.FileSystem/src/System/IO/FileSystemInfo.Windows.cs

@@ -141,6 +141,7 @@ internal long LengthCore
            }
        }

+


Extra new line?

…with paramter

stephentoub · 2020-03-28T18:11:02Z

src/libraries/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs

@@ -235,8 +235,12 @@ public void MoveTo(string destDirName)
            Invalidate();
        }

-        public override void Delete() => FileSystem.RemoveDirectory(FullPath, recursive: false);
+        public override void Delete() => Delete(false);


Suggested change

public override void Delete() => Delete(false);

public override void Delete() => Delete(recursive: false);

stephentoub · 2020-03-28T18:12:58Z

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

            info.Delete();
-            ValidateSetTimes(info, beforeTime, afterTime);
+            Assert.True(info.LastAccessTimeUtc > afterTime);


It'd be good to use Assert.InRange. If the assert fails, we'll then get meaningful informationa about why.

Also, what about the other times? ValidateSetTimes can't be used to validate those?

After some debugging I actually think we should bin this test. On my local machine the invalidated date time ends up being 01/01/1601 (W10), I could change it so the time isn't in the series, or is simple different than both but I think the over-all goal of the test has been invalidated.

Ah, we deleted the file. I think we have a test somewhere that validates the time we return for a non-existent file. Ideally we'd do the same check here. Otherwise, we can just delete the test.

@stephentoub after some digging I actually think something is incorrectly happening here. https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/src/System/IO/FileSystemInfo.Windows.cs#L111 the underlying OS specific filesysteminfo is checking if it's been initialized, and stepping through it's set to 0, vs -1 so I'm investigating that

Redacting my statement, I still think we should delete the test

Why wouldn't we want to validate the times behave as they would for a nonexistent file?

That makes sense to me too, but should be assert that it isn't in the range of those two times? Or just that the file has been defaulted to the system default

Presumably we should check for the same values as in the earlier tests in this file.
https://github.com/Jlalond/runtime/blob/f204026ab55bbeae1c62252ba67bf61fdeefda5b/src/libraries/System.IO.FileSystem/tests/Base/InfoGetSetTimes.cs#L24

Should've noticed that, eek. Implemented!

stephentoub · 2020-03-28T18:13:08Z

src/libraries/System.IO.FileSystem/tests/FileInfo/Delete.cs

+            DirectoryInfo testDir = Directory.CreateDirectory(GetTestFilePath());
+            string testFilePath = Path.Combine(testDir.FullName, GetTestFileName());
+            FileInfo info = new FileInfo(testFilePath);
+            using(FileStream fileStream = info.Create())


Suggested change

using(FileStream fileStream = info.Create())

using (FileStream fileStream = info.Create())

stephentoub · 2020-03-28T18:13:17Z

src/libraries/System.IO.FileSystem/tests/FileInfo/Create.cs

+            DirectoryInfo testDir = Directory.CreateDirectory(GetTestFilePath());
+            string testFilePath = Path.Combine(testDir.FullName, GetTestFileName());
+            FileInfo info = new FileInfo(testFilePath);
+            using(FileStream fileStream = info.Create())


Suggested change

using(FileStream fileStream = info.Create())

using (FileStream fileStream = info.Create())

src/libraries/System.IO.FileSystem/tests/Base/InfoGetSetTimes.cs

Jlalond · 2020-03-31T19:14:32Z

@stephentoub I think I implemented all your linting and I got all the tests working old and new, could I get a final review from you or @carlossanlop / @JeremyKuhne

Thanks!

stephentoub · 2020-04-01T16:18:32Z

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

            info.Delete();
-            ValidateSetTimes(info, beforeTime, afterTime);
+            Assert.NotInRange(info.LastAccessTimeUtc, beforeTime, afterTime);
+            Assert.Equal(info.LastAccessTimeUtc, DateTime.FromFileTimeUtc(0));


The two arguments here should be swapped; the first is expected, the second is actual.

stephentoub · 2020-04-01T16:18:44Z

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

            info.Delete();
-            ValidateSetTimes(info, beforeTime, afterTime);
+            Assert.NotInRange(info.LastAccessTimeUtc, beforeTime, afterTime);


This assert can be deleted.

stephentoub

A couple of small comments, otherwise LGTM. Thanks.

Jlalond · 2020-04-10T14:02:40Z

@jaredpar would you mind bumping this build

Jlalond · 2020-04-10T16:14:24Z

@stephentoub I flipped the unit test and removed the useless Assert, so I think we're good to merge on my end

danmoseley · 2020-04-10T16:41:58Z

thank you @Jlalond

Jlalond · 2020-04-10T16:48:11Z

Anytime @danmosemsft! I'll pick up a new issue shortly

danmoseley · 2020-04-10T16:52:05Z

@Jlalond sounds good to me - we have plenty marked up for grabs.

Jlalond added 3 commits March 27, 2020 22:10

Invalidate upon delete and create for fileinfo and directoryinfo

f81f86e

Add tests, remove directory info test that tested removed behavior

9bec8c5

Add delete test

1cde8e8

Dotnet-GitSync-Bot added the area-System.IO label Mar 28, 2020

jkotas reviewed Mar 28, 2020

View reviewed changes

Fix typoe in directoryinfo

e132007

jkotas reviewed Mar 28, 2020

View reviewed changes

Jlalond added 2 commits March 27, 2020 23:01

Remove new line, and refactor away private method into delete method …

ea746cc

…with paramter

Change set time tests

f204026

stephentoub reviewed Mar 28, 2020

View reviewed changes

Jlalond added 3 commits March 28, 2020 18:24

Code review linting as well as test change

c9f8935

Modify other test that tested prior behavior

2d1f527

Rework Reset test

03c68b7

Jlalond commented Mar 29, 2020

View reviewed changes

src/libraries/System.IO.FileSystem/tests/Base/InfoGetSetTimes.cs Outdated Show resolved Hide resolved

Jlalond added 2 commits March 29, 2020 20:56

Edits for comments in tests

9d1d82d

Slight method refactor

e914b47

stephentoub reviewed Apr 1, 2020

View reviewed changes

stephentoub approved these changes Apr 1, 2020

View reviewed changes

carlossanlop mentioned this pull request Apr 1, 2020

Update DirectoryInfo and FileInfo Create, Delete and Exists changed behavior dotnet/dotnet-api-docs#4061

Open

Minor test cleanup

1c8242a

jaredpar mentioned this pull request Apr 2, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

stephentoub closed this Apr 10, 2020

stephentoub reopened this Apr 10, 2020

danmoseley merged commit 1b683ec into dotnet:master Apr 10, 2020

Jlalond deleted the fileinfo-delete branch April 10, 2020 16:48

andrew-boyarshin mentioned this pull request Jun 19, 2020

FileInfo.Delete() does not update .Exists #31425

Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalidate FileInfo and DirectoryInfo upon create and delete #34229

Invalidate FileInfo and DirectoryInfo upon create and delete #34229

Jlalond commented Mar 28, 2020

jkotas Mar 28, 2020

Jlalond Mar 28, 2020

jkotas Mar 28, 2020

Jlalond Mar 28, 2020

jkotas Mar 28, 2020

stephentoub Mar 28, 2020

stephentoub Mar 28, 2020

Jlalond Mar 28, 2020

stephentoub Mar 28, 2020

Jlalond Mar 28, 2020

Jlalond Mar 28, 2020

stephentoub Mar 28, 2020

Jlalond Mar 28, 2020

stephentoub Mar 28, 2020

Jlalond Mar 28, 2020

stephentoub Mar 28, 2020

stephentoub Mar 28, 2020

Jlalond commented Mar 31, 2020

stephentoub Apr 1, 2020

stephentoub Apr 1, 2020

stephentoub left a comment

Jlalond commented Apr 10, 2020

Jlalond commented Apr 10, 2020

danmoseley commented Apr 10, 2020

Jlalond commented Apr 10, 2020

danmoseley commented Apr 10, 2020

	public override void Delete() => DeleteAndInvalidate(false);
	public override void Delete() => Delete(false);

	using(FileStream fileStream = info.Create())
	using (FileStream fileStream = info.Create())

Invalidate FileInfo and DirectoryInfo upon create and delete #34229

Invalidate FileInfo and DirectoryInfo upon create and delete #34229

Conversation

Jlalond commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jlalond commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

Jlalond commented Apr 10, 2020

Jlalond commented Apr 10, 2020

danmoseley commented Apr 10, 2020

Jlalond commented Apr 10, 2020

danmoseley commented Apr 10, 2020