Implement Creation, LastAccess and LastWrite Dates on MockDirectoryInfo #311

robkeim · 2018-07-09T14:56:14Z

This contains the following changes:

Original changes from Implement Creation, LastAccess and LastWrite dates on MockDirectoryInfo #157
Auto-formatting from Visual Studio
Moving to use properties with default values (leveraging new C# 6 syntax)

fgreinacher · 2018-07-09T18:32:00Z

Thanks - looks good from my side. Waiting for an additional pair of eyes to review.

jpreese · 2018-07-09T18:42:06Z

TestingHelpers/MockDirectoryInfo.cs

+            {
+                if (!returnNullObject)
+                {
+                    throw new FileNotFoundException("File not found", directoryPath);


I would use the string in the Resources.resx file

jpreese · 2018-07-09T18:42:20Z

TestingHelpers/MockDirectoryInfo.cs

        }

-        MockFileData MockFileData
+        MockFileData GetMockFileData(bool returnNullObject)


Could we get private added to this?

Oh, I also wanted to bring up as food for thought. I'm usually pretty against parameters that change behaviors within a method. Usually parameters like this where its a boolean and branches of true/false. To me, it's a little confusing to say..

GetMockFileData(true); // gasp! what does true do?!

You have to dive into the implementation to figure it out, rather than just read the intention. A common fix for this is to just have two separate methods. In this case, one for returning the null object (always used in the getter), and one for not (always used in the setter). GetMockFileDataWithExceptionChecking, who knows, naming is hard.

Though after typing all this out, it got me to thinking. Why is the behavior different for getting and setting when it comes to returning a NullObject? I would think we'd return FileNotFound for either case?

Great question @jpreese, but I don't have the answer right now. I think I'm going to have to do some more digging to get a better understanding of the overall project and where this PR fits into that puzzle.

jpreese · 2018-07-09T18:44:55Z

TestHelpers.Tests/MockDirectoryInfoTests.cs

+            Assert.AreEqual(newTime, directoryInfo.LastWriteTime);
+        }
+
+        static void ExecuteDefaultValueTest<T>(Func<MockDirectoryInfo, T> getDateValue, T expected)


Could we get private added to this?

jpreese · 2018-07-09T18:45:01Z

TestHelpers.Tests/MockDirectoryInfoTests.cs

+            Assert.AreEqual(expected, actual);
+        }
+
+        static void ExecuteSetDefaultValueThrowsTest(Action<MockDirectoryInfo> setDateValue)


Could we get private added to this?

jpreese · 2018-07-09T18:49:15Z

TestHelpers.Tests/MockDirectoryInfoTests.cs

