Teach Enlistment.GetNewGVFSLogFileName to take FileSystem parameter#1071
Teach Enlistment.GetNewGVFSLogFileName to take FileSystem parameter#1071jamill merged 1 commit intomicrosoft:masterfrom
Conversation
c6b5d84 to
2ede3dc
Compare
kewillford
left a comment
There was a problem hiding this comment.
Not sure if this helps us with the long term plans with file system.
GVFS/GVFS.Common/Enlistment.cs
Outdated
| PhysicalFileSystem fileSystem, | ||
| string logsRoot, | ||
| string prefix, | ||
| string logId = null) |
There was a problem hiding this comment.
I don't like how this caused a bunch of new PhysicalFileSystem() calls to be added to that callers especially with what we were talking about yesterday with working towards having only one on the context or platform. I haven't look at all the call sites but could we instead change this to not be a static call and pass the file system into the constructor of the Enlistment? Not sure if all the call sites have an enlistment to use though. But that would eventually allow us to mock the GetNewLogFileName if needed. Statics make it really hard for unit testing and mocking.
There was a problem hiding this comment.
I haven't look at all the call sites but could we instead change this to not be a static call and pass the file system into the constructor of the Enlistment?
Not all of the call sites have a GVFSEnlistment instance :) For example, the upgrader is not run in the context of an enlistment, and neither is the service callsites - which maybe hints that the method is not in the most appropriate location.
Statics make it really hard for unit testing and mocking.
Yep - the motiviation for this change is around making the callers of this method more testable :)
kewillford
left a comment
There was a problem hiding this comment.
One suggestion. Using new PhysicalFileSystem() in the GetNewLogFileName method will make this change much smaller.
This will enable the Enlistment.GetNewGVFSLogFileName method to interact with the file system through the file system abstraction layer, enabling us to unit test this method and methods that call this method. Instead of updating all call sites to pass in a physical file system, this is an optional argument. If one is passed in, it will be used. If no physical file system argument is provided, a default physical file system object will be created.
2ede3dc to
e75f659
Compare
I like the idea of reducing the impact on call sites. I tweaked the method to optionally take in a |
mjcheetham
left a comment
There was a problem hiding this comment.
Looks much cleaner/less churn with the new update (fileSystem ?? new PhysicalFS())
👍
This will enable the Enlistment.GetNewGVFSLogFileName method to interact with
the file system through the file system abstraction layer, enabling us to unit
test this method and methods that call this method.
With this change, the GitHubUpgrader tests should no longer create actual directories when running the unit tests.