Skip to content
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

fix: set last access time with unspecified date time kind #879

Closed
wants to merge 5 commits into from
Closed

fix: set last access time with unspecified date time kind #879

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55ca1ab
Fix missing conversion call
xtopaz Aug 25, 2022
dbd1a6a
Add unit tests
xtopaz Aug 25, 2022
6cc2b91
Merge branch 'main' into bugfix/setLastWriteTimeWithUnspecifiedDateTi…
xtopaz Aug 26, 2022
86a509e
Merge branch 'main' into bugfix/setLastWriteTimeWithUnspecifiedDateTi…
xtopaz Sep 13, 2022
c939931
Adjust DateTimeKind.Unspecified
vbtig Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/System.IO.Abstractions.TestingHelpers/MockFileInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public override DateTime LastAccessTimeUtc
set
{
var mockFileData = GetMockFileDataForWrite();
mockFileData.LastAccessTime = value;
mockFileData.LastAccessTime = value.ToLocalTime();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also do this for the other *TimeUtc properties?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether ToLocalTime is correct here. I think we rather want something like this: https://github.com/dotnet/runtime/blob/96d1c8e3ba4bc94afdcf15ee2896a3bc984820e7/src/libraries/System.Private.CoreLib/src/System/IO/FileSystemInfo.cs#L108

Copy link
Contributor Author

@xtopaz xtopaz Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fgreinacher Sorry for the late reply, just got back from vacation.

All other *TimeUtc properties already contain the ToLocalTime call. This way the behavior has been unified.

File.File.GetUtcDateTimeOffsetcannot be used because it is internal. However, this behaves analogously to the proposed implementation, since it takes Utc for DateTimeKind.Unspecified.

To be more precise, the preliminary conversion using ToLocalTime for values with DateTimeKind.Unspecified assumes UTC. The subsequent implicit conversion to DateTimeOffset then takes the time zone into account correctly. Thus the timestamps behave analogously to those in System.FileInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fgreinacher I've incorporated the recent changes from #875 and all test look fine.

}
}

Expand Down
51 changes: 51 additions & 0 deletions tests/System.IO.Abstractions.TestingHelpers.Tests/MockFileInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,57 @@ public void MockFileInfo_LastAccessTimeUtc_ShouldSetCreationTimeUtcOfFileInMemor
Assert.AreEqual(newUtcTime, fileInfo.LastAccessTimeUtc);
}

[Test]
public void MockFileInfo_CreationTimeUtcWithUnspecifiedDateTimeKind_ShouldSetCreationTimeUtcOfFileInMemoryFileSystem()
{
var date = DateTime.SpecifyKind(DateTime.Now.AddHours(-4), DateTimeKind.Unspecified);
var fileData = new MockFileData("Demo text content");
var fileSystem = new MockFileSystem(new Dictionary<string, MockFileData>
{
{ MockUnixSupport.Path(@"c:\a.txt"), fileData }
});
var fileInfo = new MockFileInfo(fileSystem, MockUnixSupport.Path(@"c:\a.txt"))
{
CreationTimeUtc = date,
};

Assert.AreEqual(date, fileInfo.CreationTimeUtc);
}

[Test]
public void MockFileInfo_LastAccessTimeUtcWithUnspecifiedDateTimeKind_ShouldSetLastAccessTimeUtcOfFileInMemoryFileSystem()
{
var date = DateTime.SpecifyKind(DateTime.Now.AddHours(-4), DateTimeKind.Unspecified);
var fileData = new MockFileData("Demo text content");
var fileSystem = new MockFileSystem(new Dictionary<string, MockFileData>
{
{ MockUnixSupport.Path(@"c:\a.txt"), fileData }
});
var fileInfo = new MockFileInfo(fileSystem, MockUnixSupport.Path(@"c:\a.txt"))
{
LastAccessTimeUtc = date
};

Assert.AreEqual(date, fileInfo.LastAccessTimeUtc);
}

[Test]
public void MockFileInfo_LastWriteTimeUtcWithUnspecifiedDateTimeKind_ShouldSetLastWriteTimeUtcOfFileInMemoryFileSystem()
{
var date = DateTime.SpecifyKind(DateTime.Now.AddHours(-4), DateTimeKind.Unspecified);
var fileData = new MockFileData("Demo text content");
var fileSystem = new MockFileSystem(new Dictionary<string, MockFileData>
{
{ MockUnixSupport.Path(@"c:\a.txt"), fileData }
});
var fileInfo = new MockFileInfo(fileSystem, MockUnixSupport.Path(@"c:\a.txt"))
{
LastWriteTimeUtc = date,
};

Assert.AreEqual(date, fileInfo.LastWriteTimeUtc);
}

[Test]
public void MockFileInfo_LastWriteTime_ShouldReturnDefaultTimeForNonExistingFile()
{
Expand Down