Differences in behaviour between real windows FS and Window-style in-memory FS about file.dart HOT 18 OPEN

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

from file.dart.

My understanding from @zanderso's comment #112 (comment) 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).

from file.dart.

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.

from file.dart.

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).

from file.dart.

/cc @zanderso

from file.dart.

@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?

from file.dart.

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)

from file.dart.

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.

from file.dart.

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) :-)

from file.dart.

@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.

from file.dart.

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

#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.

from file.dart.

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

from file.dart.

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

from file.dart.

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?

from file.dart.

Yep, that sounds like a good solution.

Thanks!

from file.dart.

Friendly ping~

Fix available in PR #121

from file.dart.

Should this issue be closed?

from file.dart.

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

from file.dart.