Add more tests for .packages

Add more tests for .packages

BUG=
R=iposva@google.com, pquitslund@google.com

Committed: https://github.com/dart-lang/sdk/commit/c6655ad43de17f1f87083cc6b6e16fd6bfe677b0

[Harry Terkelsen, 2015-08-18 23:30:04]

het@google.com changed reviewers:
+ iposva@google.com, pquitslund@google.com

[Ivan Posva, 2015-08-18 23:46:07]

iposva@google.com changed reviewers:
+ lrn@google.com

[Ivan Posva, 2015-08-18 23:46:07]

Generally LGTM. Still we should before committing clarify the open question with
Lasse about the intent of empty URI references.

Thanks
-Ivan

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
File
tests/standalone/package/scenarios/empty_packages_file/empty_packages_file_noimports_test.dart
(right):

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
tests/standalone/package/scenarios/empty_packages_file/empty_packages_file_noimports_test.dart:8:
library empty_packages_file_noimports_test;
Please clarify in a comment in this file whether you expect this to fail to load
or not.

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
File
tests/standalone/package/scenarios/packages_file_strange_formatting/empty_package_dir_test.dart
(right):

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
tests/standalone/package/scenarios/packages_file_strange_formatting/empty_package_dir_test.dart:11:
// `import 'package:foo/foo.dart'` is equivalent to `import '/foo.dart'`.
I am wondering whether the VM and dart2js implement the intended behaviour. The
packages spec says that "The package location is a URI reference" which the
empty string is, but is this really what we want. Lasse?

[Lasse Reichstein Nielsen, 2015-08-19 06:54:40]

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
File
tests/standalone/package/scenarios/packages_file_strange_formatting/empty_package_dir_test.dart
(right):

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
tests/standalone/package/scenarios/packages_file_strange_formatting/empty_package_dir_test.dart:11:
// `import 'package:foo/foo.dart'` is equivalent to `import '/foo.dart'`.
I think we do want it, even if it's likely to be ridiculous.
It *should* resolve to the path of the ".packages" file with a "/" after, which
is likely not a real directory.
Or maybe to the directory the ".packages" file is in, I'm not sure the
specification I wrote considered the possibility that it could make a difference
- which it doesn't if the URI reference has a non-empty path.

It's unlikely to be what you want, but I'd also prefer to not add extra
restrictions on which URI references you can use - it's just a URI reference,
and there are lots of bad references you can write. It has to make sense or your
program to work, empty or not.

[Lasse Reichstein Nielsen, 2015-08-19 11:41:05]

On 2015/08/19 06:54:40, Lasse Reichstein Nielsen wrote:

> I think we do want it, even if it's likely to be ridiculous.
> It *should* resolve to the path of the ".packages" file with a "/" after,
which
> is likely not a real directory.

I checked the wording and this is correct.

> Or maybe to the directory the ".packages" file is in, 

and not this. I'll check what the package_config parser actually does.

[Lasse Reichstein Nielsen, 2015-08-19 12:51:35]

On 2015/08/19 11:41:05, Lasse Reichstein Nielsen wrote:
> I'll check what the package_config parser actually does.

That actually adds the trailing "/" before resolving, so the result becomes the
same as for `foo:/` - the root directory. That's still not useful.
Whether adding the "/" before or after resolving is correct isn't clear from the
text, but after is probably the saner choice. I'll update the package_config.

[pquitslund, 2015-08-19 15:50:22]

On 2015/08/19 12:51:35, Lasse Reichstein Nielsen wrote:
> On 2015/08/19 11:41:05, Lasse Reichstein Nielsen wrote:
> > I'll check what the package_config parser actually does.
> 
> That actually adds the trailing "/" before resolving, so the result becomes
the
> same as for `foo:/` - the root directory. That's still not useful.
> Whether adding the "/" before or after resolving is correct isn't clear from
the
> text, but after is probably the saner choice. I'll update the package_config.

Cool.  Is this a change we want to pull in for 1.12?

[pquitslund, 2015-08-19 15:50:49]

On 2015/08/19 15:50:22, pquitslund wrote:
> On 2015/08/19 12:51:35, Lasse Reichstein Nielsen wrote:
> > On 2015/08/19 11:41:05, Lasse Reichstein Nielsen wrote:
> > > I'll check what the package_config parser actually does.
> > 
> > That actually adds the trailing "/" before resolving, so the result becomes
> the
> > same as for `foo:/` - the root directory. That's still not useful.
> > Whether adding the "/" before or after resolving is correct isn't clear from
> the
> > text, but after is probably the saner choice. I'll update the
package_config.
> 
> Cool.  Is this a change we want to pull in for 1.12?

Otherwise, LGTM.

Thanks for pushing on this Harry!

[Harry Terkelsen, 2015-08-19 20:17:07]

Committed patchset #2 (id:20001) manually as
c6655ad43de17f1f87083cc6b6e16fd6bfe677b0 (presubmit successful).

[Harry Terkelsen, 2015-08-19 20:17:22]

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
File
tests/standalone/package/scenarios/empty_packages_file/empty_packages_file_noimports_test.dart
(right):

https://codereview.chromium.org/1298323002/diff/1/tests/standalone/package/sc...
tests/standalone/package/scenarios/empty_packages_file/empty_packages_file_noimports_test.dart:8:
library empty_packages_file_noimports_test;
On 2015/08/18 23:46:07, Ivan Posva wrote:
> Please clarify in a comment in this file whether you expect this to fail to
load
> or not.

Done.