Add FileStreamFactory and support for mocking FileStreams by augustoproiete · Pull Request #177 · TestableIO/System.IO.Abstractions

augustoproiete · 2016-10-29T19:10:59Z

Add IFileStreamFactory, FileStreamFactory, and MockFileStreamFactory to allow creating instances of FileStream with more advanced options not exposed by FileBase and FileInfo, such as bufferSize, FileOptions, FileSecurity, and FileSystemRights, as well as opening FileStream from a SafeFileHandle instead of a file path, etc.

I've included all public constructors/overloads of the FileStream in IFileStreamFactory and consequently in FileStreamFactory

jpreese · 2018-07-09T04:44:36Z

@caioproiete thanks for the PR! At first glance, this looks like it's in pretty decent shape, but does have some merge conflicts.

fgreinacher · 2018-07-10T21:19:26Z

@jpreese I resolved the merge conflicts and updated it to handle the changes to MockFileStream.

For me it looks good now - and fills an major functionality gap.

augustoproiete · 2018-07-10T22:18:04Z

@jpreese You're welcome! You know... The PR had zero merge conflicts 2 years ago when I created it 🙃 haha anyway thanks for reviewing it

@fgreinacher Awesome! Thanks for helping to move this one forward!

jpreese · 2018-07-10T22:22:41Z

@caioproiete you're alive! Again, truly sorry for the delay in all of this. We're working on getting some momentum on this project again. Your PR is very appreciated.

augustoproiete · 2018-07-11T16:57:39Z

@jpreese 😄 No worries! Great to see some traction on this project again! It's a great library!

jpreese · 2018-07-11T17:37:46Z

        FileBase File { get; }
        DirectoryBase Directory { get; }
        IFileInfoFactory FileInfo { get; }
+        IFileStreamFactory FileStream { get; }


This is my only concern. Since we're adding onto IFileSystem, it's a breaking change.. but is anyone really writing their own classes using IFileSystem?

Yeah, same goes for 07227bc... I also don't think this will be a big problem for people. Whenever we are done with the old issues/PRs we should again think about release strategy, versioning, issue tagging, etc...

To be clear. I would merge this one and as a follow-up take care about our future strategy.

Yeah, I would agree. It's probably an incredibly small subset of users it would impact.

jpreese · 2018-07-12T01:07:29Z

+        Stream Create(string path, FileMode mode, FileSystemRights rights, FileShare share, int bufferSize, FileOptions options);
+#endif
+
+#if NET40 || NETSTANDARD_20


Do we need the NET40/NETSTANDARD check here? I don't see a mirroring check in the MockFileStreamFactory

Yeah, because the “real” implementation doesn’t support these overloads on NS1.4.

jpreese · 2018-07-12T01:07:53Z

+            return new MockFileStream(mockFileSystem, path, GetStreamType(mode, access));
+        }
+
+        public Stream Create(string path, FileMode mode, FileSystemRights rights, FileShare share, int bufferSize, FileOptions options, FileSecurity fileSecurity)


The interface has a preprocessor check, but this is exposed

Oops, will add a check here.

jpreese · 2018-07-12T01:14:37Z

Have a couple eye raises around the NET40/NETSTANDARD stuff, seems to be some inconcistencies.

…System.IO.Abstractions into add-file-stream-factory-etc

fgreinacher · 2018-07-13T22:16:08Z

OK, feedback is addressed. I also mixed in some minor cleanups :)

jpreese · 2018-07-13T23:27:35Z

+        [Test]
+        [TestCase(FileMode.Create)]
+        [TestCase(FileMode.Append)]
+        public void MockFileStreamFactory_Create_string_FileMode__ShouldReturnStreamForNonExistingFile(FileMode fileMode)


This test name could use some love. MockFileStreamFactory_Create_ShouldReturnStreamForNonExistingFile()?

Quick tangent... I'm not a huge fan of the idea that we include the name of the class in the name of the test. In a greenfield scenario, I'd name this test Create_FileDoesNotExist_ReturnsStream -- but I think the suggestion follows more suite with how our tests are currently named.

jpreese · 2018-07-13T23:27:55Z

+        [Test]
+        [TestCase(FileMode.Create)]
+        [TestCase(FileMode.Append)]
+        public void MockFileStreamFactory_Create_string_FileMode__ShouldReturnStreamForExistingFile(FileMode fileMode)


More test name love.

jpreese · 2018-07-13T23:28:18Z

+    [Serializable]
+    internal sealed class FileStreamFactory : IFileStreamFactory
+    {
+        public Stream Create(string path, FileMode mode)


Not a nit, but thought this was interesting.
https://ericlippert.com/2014/09/15/internal-or-public/

👍 That's a good read, thanks! I also prefer public in these cases, but I could never provide non-subjective reasons for that

jpreese

Looks fine to me overall, just a small issue with the test naming.

fgreinacher · 2018-07-14T21:50:13Z

a6dc64d addresses the feedback and simplifies the test cases a bit more.

fgreinacher · 2018-07-17T21:31:14Z

Merging this - if there is additional feedback on the changes just add it here and I’ll address it in a follow-up.

Done

Add FileStreamFactory and support for mocking FileStreams

f6f1d1a

manne mentioned this pull request Dec 3, 2016

Add events on major events #168

Closed

THammond9 mentioned this pull request May 11, 2017

Is FileStream covered? #224

Closed

jpreese added the type: enhancement Issues that propose new functionality label Jul 9, 2018

fgreinacher and others added 3 commits July 10, 2018 21:45

Merge branch 'master' into add-file-stream-factory-etc

9444c03

Fix .NET Standard compile

3d50fcb

Adapt MockFileStreamFactory to MockFileStream changes

46ba25d

jpreese reviewed Jul 11, 2018

View reviewed changes

Merge branch 'master' into add-file-stream-factory-etc

0e3d7ec

jpreese reviewed Jul 12, 2018

View reviewed changes

fgreinacher and others added 5 commits July 13, 2018 23:30

Add missing ifdefs for MockFileStreamFactory

520db27

Use expression bodies

84574a8

Merge branch 'master' into add-file-stream-factory-etc

9fb859d

Delete unnecessary nuspec file

2ed93bc

Merge branch 'add-file-stream-factory-etc' of github.com:caioproiete/…

371288b

…System.IO.Abstractions into add-file-stream-factory-etc

jpreese reviewed Jul 13, 2018

View reviewed changes

jpreese previously requested changes Jul 13, 2018

View reviewed changes

Simplify MockFileStreamFactory tests

a6dc64d

Merge branch 'master' into add-file-stream-factory-etc

d925c8c

fgreinacher merged commit 3fc0b03 into TestableIO:master Jul 17, 2018

augustoproiete deleted the add-file-stream-factory-etc branch July 18, 2018 17:34

Conversation

augustoproiete commented Oct 29, 2016

Uh oh!

jpreese commented Jul 9, 2018

Uh oh!

fgreinacher commented Jul 10, 2018

Uh oh!

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

augustoproiete commented Jul 11, 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 12, 2018

Uh oh!

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

jpreese left a comment

Choose a reason for hiding this comment

Uh oh!

fgreinacher commented Jul 14, 2018

Uh oh!

fgreinacher commented Jul 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jpreese commented Jul 10, 2018 •

edited

Loading