Make servePackages's contents argument saner.

sdk/lib/_internal/pub/test/serve_packages.dart

+// Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library serve_packages;
+
+import 'dart:async';
+import 'dart:convert';
+
+import 'package:path/path.dart' as p;
+import 'package:scheduled_test/scheduled_test.dart';
+import 'package:yaml/yaml.dart';
+
+import '../lib/src/io.dart';
+import '../lib/src/utils.dart';
+import '../lib/src/version.dart';
+import 'descriptor.dart' as d;
+import 'test_pub.dart';
+
+/// The [d.DirectoryDescriptor] describing the server layout of `/api/packages`
+/// on the test server.
+///
+/// This contains metadata for packages that are being served via
+/// [servePackages]. It's `null` if [servePackages] has not yet been called for
+/// this test.
+d.DirectoryDescriptor _servedApiPackageDir;
+
+/// The [d.DirectoryDescriptor] describing the server layout of `/packages` on
+/// the test server.
+///
+/// This contains the tarballs for packages that are being served via
+/// [servePackages]. It's `null` if [servePackages] has not yet been called for
+/// this test.
+d.DirectoryDescriptor _servedPackageDir;
+
+PackageServerBuilder _builder;
+
+/// Creates an HTTP server that replicates the structure of pub.dartlang.org.
+///
+/// [pubspecs] is a list of unserialized pubspecs representing the packages to
+/// serve.
+///
+/// If [replace] is false, subsequent calls to [servePackages] will add to the
+/// set of packages that are being served. Previous packages will continue to be
+/// served. Otherwise, the previous packages will no longer be served.
+///
+/// If [contents] is given, it should be a map from package names and versions
+/// (e.g. "foo-1.2.3") to their contents.

[Bob Nystrom, 2014/08/21 16:44:17]

Remove this.

[nweiz, 2014/08/21 18:28:51]

Done.

+///
+/// If [serveBarback] is true, the repo versions of barback and its dependencies
+/// will be served as well.

[Bob Nystrom, 2014/08/21 16:44:17]

Remove this.

[nweiz, 2014/08/21 18:28:52]

Done.

+void servePackages(void callback(PackageServerBuilder builder),
+    {bool replace: false}) {

[Bob Nystrom, 2014/08/21 16:44:17]

Ah, now I see why builder isn't optional. You *could* make two functions:
servePackages() and serveOnlyPackages() if you wanted.

[nweiz, 2014/08/21 18:28:52]

Done.

+  if (_servedPackageDir == null) {
+    _builder = new PackageServerBuilder();
+    _servedApiPackageDir = d.dir('packages', []);
+    _servedPackageDir = d.dir('packages', []);
+    serve([
+      d.dir('api', [_servedApiPackageDir]),
+      _servedPackageDir
+    ]);
+
+    currentSchedule.onComplete.schedule(() {
+      _builder = null;
+      _servedApiPackageDir = null;
+      _servedPackageDir = null;
+    }, 'cleaning up served packages');
+  }
+
+  schedule(() {
+    if (replace) _builder = new PackageServerBuilder();
+    callback(_builder);
+    return _builder._await().then((resolvedPubspecs) {
+      _servedApiPackageDir.contents.clear();
+      _servedPackageDir.contents.clear();
+      _builder._packages.forEach((name, versions) {
+        _servedApiPackageDir.contents.addAll([
+          d.file('$name', JSON.encode({
+            'name': name,
+            'uploaders': ['nweiz@google.com'],
+            'versions': versions.map((version) =>
+                packageVersionApiMap(version.pubspec)).toList()
+          })),
+          d.dir(name, [
+            d.dir('versions', versions.map((version) {
+              return d.file(version.version.toString(), JSON.encode(
+                  packageVersionApiMap(version.pubspec, full: true)));
+            }))
+          ])
+        ]);
+
+        _servedPackageDir.contents.add(d.dir(name, [
+          d.dir('versions', versions.map((version) =>
+              d.tar('${version.version}.tar.gz', version.contents)))
+        ]));
+      });
+    });
+  }, 'initializing the package server');
+}
+
+class PackageServerBuilder {

[Bob Nystrom, 2014/08/21 16:44:17]

Doc comment here and below.

[nweiz, 2014/08/21 18:28:52]

Done.

+  final _packages = new Map<String, List<_ServedPackage>>();
+  var _futures = new FutureGroup();

[Bob Nystrom, 2014/08/21 16:44:17]

I know it's initialized here, but I think we still tend to annotate non-final
fields.

[nweiz, 2014/08/21 18:28:52]

I don't think we're consistent either way. Why would we annotate when the type
is already right there, though?

+
+  void serve(String name, String version, {Map pubspec,
+      Iterable<d.Descriptor> contents}) {
+    _futures.add(awaitObject(pubspec)).then((resolvedPubspec) {
+      var pubspecFields = {
+        "name": name,
+        "version": version
+      };
+      if (resolvedPubspec != null) pubspecFields.addAll(resolvedPubspec);
+
+      if (contents == null) contents = [d.libDir(name, "$name $version")];
+      contents = [d.file("pubspec.yaml", yaml(pubspecFields))]
+          ..addAll(contents);
+
+      var packages = _packages.putIfAbsent(name, () => []);
+      packages.add(new _ServedPackage(pubspecFields, contents));
+    });
+  }
+
+  void serveRepoPackage(String package) {
+    _addPackage(name) {
+      if (_packages.containsKey(name)) return;
+      _packages[name] = [];
+
+      var pubspec = new Map.from(loadYaml(
+          readTextFile(p.join(repoRoot, 'pkg', name, 'pubspec.yaml'))));
+
+      // Remove any SDK constraints since we don't have a valid SDK version
+      // while testing.
+      pubspec.remove('environment');
+
+      _packages[name].add(new _ServedPackage(pubspec, [
+        d.file('pubspec.yaml', yaml(pubspec)),
+        new d.DirectoryDescriptor.fromFilesystem('lib',
+            p.join(repoRoot, 'pkg', name, 'lib'))
+      ]));
+
+      if (pubspec.containsKey('dependencies')) {
+        pubspec['dependencies'].keys.forEach(_addPackage);
+      }
+    }
+
+    _addPackage(package);
+  }
+
+  Future _await() {
+    if (_futures.futures.isEmpty) return new Future.value();
+    return _futures.future.then((_) => _futures = new FutureGroup());

[Bob Nystrom, 2014/08/21 16:44:17]

The intent here isn't to return the FutureGroup, right? You're just using => for
brevity?

In most cases, that's fine, but I think here it might be clearer to use a {}
body just to make it obvious that FutureGroup isn't itself a Future that's being
deliberately awaited on again.

[nweiz, 2014/08/21 18:28:52]

Done.

+  }
+}
+
+class _ServedPackage {
+  final Map pubspec;
+  final List<d.Descriptor> contents;
+
+  Version get version => new Version.parse(pubspec['version']);
+
+  _ServedPackage(this.pubspec, this.contents);
+}