Added TestingHelper for directory SetAttributes by kmcginnes · Pull Request #138 · TestableIO/System.IO.Abstractions

kmcginnes · 2015-09-22T19:11:06Z

I've implemented the logic for SetAttributes for directories. The code for files was already in place. And someone else had already provided the code to get attributes from directory paths. I just borrowed and adapted that code.

manne · 2015-12-29T11:34:02Z

@kmcginnes the implementation looks good. Can you add the parameter checks (et al) based on the documentation and the actual implementation.
it would be awesome, if you could also adapt the parameter check implementation of MockFile.GetAttributes.

kmcginnes · 2015-12-29T18:40:57Z

I updated the code to the latest and fixed conflicts. I also added some tests that prove out some of the parameter preconditions. Let me know what else you want.

manne · 2015-12-29T21:15:42Z

The name of the method does not match the thrown exception.
see other comment below

Good catch. Copy pasta...

manne · 2015-12-29T21:33:33Z

there is a useful vs extension to recognize the trailing whitespaces

it really helps a lot. I deactivated removing the trailing whitespaces on saving.

kmcginnes · 2015-12-30T18:58:54Z

I've made the changes requested. I would also like to move the SetAttributes tests into their own file. Although, that might make more sense as a separate pull request. What do you think?

jpreese · 2018-07-07T04:56:43Z

Thanks for this PR @kmcginnes. We're making progress on getting through the open PRs and issues that have been sitting around for awhile.. (ok a LONG while). This one looks like it's in pretty good shape, and you still seem pretty active. If you have interest in resolving the merge conflicts, we can re-run the checks and get this merged in for you.

kmcginnes · 2018-07-07T16:39:09Z

I'll take a look. I'll have to reacquaint myself with these changes.

@jpreese I think it would be a good idea if I squash these commits down to one. I don't see a reason to keep the multiple commits around.

jpreese · 2018-07-07T16:44:43Z

I agree. When it's time to merge into master, I'll be squashing them.

kmcginnes · 2018-07-07T16:50:17Z

So, I take it you want me to merge master, not rebase master.

jpreese · 2018-07-07T17:00:07Z

For the PR I'm indifferent. I was just going to squash before approving.

kmcginnes · 2018-07-07T18:17:30Z

A couple of questions:

Should I create a new file for the SetAttribute tests called MockFileSetAttributesTests.cs and move those tests out of MockFileTests.cs?
Should I remove the PathTooLongExceptoin test? It's failing and I don't see any others in the code base. Maybe because Microsoft fixed it at some point?

[Test]
public void MockFile_SetAttributeOfExistingDirectoryWithPathTooLong_ShouldThrowPathTooLongException()
{
    var path = XFS.Path("C:\\8e95d05bc8d747d194e59516f8d1493b\\aa3a4391679642f9bf83f9501f7fb0e1\\8c51cab5b63543909fb7da59a3932b47\\49910ca19d0a4b59aadd4276829cc476\\3b0540cbd57b4770a52166c0ffd72e10\\70ab25de331e473a9cffbc5e297c79c1\\607c63a6f57645aa98073e9df7c7edd8\\dc473e93228c44c08d127c36d9cd9b01");
    var attributes = FileAttributes.Normal;
    var fileSystem = new MockFileSystem();

    TestDelegate action = () => fileSystem.File.SetAttributes(path, attributes);

    Assert.Throws<PathTooLongException>(action);
}

jpreese · 2018-07-07T19:28:26Z

