Clean up barback tests.

pkg/barback/test/utils.dart

Index: pkg/barback/test/utils.dart
diff --git a/pkg/barback/test/utils.dart b/pkg/barback/test/utils.dart
index 23f8e2a1267320b4297299264186dd5f615bde8f..67762e51bb0653980dd85cb7b9dd0e8ec1914e6c 100644
--- a/pkg/barback/test/utils.dart
+++ b/pkg/barback/test/utils.dart
@@ -5,6 +5,7 @@
 library barback.test.utils;
 
 import 'dart:async';
+import 'dart:collection';
 
 import 'package:barback/barback.dart';
 import 'package:barback/src/asset_graph.dart';
@@ -16,14 +17,141 @@ import '../../../sdk/lib/_internal/pub/test/command_line_config.dart';
 
 var _configured = false;
 
+MockProvider _provider;
+AssetGraph _graph;
+
+/// [BuildResult]s that have been output by the graph before an expectation has
+/// consumed them.
+///
+/// Since [AssetGraph] starts building immediately in the background, results
+/// may start streaming before we've set an explicit expectation. When that
+/// occurs, we just queue them up here.
+final _buildResults = new Queue<BuildResult>();
+
+/// The expectations we have on upcoming [BuildResult]s.
+final _buildExpectations = new Queue<Completer<BuildResult>>();

[nweiz, 2013/06/25 22:37:56]

I really don't like this [_buildResults]/[_buildExpectations] setup. We already
have a stream that emits results; we should use that. If we can't, how do we
expect users of barback to use it?

In particular, I think we can get rid of any explicit buffering here. As long as
we subscribe to the expectations synchronously, we can avoid dropping any on the
ground. This means that [_getNextBuildResult] should call
[_graph.results.elementAt] and return the result, and anything consuming that
should use expect(..., completes).

If it's important that the result appears at a certain point in the schedule,
you can wrap the result in schedule(). E.g.:

    Future<BuildResult> _getNextBuildResult() {
      var result = currentSchedule.wrapFuture(
          _graph.results.elementAt(i++));
      return schedule(() => result);
    }

[Bob Nystrom, 2013/06/26 20:44:45]

Maybe the results stream shouldn't be broadcast by default?

Most of the complexity here is to avoid dropping results on the floor before the
test is ready for them. If that's a concern for outside users too, maybe it
should be single-cast.
Right. My goal here was to not require the subscriptions to be synchronous.
Doing that means ordering the tests in a non-intuitive way. For example, I think
this reads well:

    initGraph();
    updateSources(["app|unknown.txt"]);

    buildShouldFail((error) {
      expect(error, new isInstanceOf<AssetNotFoundException>());
      expect(error.id, equals(new AssetId.parse("app|unknown.txt")));
    });

If it didn't queue, you'd have to do:

    initGraph();
    buildShouldFail((error) {
      expect(error, new isInstanceOf<AssetNotFoundException>());
      expect(error.id, equals(new AssetId.parse("app|unknown.txt")));
    });

    updateSources(["app|unknown.txt"]);

Which isn't the order that things happen in. The reason is that updateSource()
starts a build going in the background immediately.
It's not usually important that it appear at a certain time in the schedule,
it's more about how the code is ordered and read.

[nweiz, 2013/06/27 00:19:25]

You're being fuzzy about what "in the background" and "immediately" mean. Unless
the AssetGraph constructor is synchronously emitting results, which it shouldn't
be, there's no way for results to be emitted between construction and the call
to [_getNextBuildResult].

[Bob Nystrom, 2013/06/27 17:55:14]

That's correct. It always pumps the event loop. Actually, the constructor
doesn't start a build at all. You have to update some sources first. But those
will also pump the event loop (and there is a test for that).
Right. Here's an example of where the queue helps. This is 

  test("does not report asset not found errors in results", () {
    initGraph();
    expectNoAsset("app|foo.txt");
    buildShouldSucceed();
  });

Those last two calls are scheduled, but actually rely on some interleaving. What
happens is:

1. expectNoAsset() calls getAssetById()
   a. that starts a build
   b. the build completes
   c. a result is sent to the stream
   d. the result is added to the result queue
   e. the getAssetById() future completes with a "no asset" error
   f. expectNoAsset() catches that error and validates it
2. buildShouldSucceed() calls _getNextBuildResult()
   a. the result queue already has a result, so we just dequeue it
   b. it validates the result

If we don't queue the results then 1. and 2. are blocked on each other. We could
split those out, but then test gets a bunch nastier. More like:

  test("does not report asset not found errors in results", () {
    var graph = initGraph();

    var resultFuture = graph.results.first.then((result) {
      expect(result.succeeded, isTrue);
    });

    schedule(() => resultFuture);

    var assetFuture = getAsset("app|foo.txt").then((asset) {
      fail("Should have thrown error but got $asset.");
    }).catchError((error) {
      expect(error, new isInstanceOf<AssetNotFoundException>());
      expect(error.id, equals(new AssetId.parse("app|foo.txt")));
    });

    schedule(() => assetFuture);
  });

Note that here the calls to *create* the futures are *not* scheduled, but then
*waiting for them to complete* is.

I'm not too crazy about the queuing, but it was the cleanest solution I could
come up with that left the tests simple.

[nweiz, 2013/06/27 21:08:46]

I don't understand why they're blocked on one another. [getAssetById] should
terminate once the asset fails to be retrieved, regardless of who (if anyone) is
listening on [results]. [_getNextBuildResult] should terminate regardless of
when anyone called [getAssetById].
I don't understand the point of this example. It seems like a reasonable way of
defining [buildShouldSucceed] and [expectNoAsset], except that the call to
[getAsset] should be scheduled.
I may be misunderstanding your point here, but I think my original proposal for
getting rid of queuing still holds up.

[Bob Nystrom, 2013/06/27 22:23:07]

Done!

+
 void initConfig() {
   if (_configured) return;
   _configured = true;
   unittestConfiguration = new CommandLineConfiguration();
 }
 
