Use a real package-root for all of our samples (and tests inside a package) instead of buildDir/pac…

Use a real package-root for all of our samples (and tests inside a package) instead of buildDir/packages/

For all tests that depend on other packages (e.g. samples) we can
 - use packages from pub.dartlang.org
 - use packages from pub.dartlang.org with overrides for the packages we have
   in the dart repository.

This CL does NOT include test-specific package-roots for dartium. Dartium will request a URL containing
"packages" when it encounters a package import. Our HTTP server will then serve from buildDir/packages.
This needs to change when we have test specific VMOptions for dartium.

For normal tests (without a pubspec.yaml on the path to dart/, we still use buildDir/packages).

R=ricow@google.com, whesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31691

[kustermann, 2014-01-07 17:16:37]

The CL isn't completely finished (but 95%). 
Feedback is welcome.

[ricow1, 2014-01-08 11:48:48]

Didn't I alrady review parts of this before?

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:533: }).then((_) {
remove this line, no async stuff above

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_suite.dart:720: }).then((_) {
remove this then

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_suite.dart:1179: // FIXME: If we're on dartium, we
need to pass [packageRoot] on to the
FIXME -> TODO(issue 14651)

[kustermann, 2014-01-09 13:10:48]

PTAL

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:533: }).then((_) {
On 2014/01/08 11:48:48, ricow1 wrote:
> remove this line, no async stuff above

Done.

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_suite.dart:720: }).then((_) {
On 2014/01/08 11:48:48, ricow1 wrote:
> remove this then

Done.

https://codereview.chromium.org/126173002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_suite.dart:1179: // FIXME: If we're on dartium, we
need to pass [packageRoot] on to the
On 2014/01/08 11:48:48, ricow1 wrote:
> FIXME -> TODO(issue 14651)

Done.

[ricow1, 2014-01-09 13:58:29]

LGTM

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:878:
.append('pkg').append('expect').append('lib');
bob: Please be aware of this, if we ever push expect to pub we could get into
issues here

[Bill Hesse, 2014-01-09 14:40:08]

LGTM.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:536: return link.delete().then((_) =>
link.create(_target));
This could be written

return link.exists()
    .then((bool exists) { if (exists) return link.delete(); })
    .then((_) => link.create(_target));

(The middle function completes its return future with null if !exists)

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:718:
discoverPackagesInRepository().then((Map packageDirectories) {
This does the entire search for each test.  I would do

return updateDartium().then((_) {
  if (localPackageDirectories == null) {
    return discoverPackagesInRepository()
      .then((Map packageDirectories) {
        localPackageDirectories = packageDirectories;
      });
}).then((_) {
...

or else I would make discoverPackagesInRepository cache its results, and return
a completed future if it has already found them, and change its name to
packagesInRepository.


or I would move both updateDartium and localPackageDirectories to enqueueTests.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:837: // the root directory (excluding
the root).
This is easily memoizable, with a map root -> filepath -> result,
so that each directory only needs to be searched once, and then the result of
the search of it and all its parent directories is saved.

if (memoized) return cached result
else result = (search this directory) or result(parent)
   memoize result
   return result

And root is always the same, isn't it?

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:870: // pubspec.yaml if there is no lib
directory.
This comment seems backwards, since the code that uses packageLinkTarget creates
the link if there _is_ a lib directory.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:914:
findPubspecYamlFile(TestUtils.dartDir(), filePath);
pubspecYamlFile could be put in the cached testinfo, instead of doing the search
each time.  But this is not necessary.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:945: enqueueBrowserTest(baseCommands,
packageRoot, info, testName,
Move all args to next line, or one per line?

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:1180: 
So currently, packageRoot is unused here?

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:1195: optionsName =
vmOptions.join('-').replaceAll('-','')
There is something wrong with .join('-').replaceAll('-', '').  Maybe the replace
should be a "_"?

[kustermann, 2014-01-10 12:30:03]

Committed patchset #3 manually as r31691 (presubmit successful).

[kustermann, 2014-01-10 12:30:07]

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:536: return link.delete().then((_) =>
link.create(_target));
On 2014/01/09 14:40:08, Bill Hesse wrote:
> This could be written
> 
> return link.exists()
>     .then((bool exists) { if (exists) return link.delete(); })
>     .then((_) => link.create(_target));
> 
> (The middle function completes its return future with null if !exists)

Done.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:718:
discoverPackagesInRepository().then((Map packageDirectories) {
On 2014/01/09 14:40:08, Bill Hesse wrote:
> This does the entire search for each test.  I would do
> 
> return updateDartium().then((_) {
>   if (localPackageDirectories == null) {
>     return discoverPackagesInRepository()
>       .then((Map packageDirectories) {
>         localPackageDirectories = packageDirectories;
>       });
> }).then((_) {
> ...
> 
> or else I would make discoverPackagesInRepository cache its results, and
return
> a completed future if it has already found them, and change its name to
> packagesInRepository.
> 
> 
> or I would move both updateDartium and localPackageDirectories to
enqueueTests.

We're not doing this for each test. This method is called to enqueue *ALL* tests
in a test suite. Enqueueing all tests is done in the "enqueueTests()" call
below.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:837: // the root directory (excluding
the root).
On 2014/01/09 14:40:08, Bill Hesse wrote:
> This is easily memoizable, with a map root -> filepath -> result,
> so that each directory only needs to be searched once, and then the result of
> the search of it and all its parent directories is saved.
> 
> if (memoized) return cached result
> else result = (search this directory) or result(parent)
>    memoize result
>    return result
> 
> And root is always the same, isn't it?

I haven't noticed a slowdown due to this. But yes, I can of course cache the
information.

The expensive operation is the the exists syscall, so I implemented caching for
this.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:870: // pubspec.yaml if there is no lib
directory.
On 2014/01/09 14:40:08, Bill Hesse wrote:
> This comment seems backwards, since the code that uses packageLinkTarget
creates
> the link if there _is_ a lib directory.

That comment is fine. Look here:

dart $ l samples/clock      
-rw-r----- 1 kustermann eng  160 Dec 19 10:18 pubspec.yaml
drwxr-x--- 2 kustermann eng 4.0K Dec 19 10:18 test/
drwxr-x--- 3 kustermann eng 4.0K Dec 19 10:18 web/

dart $ l
out/ReleaseIA32/generated_pubspec_checkouts/repo_packages-sdk/samples_clock/
drwxr-x--- 2 kustermann eng 4.0K Jan 10 13:17 packages/
-rw-r----- 1 kustermann eng 8.6K Jan 10 13:17 pubspec.lock
-rw-r----- 1 kustermann eng 4.8K Jan 10 13:17 pubspec.yaml

We copy the directory to out/ReleaseIA32/.... and make a modified pubspec.yaml
file in there. When running 'pub get' inside that directory, the directory
itself doesn't contain a lib directory, so pub doesn't create a link in
packages/clock -> ...
What this code means that if there is a lib directory inside samples/clock/lib
then we'll create a link from out/..../packages to samples/clock/lib.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:914:
findPubspecYamlFile(TestUtils.dartDir(), filePath);
On 2014/01/09 14:40:08, Bill Hesse wrote:
> pubspecYamlFile could be put in the cached testinfo, instead of doing the
search
> each time.  But this is not necessary.

The caching of TestInformation objects does usually not provide much benefit.
Only if we run multiple configurations (which doesn't happen that often).

Furthermore we'd pollute this class with information that is only relevant for
specific tests, not for all.

I think it's fine. There are more low hanging fruits to make test.dart faster.
If it turns out to be a problem we can change it.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:945: enqueueBrowserTest(baseCommands,
packageRoot, info, testName,
On 2014/01/09 14:40:08, Bill Hesse wrote:
> Move all args to next line, or one per line?

All args to next line: It doesn't fit!
Having one line per argument is not so nice IMHO.

I'm looking forward to the dartfmt tool, so we can just run it as a presubmit
script, and it will always do the right thing :)

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:1180: 
On 2014/01/09 14:40:08, Bill Hesse wrote:
> So currently, packageRoot is unused here?

Yes, and packageRoot is not necessary for dart2js since we've only one test.js
(dart2js needs the packageRoot not the browser).

The only other case when we go into this method is none-dartium/none-drt.

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:1195: optionsName =
vmOptions.join('-').replaceAll('-','')
On 2014/01/09 14:40:08, Bill Hesse wrote:
> There is something wrong with .join('-').replaceAll('-', '').  Maybe the
replace
> should be a "_"?

Not related to this CL. Feel free to investigate.

[Bill Hesse, 2014-01-10 15:12:41]

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/126173002/diff/70001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_suite.dart:870: // pubspec.yaml if there is no lib
directory.
OK, it is unclear.  I wouldn't say "if there is no lib directory", I would say
"since the lib directory in the source is not copied to the checkout directory".
 Because you only make the link if there _is_ a directory in the source
directory.

On 2014/01/10 12:30:08, kustermann wrote:
> On 2014/01/09 14:40:08, Bill Hesse wrote:
> > This comment seems backwards, since the code that uses packageLinkTarget
> creates
> > the link if there _is_ a lib directory.
> 
> That comment is fine. Look here:
> 
> dart $ l samples/clock      
> -rw-r----- 1 kustermann eng  160 Dec 19 10:18 pubspec.yaml
> drwxr-x--- 2 kustermann eng 4.0K Dec 19 10:18 test/
> drwxr-x--- 3 kustermann eng 4.0K Dec 19 10:18 web/
> 
> dart $ l
> out/ReleaseIA32/generated_pubspec_checkouts/repo_packages-sdk/samples_clock/
> drwxr-x--- 2 kustermann eng 4.0K Jan 10 13:17 packages/
> -rw-r----- 1 kustermann eng 8.6K Jan 10 13:17 pubspec.lock
> -rw-r----- 1 kustermann eng 4.8K Jan 10 13:17 pubspec.yaml
> 
> We copy the directory to out/ReleaseIA32/.... and make a modified pubspec.yaml
> file in there. When running 'pub get' inside that directory, the directory
> itself doesn't contain a lib directory, so pub doesn't create a link in
> packages/clock -> ...
> What this code means that if there is a lib directory inside samples/clock/lib
> then we'll create a link from out/..../packages to samples/clock/lib.