Differences in behaviour between real windows FS and Window-style in-memory FS

[DanTup]

I was moving some tests in Flutter over to the memory file system and hit some issues because of differences in behaviour. In the real implementation, if you use a forward slash in a path, it still resolves correctly. However in the memory file system it treats it as a different path.

The below (when run on Windows) prints:

true
false

import 'package:file/local.dart';
import 'package:file/memory.dart';

final localFs = new LocalFileSystem();
final memoryFs = new MemoryFileSystem(style: FileSystemStyle.windows);

main() {
  [
    new LocalFileSystem(),
    new MemoryFileSystem(style: FileSystemStyle.windows),
  ].forEach((fs) {
    // Create a file at test\test.txt
    fs.currentDirectory = fs.systemTempDirectory;
    fs.directory('test').createSync(recursive: true);
    fs.file('test\\test.txt').createSync();

    // Check if it exists using a forward slash
    print(fs.file('test/test.txt').existsSync());
  });
}

I'll fix this in flutter by using the correct slashes, but I guess the intention is for these to behave the same where possible. It's possibly that https://github.com/google/file.dart/issues/56 would cover this, I'm not sure.

[goderbauer]

On Windows it depends on which underlaying Windows API is used. Some can convert slashes, others don't. The only safe way is to use the correct slash on Windows everywhere and I think the memory file system should enforce that.

[DanTup]

In that case, I wonder if Dart should be more aggressive (not just the memory provider). It seems weird if Dart's expected behaviour depends on whichever Windows API its implementation happens to use at a point in time (eg., internal refactors to use different APIs in Windows probably shouldn't have observable effects?).

But at least, having the memory provider enforce slashes would be a great change. If you move from the real FS to the Memory FS (as I just did) it would be better to get clear exceptions that are obvious to fix rather than subtle differences in behaviour (in this case, some code just claimed files didn't exist which had just been created - albeit with different path separators).

[tvolkert]

/cc @zanderso

[zanderso]

@bkonyi I won't have access to a Windows machine until next week, would you mind just trying to reproduce this using dart:io File directly?

[DanTup]

Sure! I wasn't sure how much LocalFileSystem did and assumed it was the same. In this case, the behaviour does match anyway:

int('native');
// Create a file at test\test.txt
Directory.current = Directory.systemTemp;
Directory('test').createSync(recursive: true);
File('test\\test.txt').createSync();

// Check if it exists using a forward slash
print(File('test/test.txt').existsSync());
print('');

[
  new LocalFileSystem(),
  new MemoryFileSystem(style: FileSystemStyle.windows),
].forEach((fs) {
  print(fs.runtimeType);
  // Create a file at test\test.txt
  fs.currentDirectory = fs.systemTempDirectory;
  fs.directory('test').createSync(recursive: true);
  fs.file('test\\test.txt').createSync();

  // Check if it exists using a forward slash
  print(fs.file('test/test.txt').existsSync());
  print('');
});

native
true

LocalFileSystem
true

_MemoryFileSystem
false

(Running on Windows 10 Pro 10.0.17134 Build 17134)

[zanderso]

Oh, sorry, I misunderstood the issue. I had thought that the dart:io File was failing to handle both slash directions. We have lots of tests and code written in the SDK that rely on both directions working, so I think we're probably going to continue doing that.

[DanTup]

That sounds good to me. My request is really that switching from real file system methods (or LocalFileSystem) to MemoryFileSystem doesn't introduce differences in behaviour (especially if they're subtle, silent and potentially only on one platform) :-)

[tvolkert]

@DanTup would you be willing to write some tests in common_tests.dart that show this failure? We can skip them and then enable them when we fix the bug.

[DanTup]

I've opened a PR with some tests that pass if I use the same slash direction, but fail as-committed:

https://github.com/google/file.dart/pull/115

I couldn't see an obvious way to skip them though.

Also, FWIW, I get 4 existing failures running on Windows, for example:

00:07 +316 -1: test\local_test.dart: LocalFileSystem common File length succeedsIfExistsAsLinkToFile [E]
  FileSystemException: Cannot retrieve length of file, path = 'C:\Users\danny\AppData\Local\Temp\file_test_319fad9a-b0fa-11e8-ac4e-001a7dda7113\bar' (OS Error: The directory name is invalid.

Let me know if I should open an issue with details.

[ghost]

I just stumbled upon these tests failing internally.

@DanTup, you mentioned not seeing any obvious way to skip the tests. But as someone knowing nothing about this, I see runCommonTests(..) has a skip argument, and the test package has some skipping functionality[0] - would this help here?

I'd like if we could somehow disable these test if they're known failing so our internal Dart test targets can cycle green.

[0] https://pub.dartlang.org/packages/test#skipping-tests

[tvolkert]

@cskau-g can you skip them in the BUILD rule?

[ghost]

The tests are in common_tests.dart which isn't a test by itself, instead it's imported into chroot_test.dart, local_test.dart and memory_test.dart. I can exclude those three files from the test suite, but then we're left with three tiny tests in utils_test.dart.

I had a quick crack at it and it seems patching the test(..) function in common_tests.dart to have a {dynamic skip} argument which is passed through to the two testpkg.test(.., skip: skip) works.

Would that be a workable solution?

[tvolkert]

Yep, that sounds like a good solution.

Thanks!

[ghost]

Friendly ping~

Fix available in PR https://github.com/google/file.dart/pull/121

[Hixie]

Should this issue be closed?

[dnfield]

The linked PR didn't fix the issue, it skipped the tests.

[Hixie]

It's not clear to me that there is agreement that there actually is an issue.

[DanTup]

My understanding from @zanderso's comment https://github.com/google/file.dart/issues/112#issuecomment-417269305 is that both slash directions should work using this, as they do in the real implementations. Having subtle differences in behaviour between your tests and what your users would actually see makes the tests less useful (in this case I had failing tests that should pass, but it wouldn't be hard to end up with the opposite).