Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File system should be case-sensitive on Unix #321

Closed
rkoeninger opened this issue Jul 12, 2018 · 6 comments
Closed

File system should be case-sensitive on Unix #321

rkoeninger opened this issue Jul 12, 2018 · 6 comments
Labels
state: in work Issues that are currently worked on type: bug Issues that describe misbehaving functionality

Comments

@rkoeninger
Copy link
Contributor

The MockFileSystem uses a case-insensitive dictionary, but only on Windows is the file system case insensitive. MockFileSystem won't exhibit correct case-sensitivity behavior on Unix.

Relevant line in current HEAD

@fgreinacher fgreinacher added type: bug Issues that describe misbehaving functionality state: ready to pick Issues that are ready for being worked on labels Jul 12, 2018
@fgreinacher
Copy link
Contributor

fgreinacher commented Jul 12, 2018

Good catch - thanks! We have to consider back compat here though whenever someone starts working on it.

@robkeim
Copy link
Contributor

robkeim commented Jul 15, 2018

@fgreinacher how do we typically handle backwards compat for "breaking" changes like this?

This looks like an easy one that I could try to pick up, I just need some guidance on handling backwards compat.

@fgreinacher
Copy link
Contributor

Hm, not sure about how it was handled up to now, but what do you think about something like this:

New types (names to be discussed):

public enum CasingBehavior 
{
   IgnoreCase,
   InheritFromOperatingSystem
}

// Immutable class, extensible for adding new options in the future
public sealed class MockFileSystemOptions
{
    // The default instance would have an empty current directory
    // and CasingBehavior.IgnoreCase
    public static MockFileSystemOptions Default { get; };

    public MockFileSystemOptions WithCurrentDirectory(string currentDirectory);
    public MockFileSystemOptions WithCasingBehavior(CasingBehavior casingBehavior);
}

New constructor setup for MockFileSystem:

public MockFileSystem()
   : this(null, MockFileSystemOptions.Default)
{
}

public MockFileSystem(IDictionary<string, MockFileData> files)
   : this(files, MockFileSystemOptions.Default)
{
}

public MockFileSystem(IDictionary<string, MockFileData> files, string currentDirectory)
   : this(files, MockFileSystemOptions.Default.WithCurrentDirectory(currentDirectory))
{
}

// New primary constructor
public MockFileSystem(IDictionary<string, MockFileData> files, MockFileSystemOptions options)
{
}

Sample usage:

var mockFileSystem = new MockFileSystem(
   files,
   MockFileSystemOptions.Default.WithCasingBehavior(CasingBehavior.InheritFromOperationSystem)
);

@rkoeninger
Copy link
Contributor Author

rkoeninger commented Jul 16, 2018

MockFileSystem.CurrentPlatform(files?, currentDirectory?) would be a nice convenience. Even just in the test suite for this library.

Although, the thought did cross my mind earlier that we could have both MockWindowsFileSystem and MockUnixFileSystem as two different implementations so one could test their code against both Windows and Unix behavior on the same machine. Would be a lot of work. Couldn't depend on System.IO.Path like it does now. Not suggesting as a part of this issue.

@fgreinacher
Copy link
Contributor

In a world where it’s super simple to run tests in multiple environments I think it’s not necessary to invest a lot of time trying to mirror platform behaviors in a lib like this.

Regarding the convenience method: I am a big fan of immutability so I’d rather not like to have a method that changes an objects behavior drastically after it has been constructed. If you need a different file system just construct a new one.

@rkoeninger
Copy link
Contributor Author

rkoeninger commented Jul 17, 2018

MockFileSystem.CurrentPlatform was intended to be a static method, as it would pick one of two concrete classes to return.

Ignoring any out there ideas like mirroring platforms, this starts as a one-line change. The MockFileSystem constructor would just pick whether to use a case sensitive or insensitive comparator when it makes it's dictionary. There would be more changes necessary like here and I don't know where else. I guess do a search for "OrdinalIgnoreCase".

This would be more similar to current behavior where Unix/Windows behavior is not controllable and just reacts to what platform you're running on.

Case sensitivity/insensitivity tests can be added with WindowsOnly/UnixOnly.

@fgreinacher fgreinacher added state: in work Issues that are currently worked on and removed state: ready to pick Issues that are ready for being worked on labels Dec 6, 2018
fgreinacher pushed a commit that referenced this issue Dec 7, 2018
Fixes #321
Fixes #395
Fixes #405

* Added StringComparer to MockFileSystem, used in Mock* classes

* Removed drive letter ToUpper in MockDriveInfo

* Labelled tests as being Windows/Unix specific based on case sensitivity

* Corrected UnixOnly test to check for null

* Adjusted *Specifics wording

* Fixed MockDirectory.GetParent to correctly identify and return '/' root

* Fixed MockDirectory.GetFiles to handle un-normalized slashes

* Made NormalizeSlashes handle UNC paths

* Moved Exists check in MockDirectory.GetFiles into GetFilesInternal after path normalization

* Made MockDirectory.GetLogicalDrives return drive letters in upper case instead of lower case

* Fixed GetLogicalDrives test

* De-triplicated check for Directory.Exists(DirName(FullPath(p)))

* Added StringOperations, used throughout Mock* classes

Cleaned up MockFileExistsTests, MockFileSystemTests

* Removed WindowsSpecifics.EmptyInvalidPathChars

Removed WindowsOnly label from the one that that used this reason

* Made MockDirectory_GetParent_ShouldThr... WindowsOnly again

because of Windows' stricter path rules

* Combined private CheckDirectoryExists and VerifyDir... in MockFile

* Cleanup, review change, in MockFileExistsTests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: in work Issues that are currently worked on type: bug Issues that describe misbehaving functionality
Projects
None yet
Development

No branches or pull requests

3 participants