-/// Expects that [graph] will return an asset matching [name] and [contents].
-void expectAsset(AssetGraph graph, String name, [String contents]) {
+/// Creates a new [AssetProvider] and [AssetGraph] with the given [assets] and
+/// [transformers].
+///
+/// This graph is used internally by most of the other functions in this
+/// library so you must call it in the test before calling any of the other
+/// functions.

[nweiz, 2013/06/25 22:37:56]

Mention that all operations on the graph are scheduled.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+AssetGraph initGraph([assets, Iterable<Iterable<Transformer>> transformers]) {

[nweiz, 2013/06/25 22:37:56]

Since [assets] isn't type-annotated, the documentation should explain what types
are allowed for it.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+  if (assets == null) assets = [];
+  if (transformers == null) transformers = [];
+
+  _provider = new MockProvider(assets);
+  _graph = new AssetGraph(_provider, transformers);
+
+  _graph.results.listen(wrapAsync((result) {
+    if (_buildExpectations.isEmpty) {
+      // We aren't waiting for a result yet, so just queue it up for later.
+      _buildResults.add(result);
+    } else {
+      // See if it meets the expectation.
+      _buildExpectations.removeFirst().complete(result);
+    }
+  }));
+
+  // Discard any previous builds from earlier tests. It's OK for a test to not
+  // care about some build results.
+  _buildResults.clear();

[nweiz, 2013/06/25 22:37:56]

Earlier tests should clean up their own mess. This function should use
currentSchedule.onComplete to clear out its variables.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+
+  // This library should ensure that you can't move to the next test until all
+  // build expectations are processed.
+  assert(_buildExpectations.isEmpty);
+
+  // TODO(bob): Temp!

[nweiz, 2013/06/25 22:37:56]

*cough cough*

[Bob Nystrom, 2013/06/26 20:44:45]

Oops! Done.

+  return _graph;
+}
+
+/// Updates [assets] in the current [AssetProvider]. Each item in the list may
+/// either be an [AssetId] or a string that can be parsed as one.

[nweiz, 2013/06/25 22:37:56]

Style nit: the second sentence here should be in a new paragraph. Same goes for
other doc comments in this library.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+void updateSources(List assets) {
+  // Allow strings as asset IDs.
+  assets = assets.map((asset) {
+    if (asset is String) return new AssetId.parse(asset);
+    return asset;
+  });
+
+  _graph.updateSources(assets);
+}
+
+/// Removes [assets] from the current [AssetProvider]. Each item in the list
+/// may either be an [AssetId] or a string that can be parsed as one.
+void removeSources(assets) {
+  // Allow strings as asset IDs.
+  assets = assets.map((asset) {
+    if (asset is String) return new AssetId.parse(asset);
+    return asset;
+  });
+
+  _graph.removeSources(assets);
+}
+
+/// Changes the contents of an asset identified by [name] to [contents]. Does
+/// not update it in the graph.
+void modifyAsset(String name, String contents) {
+  _provider._modifyAsset(name, contents);

[nweiz, 2013/06/25 22:37:56]

This seems like it should be scheduled.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+}
+
+/// Pauses the internally created [AssetProvider]. All asset requests that come
+/// in after this will wait until the provider is unpaused.

[nweiz, 2013/06/25 22:37:56]

"that come in" -> "that the [AssetGraph] makes to the provider", "will wait" ->
"will not complete"

This should refer explicitly to [resumeProvider].

It's not very clear which of these operations are scheduled and which aren't.
Make this clearer.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.
 
Done.
Almost all of them are except for updateSources() and removeSources() because
there are some tests related to how multiple synchronous calls to those
interact. Added some more docs.

+void pauseProvider() {
+  schedule(() {

[nweiz, 2013/06/25 22:37:56]

Style nit: =>. Also below.

All [schedule] calls in libraries should include descriptions to make it clearer
what's happening when an error occurs.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+    _provider._wait();

[nweiz, 2013/06/25 22:37:56]

It's weird that the public methods are named "pause" and "resume" but the
private methods are named "wait" and "complete". They should probably be
consistent.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+  });
+}
+
+/// Unpauses the provider and allows all pending asset loads to finish.

[nweiz, 2013/06/25 22:37:56]

This should refer explicitly to [pauseProvider].

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+void resumeProvider() {
+  schedule(() {
+    _provider._complete();
+  });
+}
+
+/// Waits for the next [BuildResult] to be emitted and asserts that it is a
+/// build success.
+Future buildShouldSucceed() {

[nweiz, 2013/06/25 22:37:56]

This doesn't return a Future.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.

+  schedule(() {
+    return _getNextBuildResult().then((result) {
+      expect(result.succeeded, isTrue);
+    });
+  });
+}
+
+/// Waits for the next [BuildResult] to be emitted and asserts that it is a
+/// build failure. Invokes [callback] with the error (not the result) so that
+/// it can provide more precise expectations.
+Future buildShouldFail(void callback(error)) {

[nweiz, 2013/06/25 22:37:56]

This doesn't return a Future either, although it's a little weird that it
doesn't return the error as a Future.

[Bob Nystrom, 2013/06/26 20:44:45]

Done.
I thought this style looked nice in the tests.

+  schedule(() {
+    return _getNextBuildResult().then((result) {
+      expect(result.succeeded, isFalse);
+      callback(result.error);
+    });
+  });
+}
+
+Future<BuildResult> _getNextBuildResult() {
+  if (_buildResults.isNotEmpty) {
+    return new Future.value(_buildResults.removeFirst());
+  }
+
+  // We don't have any results yet, so enqueue the expectation.
+  var completer = new Completer<BuildResult>();
+  _buildExpectations.add(completer);
+  return completer.future;
+}
+
+/// Expects that the graph will return an asset matching [name] and [contents].

[nweiz, 2013/06/25 22:37:56]

This comment is confusing. "return" implies that some method is returning a
value, which isn't what's going on here.

This should also document what happen when [contents] isn't passed.

[Bob Nystrom, 2013/06/26 20:44:45]

Changed to "deliver".
Done.

+void expectAsset(String name, [String contents]) {
   var id = new AssetId.parse(name);
 
   if (contents == null) {
@@ -31,7 +159,7 @@ void expectAsset(AssetGraph graph, String name, [String contents]) {
   }
 
   schedule(() {
-    return graph.getAssetById(id).then((asset) {
+    return _graph.getAssetById(id).then((asset) {
       // TODO(rnystrom): Make an actual Matcher class for this.
       expect(asset, new isInstanceOf<MockAsset>());
       expect(asset._id.package, equals(id.package));
@@ -41,13 +169,13 @@ void expectAsset(AssetGraph graph, String name, [String contents]) {
   }, "get asset $name");
 }
 
-/// Expects that [graph] will not find an asset matching [name].
-void expectNoAsset(AssetGraph graph, String name) {
+/// Expects that the graph will not find an asset matching [name].
+void expectNoAsset(String name) {
   var id = new AssetId.parse(name);
 
   // Make sure the future gets the error.
   schedule(() {
-    return graph.getAssetById(id).then((asset) {
+    return _graph.getAssetById(id).then((asset) {
       fail("Should have thrown error but got $asset.");

[nweiz, 2013/06/25 22:37:56]

Manually failing feels unnecessary when we have library methods for asserting
that futures fail. "expect(_graph.getAssetById(id), throwsA(predicate(...)))"
would be cleaner.

[Bob Nystrom, 2013/06/26 20:44:45]

The body of that predicate gets kind of nasty. This lets me give nice error
messages for each expectation instead of just "predicate failed" and also give a
nice error when getAssetById() doesn't throw.

[nweiz, 2013/06/27 00:19:25]

I thought you could just use [expect] in [predicate], but that doesn't actually
work well. Maybe something like `completion((value) => ...)` should be added,
but for now this is fine.
If [throwsA] doesn't give a nice error when it doesn't throw, that sounds like a
problem with unittest.

     }).catchError((error) {
       expect(error, new isInstanceOf<AssetNotFoundException>());
@@ -56,12 +184,12 @@ void expectNoAsset(AssetGraph graph, String name) {
   }, "get asset $name");
 }
 
-/// Expects that [graph] will have an output file collision error on an asset
+/// Expects that the graph will have an output file collision error on an asset
 /// matching [name].
-Future expectCollision(AssetGraph graph, String name) {
+Future expectCollision(String name) {
   var id = new AssetId.parse(name);
   return schedule(() {
-    return graph.results.first.then((result) {
+    return _graph.results.first.then((result) {
       expect(result.error, new isInstanceOf<AssetCollisionException>());
       expect(result.error.id, equals(id));
     });
@@ -93,19 +221,19 @@ class MockProvider implements AssetProvider {
   /// The completer that [getAsset()] is waiting on to complete.
   ///
   /// If `null` it will return the asset immediately.
-  Completer _wait;
+  Completer _waitCompleter;
 
   /// Tells the provider to wait during [getAsset] until [complete()]
   /// is called.
   ///
   /// Lets you test the asynchronous behavior of loading.
-  void wait() {
-    _wait = new Completer();
+  void _wait() {
+    _waitCompleter = new Completer();
   }
 
-  void complete() {
-    _wait.complete();
-    _wait = null;
+  void _complete() {
+    _waitCompleter.complete();
+    _waitCompleter = null;
   }
 
   MockProvider(assets) {
@@ -125,7 +253,7 @@ class MockProvider implements AssetProvider {
     }
   }
 
-  void modifyAsset(String name, String contents) {
+  void _modifyAsset(String name, String contents) {
     var id = new AssetId.parse(name);
     var asset = _packages[id.package].firstWhere((a) => a._id == id);
     asset._contents = contents;
@@ -141,8 +269,8 @@ class MockProvider implements AssetProvider {
 
   Future<Asset> getAsset(AssetId id) {
     var future;
-    if (_wait != null) {
-      future = _wait.future;
+    if (_waitCompleter != null) {
+      future = _waitCompleter.future;
     } else {
       future = new Future.value();
     }