Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17507003:
Clean up barback tests. (Closed)
|
Created:
7 years, 6 months ago by Bob Nystrom Modified:
7 years, 5 months ago Reviewers:
nweiz CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionClean up barback tests.
This defines a more DSL-like API for AssetGraph tests. It also handles
build results more cleanly.
R=nweiz@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24734
Patch Set 1 #
Total comments: 37
Patch Set 2 : Revise. #
Total comments: 14
Patch Set 3 : Revise. #Patch Set 4 : Get rid of queue. #
Total comments: 11
Patch Set 5 : Revise. #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/asset_graph/... File pkg/barback/test/asset_graph/errors_test.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/asset_graph/... pkg/barback/test/asset_graph/errors_test.dart:39: }); It definitely doesn't seem correct for a single build to emit both a success and a failure result. It seems like if a call to [getAsset] fails due to the asset not being found, that failure is local to the [getAsset] call; it shouldn't be emitted through [results]. [results] should only emit an [AssetNotFoundException] if some transformer tried to request an asset that didn't exist. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:32: final _buildExpectations = new Queue<Completer<BuildResult>>(); 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); } https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:45: /// functions. Mention that all operations on the graph are scheduled. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:46: AssetGraph initGraph([assets, Iterable<Iterable<Transformer>> transformers]) { Since [assets] isn't type-annotated, the documentation should explain what types are allowed for it. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:65: _buildResults.clear(); Earlier tests should clean up their own mess. This function should use currentSchedule.onComplete to clear out its variables. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:71: // TODO(bob): Temp! *cough cough* https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:76: /// either be an [AssetId] or a string that can be parsed as one. Style nit: the second sentence here should be in a new paragraph. Same goes for other doc comments in this library. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:102: _provider._modifyAsset(name, contents); This seems like it should be scheduled. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:106: /// in after this will wait until the provider is unpaused. "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. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:108: schedule(() { Style nit: =>. Also below. All [schedule] calls in libraries should include descriptions to make it clearer what's happening when an error occurs. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:109: _provider._wait(); 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. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:113: /// Unpauses the provider and allows all pending asset loads to finish. This should refer explicitly to [pauseProvider]. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:122: Future buildShouldSucceed() { This doesn't return a Future. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:133: Future buildShouldFail(void callback(error)) { This doesn't return a Future either, although it's a little weird that it doesn't return the error as a Future. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:153: /// Expects that the graph will return an asset matching [name] and [contents]. 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. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:179: fail("Should have thrown error but got $asset."); Manually failing feels unnecessary when we have library methods for asserting that futures fail. "expect(_graph.getAssetById(id), throwsA(predicate(...)))" would be cleaner.
Thanks! https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/asset_graph/... File pkg/barback/test/asset_graph/errors_test.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/asset_graph/... pkg/barback/test/asset_graph/errors_test.dart:39: }); On 2013/06/25 22:37:56, nweiz wrote: > It definitely doesn't seem correct for a single build to emit both a success and > a failure result. It's more that it emits a success *then* a failure result. The success is because the build does complete. Then the request fails which also gets emitted. > It seems like if a call to [getAsset] fails due to the asset > not being found, that failure is local to the [getAsset] call; it shouldn't be > emitted through [results]. OK, done. I think the last time we discussed this we'd decided that all errors get piped to the results, but I'm agnostic. > [results] should only emit an > [AssetNotFoundException] if some transformer tried to request an asset that > didn't exist. There's a slightly different error for that (MissingInputException) but sounds good. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:32: final _buildExpectations = new Queue<Completer<BuildResult>>(); On 2013/06/25 22:37:56, nweiz wrote: > 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? 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. > 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. 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. > 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); > } 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. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:45: /// functions. On 2013/06/25 22:37:56, nweiz wrote: > Mention that all operations on the graph are scheduled. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:46: AssetGraph initGraph([assets, Iterable<Iterable<Transformer>> transformers]) { On 2013/06/25 22:37:56, nweiz wrote: > Since [assets] isn't type-annotated, the documentation should explain what types > are allowed for it. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:65: _buildResults.clear(); On 2013/06/25 22:37:56, nweiz wrote: > Earlier tests should clean up their own mess. This function should use > currentSchedule.onComplete to clear out its variables. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:71: // TODO(bob): Temp! On 2013/06/25 22:37:56, nweiz wrote: > *cough cough* Oops! Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:76: /// either be an [AssetId] or a string that can be parsed as one. On 2013/06/25 22:37:56, nweiz wrote: > Style nit: the second sentence here should be in a new paragraph. Same goes for > other doc comments in this library. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:102: _provider._modifyAsset(name, contents); On 2013/06/25 22:37:56, nweiz wrote: > This seems like it should be scheduled. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:106: /// in after this will wait until the provider is unpaused. On 2013/06/25 22:37:56, nweiz wrote: > "that come in" -> "that the [AssetGraph] makes to the provider", "will wait" -> > "will not complete" > Done. > This should refer explicitly to [resumeProvider]. Done. > It's not very clear which of these operations are scheduled and which aren't. > Make this clearer. 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. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:108: schedule(() { On 2013/06/25 22:37:56, nweiz wrote: > Style nit: =>. Also below. > > All [schedule] calls in libraries should include descriptions to make it clearer > what's happening when an error occurs. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:109: _provider._wait(); On 2013/06/25 22:37:56, nweiz wrote: > 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. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:113: /// Unpauses the provider and allows all pending asset loads to finish. On 2013/06/25 22:37:56, nweiz wrote: > This should refer explicitly to [pauseProvider]. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:122: Future buildShouldSucceed() { On 2013/06/25 22:37:56, nweiz wrote: > This doesn't return a Future. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:133: Future buildShouldFail(void callback(error)) { On 2013/06/25 22:37:56, nweiz wrote: > This doesn't return a Future either, Done. > although it's a little weird that it > doesn't return the error as a Future. I thought this style looked nice in the tests. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:153: /// Expects that the graph will return an asset matching [name] and [contents]. On 2013/06/25 22:37:56, nweiz wrote: > This comment is confusing. "return" implies that some method is returning a > value, which isn't what's going on here. Changed to "deliver". > This should also document what happen when [contents] isn't passed. Done. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:179: fail("Should have thrown error but got $asset."); On 2013/06/25 22:37:56, nweiz wrote: > Manually failing feels unnecessary when we have library methods for asserting > that futures fail. "expect(_graph.getAssetById(id), throwsA(predicate(...)))" > would be cleaner. 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.
https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:32: final _buildExpectations = new Queue<Completer<BuildResult>>(); On 2013/06/26 20:44:45, Bob Nystrom wrote: > On 2013/06/25 22:37:56, nweiz wrote: > > 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? > > 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. > > > 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. > > 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. > > > 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); > > } > > 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. 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]. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:179: fail("Should have thrown error but got $asset."); On 2013/06/26 20:44:45, Bob Nystrom wrote: > The body of that predicate gets kind of nasty. 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. > 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. If [throwsA] doesn't give a nice error when it doesn't throw, that sounds like a problem with unittest. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:45: /// functions. These other functions are automatically scheduled, with the "These other functions" is unclear, especially since there are functions in this file unrelated to this change that aren't scheduled. I think it makes the most sense to just mention in each function's documentation that it's scheduled. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:48: /// [assets] may either be an [Iterable] or a [Map]. If an [Iterable], each "If an" -> "If it's an" https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:51: /// [AssetId] and the value is a string defining the contents of that asset. "value is" -> "value should be" https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:84: /// will typically wrap it in a call to [schedule] yourself. "you will typically wrap it" is confusing. Most of your tests don't schedule this. If they did, then it seems like it should be automatically scheduled. Same goes for [removeSources]. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:120: /// Pauses the internally created [AssetProvider]. All asset requests that the Paragraph break https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:124: schedule(() =>_provider._pause(), "resume provider"); "resume" -> "pause" https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:130: schedule(() => _provider._resume(), "pause provider"); "pause" -> "resume"
https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:32: final _buildExpectations = new Queue<Completer<BuildResult>>(); On 2013/06/27 00:19:25, nweiz wrote: > Unless the AssetGraph constructor is synchronously emitting results, which it shouldn't > be, 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). > there's no way for results to be emitted between construction and the call to [_getNextBuildResult]. 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. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:45: /// functions. These other functions are automatically scheduled, with the On 2013/06/27 00:19:25, nweiz wrote: > "These other functions" is unclear, especially since there are functions in this > file unrelated to this change that aren't scheduled. I think it makes the most > sense to just mention in each function's documentation that it's scheduled. Done. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:48: /// [assets] may either be an [Iterable] or a [Map]. If an [Iterable], each On 2013/06/27 00:19:25, nweiz wrote: > "If an" -> "If it's an" Done. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:51: /// [AssetId] and the value is a string defining the contents of that asset. On 2013/06/27 00:19:25, nweiz wrote: > "value is" -> "value should be" Done. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:84: /// will typically wrap it in a call to [schedule] yourself. On 2013/06/27 00:19:25, nweiz wrote: > "you will typically wrap it" is confusing. Most of your tests don't schedule > this. If they did, then it seems like it should be automatically scheduled. > > Same goes for [removeSources]. Done. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:120: /// Pauses the internally created [AssetProvider]. All asset requests that the On 2013/06/27 00:19:25, nweiz wrote: > Paragraph break Done. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:124: schedule(() =>_provider._pause(), "resume provider"); On 2013/06/27 00:19:25, nweiz wrote: > "resume" -> "pause" Done. https://codereview.chromium.org/17507003/diff/6001/pkg/barback/test/utils.dar... pkg/barback/test/utils.dart:130: schedule(() => _provider._resume(), "pause provider"); On 2013/06/27 00:19:25, nweiz wrote: > "pause" -> "resume" Done.
https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:32: final _buildExpectations = new Queue<Completer<BuildResult>>(); On 2013/06/27 17:55:14, Bob Nystrom wrote: > On 2013/06/27 00:19:25, nweiz wrote: > > Unless the AssetGraph constructor is synchronously emitting results, which it > shouldn't > > be, > > 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). > > > there's no way for results to be emitted between construction and the call to > [_getNextBuildResult]. > > 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. 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]. > 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 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'm not too crazy about the queuing, but it was the cleanest solution I could > come up with that left the tests simple. I may be misunderstanding your point here, but I think my original proposal for getting rid of queuing still holds up.
Ditched the queue. https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/1/pkg/barback/test/utils.dart#n... pkg/barback/test/utils.dart:32: final _buildExpectations = new Queue<Completer<BuildResult>>(); On 2013/06/27 21:08:46, nweiz wrote: > On 2013/06/27 17:55:14, Bob Nystrom wrote: > > On 2013/06/27 00:19:25, nweiz wrote: > > > Unless the AssetGraph constructor is synchronously emitting results, which > it > > shouldn't > > > be, > > > > 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). > > > > > there's no way for results to be emitted between construction and the call > to > > [_getNextBuildResult]. > > > > 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. > > 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]. > > > 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 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'm not too crazy about the queuing, but it was the cleanest solution I could > > come up with that left the tests simple. > > I may be misunderstanding your point here, but I think my original proposal for > getting rid of queuing still holds up. Done!
https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:26: int _nextBuildResult; Apparently we have a StreamIterator class, which seems applicable here. https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:109: _graph.results.elementAt(_nextBuildResult++).then(wrapAsync((result) { I strongly prefer expect(..., completes) to [wrapAsync] when using futures. With [wrapAsync], if [elementAt] fails for any reason, that error will be top-leveled. https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:127: void waitForBuild() { Isn't it possible that a build emits an error before it finishes? Even if it's not now, it will be in the future, right? https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:129: return _graph.results.first; Style nit: =>
Thanks! https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:26: int _nextBuildResult; On 2013/06/27 23:12:20, nweiz wrote: > Apparently we have a StreamIterator class, which seems applicable here. I tried that, but it doesn't allow queueing up multiple requests. If you call moveNext() again before the future returned by the first moveNext() call completes, it throws an error. https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:109: _graph.results.elementAt(_nextBuildResult++).then(wrapAsync((result) { On 2013/06/27 23:12:20, nweiz wrote: > I strongly prefer expect(..., completes) to [wrapAsync] when using futures. With > [wrapAsync], if [elementAt] fails for any reason, that error will be > top-leveled. Done. https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:127: void waitForBuild() { On 2013/06/27 23:12:20, nweiz wrote: > Isn't it possible that a build emits an error before it finishes? Even if it's > not now, it will be in the future, right? Yup. Modified this to check that the result is actually a build success. https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:129: return _graph.results.first; On 2013/06/27 23:12:20, nweiz wrote: > Style nit: => Invalidated by above change.
lgtm https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:26: int _nextBuildResult; On 2013/07/02 21:41:28, Bob Nystrom wrote: > On 2013/06/27 23:12:20, nweiz wrote: > > Apparently we have a StreamIterator class, which seems applicable here. > > I tried that, but it doesn't allow queueing up multiple requests. If you call > moveNext() again before the future returned by the first moveNext() call > completes, it throws an error. Gross. File a bug?
Thanks! https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:26: int _nextBuildResult; On 2013/07/03 00:40:35, nweiz wrote: > On 2013/07/02 21:41:28, Bob Nystrom wrote: > > On 2013/06/27 23:12:20, nweiz wrote: > > > Apparently we have a StreamIterator class, which seems applicable here. > > > > I tried that, but it doesn't allow queueing up multiple requests. If you call > > moveNext() again before the future returned by the first moveNext() call > > completes, it throws an error. > > Gross. File a bug? It's the documented behavior. :-/
Message was sent while issue was closed.
Committed patchset #5 manually as r24734 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.dart File pkg/barback/test/utils.dart (right): https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... pkg/barback/test/utils.dart:26: int _nextBuildResult; On 2013/07/03 17:12:16, Bob Nystrom wrote: > On 2013/07/03 00:40:35, nweiz wrote: > > On 2013/07/02 21:41:28, Bob Nystrom wrote: > > > On 2013/06/27 23:12:20, nweiz wrote: > > > > Apparently we have a StreamIterator class, which seems applicable here. > > > > > > I tried that, but it doesn't allow queueing up multiple requests. If you > call > > > moveNext() again before the future returned by the first moveNext() call > > > completes, it throws an error. > > > > Gross. File a bug? > > It's the documented behavior. :-/ Then file a feature request? It's good to have these things documented so when other people try to do the same thing they can add their voices.
Message was sent while issue was closed.
On 2013/07/03 18:17:15, nweiz wrote: > https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.dart > File pkg/barback/test/utils.dart (right): > > https://codereview.chromium.org/17507003/diff/22001/pkg/barback/test/utils.da... > pkg/barback/test/utils.dart:26: int _nextBuildResult; > On 2013/07/03 17:12:16, Bob Nystrom wrote: > > On 2013/07/03 00:40:35, nweiz wrote: > > > On 2013/07/02 21:41:28, Bob Nystrom wrote: > > > > On 2013/06/27 23:12:20, nweiz wrote: > > > > > Apparently we have a StreamIterator class, which seems applicable here. > > > > > > > > I tried that, but it doesn't allow queueing up multiple requests. If you > > call > > > > moveNext() again before the future returned by the first moveNext() call > > > > completes, it throws an error. > > > > > > Gross. File a bug? > > > > It's the documented behavior. :-/ > > Then file a feature request? It's good to have these things documented so when > other people try to do the same thing they can add their voices. Done: https://code.google.com/p/dart/issues/detail?id=11686 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
