Skip to content

Conversation

m-wild
Copy link
Contributor

@m-wild m-wild commented Oct 16, 2018

Fixes: #350
Tried to do this with the smallest change possible.
Hard-coding to "C:\" (which will get translated to "/" on unix) as per discussion on the issue.

Let me know if this is okay.

@fgreinacher
Copy link
Contributor

Thanks for your contribution, generally this seems like a safe and valuable change to make.

Could you please verify that a scenario like the one mentioned in #350 (comment) will work with your change?

@m-wild
Copy link
Contributor Author

m-wild commented Oct 18, 2018

I'm not exactly sure how to prove it will fix that.
When i wrote a test like the one in the comment

void Test...() {
    var fs = new MockFileSystem();
    var dirInfo = fs.DirectoryInfo.FromDirectoryName(".");
}

However, no exception was thrown.

I think you would need to have the real System.IO.Path.GetTempPath() throw an exception.

The test MockDirectory_GetCurrentDirectory_ShouldReturnDefaultPathWhenNotSet already tests that calling fileSystem.Directory.GetCurrentDirectory(); succeeds.
We could test that fileSystem.DirectoryInfo.FromDirectoryName("."); also succeeds, but as i mentioned earlier, it was already succeeding! So I'm not sure the test adds any value.

What are your thoughts?

@fgreinacher
Copy link
Contributor

I'd expect the linked snippet to blow up with our current release because the temp directory does not exist in the MockFileSystem. No need to automate the test though, I was just looking for a quick manual validation.

@fgreinacher
Copy link
Contributor

fgreinacher commented Oct 26, 2018

@tehmantra What about adding a test like this:

public void MockFileSystem_DefaultState_CurrentDirectoryExists()
{
   var fs = new MockFileSystem();

   var currentDirectory = fs.DirectoryInfo.FromDirectoryName(".");

   Assert.IsTrue(currentDirectory.Exists);
}

That should ensure that we can use the current directory safely after constructing the MockFileSystem.

@m-wild
Copy link
Contributor Author

m-wild commented Oct 29, 2018

@fgreinacher MockFileSystem will now create the current directory if it doesn't exist.
Added a couple of test cases around this too.

Cheers.

@fgreinacher fgreinacher merged commit dd4a4b8 into TestableIO:master Oct 29, 2018
@fgreinacher
Copy link
Contributor

Thanks for your work @tehmantra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants