Suites are duplicated in the test tree on Windows

[DanTup]

The results from discovery and from running don't seem to have been merged together correctly.

[DanTup]

Seems like the slashes in paths may be the wrong way in the notifications:

{"suite":{"id":0,"platform":"vm","path":"D:/Dev/Dart-Code/src/test/test_projects/hello_world/test/tree_test.dart"},"type":"suite","time":0}

[DanTup]

@jakemac53 what's the expected behaviour here? In the package:test docs just says this is a "path". My interpretation would be that this has system-specific slashes:

  // The path to the suite's file, or `null` if that path is unknown.
  String? path;

I'm not sure if this has changed or I just never noticed it (I'll do some testing with different versions shortly), although I also notice that we get relative or absolute paths based on how we run the file (which may be intended, but surprised me, and I think should be noted in the JSON reporter doc so it's clear to users of this what the expectations are):

PS D:\Dev\Dart-Code\src\test\test_projects\hello_world> dart test -r json .\test\tree_test.dart 
{"suite":{"id":0,"platform":"vm","path":"test/tree_test.dart"},"type":"suite","time":0}



PS D:\Dev\Dart-Code\src\test\test_projects\hello_world> dart test -r json "D:\Dev\Dart-Code\src\test\test_projects\hello_world\test\tree_test.dart"
{"suite":{"id":0,"platform":"vm","path":"D:/Dev/Dart-Code/src/test/test_projects/hello_world/test/tree_test.dart"},"type":"suite","time":0}

[DanTup]

It does appear this changed. Using package:test 1.22.0, I see .\test\tree_test.dart as the path, and with 1.23.1 I see test/tree_test.dart.

[jakemac53]

Yes there was a change in how paths are reported on windows related to the windows fixes that landed recently.

[DanTup]

@jakemac53 intended, or accidental?

I've pushed a fix here that will just naively replace slashes on Windows to fix the issue here, but I think the json reporter docs should be more explicit about what to expect (slash directions, absolute vs relative) since they're likely being consumed by tools.

[jakemac53]

It was a known change, tests had to be updated for it, but it was a byproduct of other changes. So I guess we can call it intended :). It was overlooked that this would also potentially affect uses of the json API though.

However there are still some cases I think where the backslashes do appear - in particular on github this was showing up still, something weird/different about that environment that I couldn't reproduce.

[DanTup]

It was a known change [...]. It was overlooked that this would also potentially affect uses of the json API though.

Does this mean it's here to stay, or the JSON reporter will be updated?

I think it's slightly confusing to use / in something called path on Windows, but it's easy enough to handle. I just think the docs should make it really clear. It's easy for people to develop code on one OS and not realise it's broken on another, so the more explicit things are the better :-)

there are still some cases I think where the backslashes do appear - in particular on github this was showing up still, something weird/different about that environment that I couldn't reproduce.

In the JSON reporter, or somewhere else?

[jakemac53]

Does this mean it's here to stay, or the JSON reporter will be updated?

I do think its here to stay unless we have a really compelling reason to do something else. It is a result of parsing all paths as a URI, we lose the context of the original separator. This allowed us to also remove a fair bit of hacky parsing/trimming of the user input that we had previously.

In the JSON reporter, or somewhere else?

It could surface in the JSON reporter yes. We only have one "path" internally for tests, for all reporters. We get that path by parsing the user provided path as a URI and then asking for the path of that URI. According to the documentation this would always normalize to forward slashes but there was some weird scenario in which it wasn't, which I couldn't track down.

[DanTup]

It is a result of parsing all paths as a URI, we lose the context of the original separator.

You can still get this from a file: URI by using toFilePath() instead of path, although you need to remove any querystring first. I had to do that in flutter test for the line/col support (I needed the full paths of each test, but also needed to support the querystrings):

https://github.com/flutter/flutter/pull/119740/files#diff-328d8a42e42b84152d577569aa043fdfa16977190165f1a11897f6f47d81c1a5R283

It's a little awkward, but if you've already split the querystring off and are just passing the path around, it'd work. This might be more inline with what's expected (and avoid the potentially breaking change - although perhaps that ship has already sailed).

According to the documentation this would always normalize to forward slashes but there was some weird scenario in which it wasn't, which I couldn't track down.

That does seem odd - I've never seen Uri.path return backslashes (only .toFilePath()). Is this still an outstanding issue or have you fixed/worked around it?

[jakemac53]

You can still get this from a file: URI by using toFilePath() instead of path, although you need to remove any querystring first. I had to do that in flutter test for the line/col support (I needed the full paths of each test, but also needed to support the querystrings):

This would be consistent - but still a divergence from what we used to do. The behavior previously would always return the path exactly how it was provided, so regardless of platform it just retained the given style.

I believe it would also always be an absolute path, which we wouldn't want generally, we want to report relative paths to the user as they are a lot easier to understand.

I could see the JSON reporter always reporting the absolute path, but that would also be breaking, and I worry about doing that change at this time.

That does seem odd - I've never seen Uri.path return backslashes (only .toFilePath()). Is this still an outstanding issue or have you fixed/worked around it?

What was actually happening is it was encoding the backslashes in the URI. So for some reason it decided to encode them instead of treat them as separators. The existing logic just decodes the path component of the URI when pulling it back out, which ends up just restoring the backslashes. Very weird though and I could not replicate the behavior at all locally.

[DanTup]

The behavior previously would always return the path exactly how it was provided, so regardless of platform it just retained the given style.

Ah, I didn't realise that :-)

One way to avoid any confusion would be to add uri and deprecate path, then it's kinda self-documenting. It's probably an unnecessary sort-of-breaking change though :-)

I believe it would also always be an absolute path, which we wouldn't want generally, we want to report relative paths to the user as they are a lot easier to understand.

I could see the JSON reporter always reporting the absolute path, but that would also be breaking, and I worry about doing that change at this time.

I'm not sure I understand how it would be breaking if it used to do that (although, I think it might have actually returned absolute if you pass absolute and relative if you pass relative, since I already have some code in the legacy debug adapter to handle relative paths).

Whatever is decided here, I think the docs/comments for path should definitely be explicit about this. If it's always forward slashes and could be either absolute or relative, that's useful info for consumers to see. Although it's probably also worth noting that older versions of package:test behaved differently, since someone looking at the docs today might be trying to write code that works with as many versions of package:test as possible.

What was actually happening is it was encoding the backslashes in the URI.

Oh, I see. Is it possible there was a mismatch between the platform and it was running on, and the path being given? (eg. a test with a hardcoded Windows path running on non-Windows or vice-versa?).

On DartPad if I run print(Uri.file('c:\\test').path); I get c%3A%5Ctest but if I pass windows: true to simulate Windows behaviour, it's /c:/test.

[jakemac53]

Whatever is decided here, I think the docs/comments for path should definitely be explicit about this.

Yeah I do agree we should update the docs.

Oh, I see. Is it possible there was a mismatch between the platform and it was running on, and the path being given? (eg. a test with a hardcoded Windows path running on non-Windows or vice-versa?).

It was definitely running on "Windows" but that is just whatever the windows environment on github is. It is possible that this was tricking Platform.isWindows into returning the wrong thing, which is I believe what is used by Uri.parse.