+        [Test]
+        public void MockDirectoryInfo_Attributes_ShouldReturnDefaultAttributeIfNotExists()
+        {
+            ExecuteDefaultValueTest((d) => d.Attributes, MockFileData.NullObject.Attributes);


This might start a bit of an opinion war, but I'm not a huge fan of this when writing tests. I dont so much mind hiding setup stuff behind a

var directoryInfo = CreateDefaultDirectoryInfo(); directoryInfo.Attributes // whatever act stuff Assert(//whatever assert)

But this approach seems a little confusing to read. Not sure what everyone else's thoughts are.

I tend to agree that in test code favoring readability at the expense of redundancy (code duplication) is not a bad thing. I'll go up ahead and update this.

jpreese · 2018-07-09T18:50:01Z

I have a couple for sure changes, small nits. And then a quick discussion about execution helpers.

jpreese · 2018-07-10T17:09:34Z

@robkeim Yeah this looks a lot better to me. The only lingering question I have is the GetMockFileData boolean switch between a NullObject and not. It just seems strange.. if the file can't be found why can we get the attributes, but not set them.

System.IO behavior:

Directory.SetLastWriteTimeUtc(@"C:\foo\bar", new DateTime()); //throws DirectoryNotFoundException
Directory.GetLastWriteTimeUtc(@"C:\foo\bar"); //returns a datetime of 1/1/1601 @ 12:00AM

So that would explain that. With that in mind, it seems like we'd want a DirectoryNotFoundException because thats the behavior I'm seeing. Which is a little weird because it's not listed in their documentation https://msdn.microsoft.com/en-us/library/system.io.directory.setlastwritetimeutc(v=vs.110).aspx (FileException is).

Edit: On a funnier note, if you call either of these methods with a file, i.e.

Directory.SetLastWriteTimeUtc(@"C:\foo\bar.txt", new DateTime()); // bar.txt does not exist and a FileNotFoundException is thrown

You actually do get a FileNotFoundException

To expand on this: dotnet/dotnet-api-docs#336

Lastly, maybe we could clean it up so we're not passing in a boolean to make the code slightly more expressive? Thoughts? Other than that, this looks really good.

jpreese · 2018-07-10T17:16:07Z

dotnet/docs#6394

rkoeninger · 2018-07-10T19:08:14Z

I think this Fixes #156.

jpreese · 2018-07-10T19:23:13Z

@rkoeninger it does, yes.

jpreese · 2018-07-12T16:36:19Z

Still waiting on a final response from dotnet/dotnet-api-docs#336, but it seems like it can be either or.

jpreese · 2018-07-12T20:59:12Z

It seems like the exception that is called may not ultimately matter dotnet/docs#6431 (comment)

jpreese · 2018-07-14T15:18:21Z

@robkeim I think if we just resolve the merge conflicts, we should be good. The type of exception that is thrown seems to be all over the place, and may even be dependent on OS. So it's probably not a huge deal, at least now. The library has always thrown a FileNotFoundException, so there's no real compelling reason to change it until it pops up, if ever.

The only potential refactor I'd like to see is the passing in of a boolean, but even that is a small nit and something we can revisit later if you'd rather just merge this in so we can get the functionality live.

robkeim · 2018-07-15T03:16:29Z

Sorry for not responding sooner @jpreese it's been a busy week here, but I appreciate you digging into this for me. Let me take another look at the boolean and see what I can do. I'll also resolve the merge conflicts.

…ates # Conflicts: # TestingHelpers/MockDirectoryInfo.cs # TestingHelpers/MockFileData.cs

robkeim · 2018-07-15T03:31:02Z

@jpreese can you this PR another look when you get a chance? I removed the boolean argument and split it into two functions that I hope have a more clear naming.

jpreese · 2018-07-15T04:04:13Z

Yep. This looks good to me now. Thanks! 🎉

Rob Keim and others added 2 commits July 9, 2018 21:51

Revive PR TestableIO#157

24a838f

Merge branch 'master' into mockDirectoryInfo-dates

b361ebd

robkeim mentioned this pull request Jul 9, 2018

Implement Creation, LastAccess and LastWrite dates on MockDirectoryInfo #157

Closed

Merge branch 'master' into mockDirectoryInfo-dates

3107564

jpreese requested changes Jul 9, 2018

View reviewed changes

jpreese mentioned this pull request Jul 9, 2018

Implementing MockDirectoryInfo.CreationTime #316

Closed

robkeim mentioned this pull request Jul 10, 2018

Misc code cleanup #313

Merged

Rob Keim and others added 2 commits July 10, 2018 23:13

Integrate PR feedback

73e8815

Merge branch 'master' into mockDirectoryInfo-dates

1f52f4e

jpreese mentioned this pull request Jul 10, 2018

MockDirectoryInfo implement LastWriteTimeUtc #156

Closed

jpreese changed the title ~~Revive PR #157~~ Implement Creation, LastAccess and LastWrite Dates on MockDirectoryInfo Jul 12, 2018

Rob Keim added 2 commits July 15, 2018 10:20

Merge remote-tracking branch 'origin/master' into mockDirectoryInfo-d…

d63cbaf

…ates # Conflicts: # TestingHelpers/MockDirectoryInfo.cs # TestingHelpers/MockFileData.cs

Remove ambiguous boolean argument from GetMockFileData

482109c

jpreese merged commit eb1e20a into TestableIO:master Jul 15, 2018

robkeim deleted the mockDirectoryInfo-dates branch July 15, 2018 04:05

Implement Creation, LastAccess and LastWrite Dates on MockDirectoryInfo #311

Implement Creation, LastAccess and LastWrite Dates on MockDirectoryInfo #311

Uh oh!

Conversation

robkeim commented Jul 9, 2018

Uh oh!

fgreinacher commented Jul 9, 2018

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!

jpreese commented Jul 9, 2018

Uh oh!

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

Uh oh!

Uh oh!

jpreese commented Jul 10, 2018

Uh oh!

rkoeninger commented Jul 10, 2018

Uh oh!

jpreese commented Jul 10, 2018

Uh oh!

jpreese commented Jul 12, 2018

Uh oh!

jpreese commented Jul 12, 2018

Uh oh!

jpreese commented Jul 14, 2018

Uh oh!

robkeim commented Jul 15, 2018

Uh oh!

robkeim commented Jul 15, 2018

Uh oh!

jpreese commented Jul 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jpreese commented Jul 10, 2018 •

edited

Loading