+        [Test]
+        public void MockFile_SetAttributeOfExistingDirectoryWithPathTooLong_ShouldThrowPathTooLongException()
+        {
+            var path = XFS.Path("C:\\8e95d05bc8d747d194e59516f8d1493b\\aa3a4391679642f9bf83f9501f7fb0e1\\8c51cab5b63543909fb7da59a3932b47\\49910ca19d0a4b59aadd4276829cc476\\3b0540cbd57b4770a52166c0ffd72e10\\70ab25de331e473a9cffbc5e297c79c1\\607c63a6f57645aa98073e9df7c7edd8\\dc473e93228c44c08d127c36d9cd9b01");


Instead of this approach, I think calling something like string.PadRight() would be a little nicer.

jpreese · 2018-07-07T19:29:59Z

+                }
+                else
+                {
+                    throw new FileNotFoundException($"Could not find file '{path}'.", path);


I would take this opportunity to move this string into the Resources.resx file. Just make sure it's the same verbiage that System.IO uses, which it already looks like it is.

As a followup, I'm about to deploy this resource verbiage as part of another PR.

Ok, you should be able to use StringResources.Manager.GetString("COULD_NOT_FIND_FILE_EXCEPTION")

I see what I'm supposed to be able to do, but the compiler keeps telling me:

'StringResources' is inaccessible due to its protection level

And I do see that you have this defined in the AssemblyInfo.cs:

using System.Runtime.CompilerServices; #if DEBUG [assembly: InternalsVisibleTo("System.IO.Abstractions.TestingHelpers.Tests")] #else [assembly: InternalsVisibleTo("System.IO.Abstractions.TestingHelpers.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010051bf2aa00ba30d507d4cebcab1751dfa13768a6f5235ce52da572260e33a11f52b87707f858fe4bbe32cd51830a8dd73245f688902707fa797c07205ff9b5212f93760d52f6d13022a286ff7daa13a0cd9eb958e888fcd7d9ed1f7cf76b19a5391835a7b633418a5f584d10925d76810f782f6b814cc34a2326b438abdc3b5bd")] #endif

So, in theory, it should work. I even tried shortening the assembly name to be TestingHelpers.Tests, which I believe is actually what it should be, but that didn't seem to help.

It looks like I'm the first consumer of StringResources in the test project. That's why I'm the first to see this.

😭

I'm not sure I follow. You're able to use it in the TestingHelpers library right? For MockInfo.cs? I wouldn't bother using it as an Assertion within the test. Just checking that it throws a FileNotFoundException is more than enough.

Oh, I totally misread your first comment, and what file it was on.

I'll make that change now.

jpreese · 2018-07-07T19:33:13Z

+        [Test]
+        public void MockFile_SetAttributeOfMissingDirectory_ShouldThrowFileNotFoundException()
+        {
+            var path = XFS.Path("C:\\Some\\Path\\That\\Does\\Not\\Exist");


While I like the intent that it shows, we can probably dramatically reduce the length of this test data to simply C:\ or something along those lines. Since we never add anything to the MockFileSystem, any path is going to fail since no paths exist.

It also reduces the chance of the test failing because it depends on two artfully crafted strings (one in the arrange, and one in the assert)

jpreese · 2018-07-07T19:37:09Z

Yes. I do like that approach better if you want to split it out.
It -should- pass, but unfortunately it's a known bug in MockFileInfo. You can see the issue here MockFileInfo doesn't throw an exception when created with a too-long path (260+ characters) #192

kmcginnes · 2018-07-07T21:32:41Z

What do you think I should do about the PathTooLongException?

Comment out the test
Comment out the assertion
Mark it inconclusive
Delete the test
Add the workaround from Issue 192 mockfileinfo wil throw exceptions for too long paths #193 to the SetAttributes() method
Something else

jpreese · 2018-07-07T21:57:48Z

My gut says 4. You're not introducing the logic that checks the length. That's probably going to be the job of GetFullPath (or maybe even elsewhere, definitely not SetAttributes) sometime in the future.

I think the check for a long path should be tied with the class/method that's going to do the check. Yeah?

kmcginnes · 2018-07-07T23:52:42Z

Ok, so I've updated with the following:

Updated with latest from master
Use StringResources for the exception message, modeling it after some other thrown exceptions I see in the file
Removed the test for long paths, as I agree that it's not SetAttributes responsibility
Removed a test scenario for an invalid character that doesn't align with similar tests. So now it only tests for the whitespace scenarios
Updated the names of the tests to better match the test naming convention in the project.
Squashed all commits

jpreese · 2018-07-07T23:55:35Z

+    [TestFixture]
+    public class MockFileSetAttributesTests
+    {
+


Itty bitty whitespace

jpreese · 2018-07-07T23:56:25Z

+
+namespace System.IO.Abstractions.TestingHelpers.Tests
+{
+    using XFS = MockUnixSupport;


I'd prefer if this was moved up with the other usings, for consistency.

I prefer the same. I only put it here for consistency with other test files:

https://github.com/System-IO-Abstractions/System.IO.Abstractions/blob/0caa9382a68061a62c69ed3ef8f6f8204204ea7e/TestHelpers.Tests/MockDirectoryTests.cs#L8

jpreese · 2018-07-08T00:04:02Z

+            fileSystem.AddDirectory(path);
+            var existingAttributes = fileSystem.File.GetAttributes(path);
+
+            fileSystem.File.SetAttributes(path, existingAttributes | FileAttributes.Hidden);


The file case makes perfect sense to me. It starts out Normal, and we set the attribute to Hidden. Thus the assertion should be to make sure that the net result is that the file is Hidden (replaced the attribute).

I'm not quite certain why the directory test is pulling the existing attributes, and the File test did not?

The explanation is that I wrote the directory ones, and the file one was simply moved from the MockFileTests.cs file and into MockFileSetAttributesTests.cs to live along side my new tests.

But I think I agree with you. The file and directory tests should be consistent. And the MSDN article on SetAttributes() shows you should always add or remove from the existing set of attributes:

https://msdn.microsoft.com/en-us/library/system.io.file.setattributes(v=vs.110).aspx

So my next commit will update the file test with these changes.

Ok. Let me grab your branch real quick and mess around with it in my IDE. I still have a couple concerns about how these tests are laid out, but I could be taking crazy pills.

Ok, just went through the debugger and confirmed. So, I think how you had it first was correct, given functionality, test name, etc.

Right now you're saying "ShouldREPLACEExistingFlags...", but really what we're doing is ADDing more flags. So the end result is a file that is both "Normal" and "Hidden".

I think that SetAttributes() can be used sort of like a pseudo-RemoveAttribute. i.e. if you have a file thats Hidden -- to remove the hidden and make it normal, I'd think SetAttributes(FileAttributes.Normal) would work. And that would be "replacing".

If you want to preserve all/some of your attributes, then it probably makes sense to selectively remove and then add onto. But if you want to remove all but "Normal" or whatever, I don't think you need to -always- get the current attributes.

Hopefully that wasnt too much of a ramble. The TLDR is that I think it's possible we have two test case scenarios here "replace" and "add additional".

I see your point on the file test.

But for directories, they always need to have the FileAttributes.Directory flag. So a full replacement isn't really possible.

How about initializing the directory's attributes to something in addition to the FileAttributes.Directory flag. Then we can replace it with something else.

Like this:

[Test] public void MockFile_SetAttributes_ShouldReplaceExistingFlagsIfExistingDirectory() { var fileSystem = new MockFileSystem(); var path = XFS.Path(@"c:\something"); fileSystem.AddDirectory(path); var directoryInfo = fileSystem.DirectoryInfo.FromDirectoryName(path); directoryInfo.Attributes = FileAttributes.Directory | FileAttributes.Normal; fileSystem.File.SetAttributes(path, FileAttributes.Directory | FileAttributes.Hidden); var attributes = fileSystem.File.GetAttributes(path); Assert.That(attributes, Is.EqualTo(FileAttributes.Directory | FileAttributes.Hidden)); }

Honestly, the more I think about it and familiarize myself with everything. I don't really see anything wrong with what you're tests are testing.

I just think the naming is less than ideal and may be a source of confusion (at least for me). Maybe we could just change them to something along the lines of MockFile_SetAttributes_ShouldSetAttributesOnDirectory and MockFile_SetAttributes_ShouldSetAttributesOnFile. Simple and to the point. No worry about what does "replace" mean or are we "adding" more attributes.

I would also drop the..

Attributes = FileAttributes.Normal

initialization on the File test since that should be set by System.IO.Abstractions for you.

What do you think about that?

Sorry, I'll hopefully have a chance to take a look at this again tomorrow.

Life, amiright 😞

No worries, we've made you wait years, I'm sure we can wait a little bit. No sweat! 👍

I've made the two changes you suggested, but also made the attributes more explicit. I'm not sure if I like it better or not.

@jpreese I'll hold off on cleaning up the commits and rebasing on master until you've had a chance to check out these changes. If you're not a fan of making the attributes more explicit in the tests, it's an easy commit to just delete.

jpreese · 2018-07-08T00:06:02Z

Couple small nits and just a general question, otherwise looks great!

fgreinacher · 2018-07-18T17:23:10Z

I’ll merge as soon as the conflicts are resolved!

kmcginnes · 2018-07-18T18:14:07Z

@fgreinacher Don't merge until I have a chance to fixup some of these commits. I'm keeping all the commits separate during review, but will cleanup once I get a 👍

fgreinacher

One minor thing from my side - otherwise LGTM (I like the shorter test names 👍)

fgreinacher · 2018-07-18T18:45:37Z

        {
+            if (path == null)
+            {
+                throw new ArgumentNullException("path", StringResources.Manager.GetString("VALUE_CANNOT_BE_NULL"));


Nit: nameof(path)

Good catch!

fgreinacher · 2018-07-20T21:11:05Z

@kmcginnes Could you update this branch one last time? I’d then squash-merge, so no worries about cleaning up the history :)

This was supported for files before, but directories behave a little differenlty. This should better match .Net System.IO behavior.

kmcginnes · 2018-07-21T16:14:57Z

The branch has been updated from upstream, and I've squashed all my commits.

We should be good to go 🚀

fgreinacher · 2018-07-21T16:30:49Z

Thanks again, merging!

kmcginnes force-pushed the support_directory_attributes branch from f57bfab to a55cbc1 Compare December 29, 2015 18:39

manne reviewed Dec 29, 2015
View reviewed changes

jpreese added the state: waiting for feedback Issues that are waiting for feedback label Jul 7, 2018

kmcginnes force-pushed the support_directory_attributes branch 4 times, most recently from 5df28a5 to c01c1a5 Compare July 7, 2018 18:00

jpreese reviewed Jul 7, 2018

View reviewed changes

kmcginnes force-pushed the support_directory_attributes branch 2 times, most recently from 501e30a to 727ac11 Compare July 7, 2018 23:45

kmcginnes force-pushed the support_directory_attributes branch from 727ac11 to c13ff38 Compare July 7, 2018 23:57

jpreese reviewed Jul 8, 2018

View reviewed changes

fgreinacher suggested changes Jul 18, 2018

View reviewed changes

fgreinacher approved these changes Jul 20, 2018

View reviewed changes

Added support for SetAttributes on directories

86c8d2c

This was supported for files before, but directories behave a little differenlty. This should better match .Net System.IO behavior.

kmcginnes force-pushed the support_directory_attributes branch from 9b2e621 to 86c8d2c Compare July 21, 2018 16:00

fgreinacher merged commit 2a3f061 into TestableIO:master Jul 21, 2018

Conversation

kmcginnes commented Sep 22, 2015

Uh oh!

manne commented Dec 29, 2015

Uh oh!

kmcginnes commented Dec 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manne commented Dec 29, 2015

Uh oh!

kmcginnes commented Dec 30, 2015

Uh oh!

jpreese commented Jul 7, 2018

Uh oh!

kmcginnes commented Jul 7, 2018

Uh oh!

jpreese commented Jul 7, 2018

Uh oh!

kmcginnes commented Jul 7, 2018

Uh oh!

jpreese commented Jul 7, 2018

Uh oh!

kmcginnes commented Jul 7, 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 7, 2018

Uh oh!

kmcginnes commented Jul 7, 2018

Uh oh!

jpreese commented Jul 7, 2018

Uh oh!

kmcginnes commented Jul 7, 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!

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 8, 2018

Uh oh!

fgreinacher commented Jul 18, 2018