Skip to content
This repository was archived by the owner on Aug 28, 2024. It is now read-only.

correctly parse package_config files on windows with relative root URI #371

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

jonahwilliams
Copy link
Contributor

When running flutter test --coverage on windows, we noticed that forcing package:coverage to use the package_config.json file would lead to errors like:

00:01 +1 -1: loading C:\Users\Jonah\flutter\examples\hello_world\test\hello_test.dart [E]
  Invalid argument(s): Uri c:/lib/ must have scheme 'file:'.
  package:path/src/style/windows.dart 86:7                       WindowsStyle.pathFromUri
  package:path/src/context.dart 1000:32                          Context.fromUri
  package:path/path.dart 417:32                                  fromUri
  package:coverage/src/resolver.dart 68:29                       Resolver.resolve
  package:coverage/src/hitmap.dart 60:31                         HitMap.parseJson
  ===== asynchronous gap ===========================
  package:flutter_tools/src/test/coverage_collector.dart 114:16  CoverageCollector.collectCoverage
  ===== asynchronous gap ===========================
  package:flutter_tools/src/test/coverage_collector.dart 33:5    CoverageCollector.handleFinishedTest
  ===== asynchronous gap ===========================
  package:flutter_tools/src/test/flutter_platform.dart 515:11    FlutterPlatform._startTest.<fn>
  ===== asynchronous gap ===========================
  dart:async/future.dart 611:5                                   Future.any.onValue

It turns out that if the root URI in a package config file was relative, it was not parsed correctly since resolving the provided packagesPath was not working correctly on windows (resolving to wrong dir).

To fix this, we first convert the provided packages path to a URI and then resolve that. Tests are updated to all pass on windows as well.

Unblocks flutter/flutter#99677 / https://dart-review.googlesource.com/c/sdk/+/231101

@@ -23,7 +30,12 @@ foo:file:///${d.sandbox}/foo/lib
"packages": [
{
"name": "foo",
"rootUri": "file:///${d.sandbox}/foo",
"rootUri": "../",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style of URI is used for the package itself and is what coverage was having problems resolving. Since the packagesPath would resolve to an incorrect directory, this would resolve to something like C:/lib which doesn't exist or isn't valid

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.577% when pulling 9bbb073 on jonahwilliams:fix_windows_paths into ba4e575 on dart-lang:master.

@jonahwilliams
Copy link
Contributor Author

@natebosch

@natebosch
Copy link
Contributor

cc @liamappelbe - based on this it looks like we already support package_config.json using the existing flag. I don't think we need to do any major changes.

I suppose we could consider updating the option description from "package spec" to "package config"...

https://github.com/dart-lang/coverage/blob/e2613263f893898e25ac28b4b3f74b199aca2388/bin/format_coverage.dart#L118

@natebosch natebosch requested a review from liamappelbe March 17, 2022 21:41
Copy link
Contributor

@natebosch natebosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM but it would be good for @liamappelbe to double check

@liamappelbe
Copy link
Contributor

LGTM

cc @liamappelbe - based on this it looks like we already support package_config.json using the existing flag. I don't think we need to do any major changes.

I suppose we could consider updating the option description from "package spec" to "package config"...

https://github.com/dart-lang/coverage/blob/e2613263f893898e25ac28b4b3f74b199aca2388/bin/format_coverage.dart#L118

It's true that no major changes are needed for the .packages breaking change. We're already migrated in fact. I really just wanted to add a --package flag so the users can omit the /.dart_tool/package_config.json from their path, since it's unnecessary (since we can use findPackageConfig) and it's weird that users need to know about this hidden file. Then Michael wanted me to delete the --packages flag at the same time. Anyway, that's not really relevant to this PR, which LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants