Add support for hybrid VM/browser tests. #509
Conversation
This adds `spawnHybridUri()` and `spawnHybridCode()` methods that spawn VM isolates even when called from the browser. It still needs some additional work to close the isolates when the test ends by default. See #109
| library that communicates over WebSockets. We call tests that run code on both | ||
| the browser and the VM **hybrid tests**. | ||
|
|
||
| Hybrid tests use one of two functions: [`spawnHybridCode()`][spawnHybridUri] and |
There was a problem hiding this comment.
Looks like the url references got reversed
| import '../util/remote_exception.dart'; | ||
| import '../utils.dart'; | ||
|
|
||
| /// A transformer that handles messages from the spawned isolate and ensures |
There was a problem hiding this comment.
can this doc mention the 'print' functionality?
| sink.add(message); | ||
| })); | ||
|
|
||
| /// Spawns a VM isolate for the given [uri], which may be a [Uri] or a [String]. |
There was a problem hiding this comment.
what is the benefit of having an untyped uri argument?
There was a problem hiding this comment.
Can be URI or String.
We need union types. 😄
There was a problem hiding this comment.
More specifically, in practice URLs are represented as Strings and Uri objects with similar frequency in Dart. I try to make my APIs handle either representation to avoid making my users write lots of annoying .toString() or Uri.parse() calls.
| /// easily spawn an isolate. | ||
| /// | ||
| /// The Dart file at [uri] must define a top-level `hybridMain()` function that | ||
| /// takes a `StreamChannel` argument and, optionally, an `Object` argument to |
There was a problem hiding this comment.
Why do we need two ways to pass data? If there is initial data can we send it as the first value through the stream channel?
There was a problem hiding this comment.
You can, but the Stream API doesn't make it particularly easy to get the first event without also canceling the stream subscription. This also mirrors the Isolate spawn APIs more closely.
| throw new ArgumentError.value(uri, "uri", "must be a Uri or a String."); | ||
| } | ||
|
|
||
| String uriString; |
There was a problem hiding this comment.
we've already seen a uri which may have been a String. Can this one be absoluteUri so the distinction is more explicit?
| /// that messages sent to it are JSON-encodable. | ||
| final _transformer = new StreamChannelTransformer( | ||
| new StreamTransformer.fromHandlers(handleData: (message, sink) { | ||
| switch (message['type']) { |
There was a problem hiding this comment.
[nit] line 159 uses double quotes in similar usage
| /// decoded after being JSON-serialized. | ||
| final _transformer = new StreamSinkTransformer.fromHandlers( | ||
| handleData: (data, sink) { | ||
| ensureJsonEncodable(data); |
There was a problem hiding this comment.
what happens if this throws? Should we be putting an error on the sink?
There was a problem hiding this comment.
This is called synchronously from the user's Sink.add() call, so if ensureJsonEncodable() throws here they'll get a helpful stack trace. I'll add a comment to that effect.
| } | ||
|
|
||
| if (main is! Function) { | ||
| _sendError(channel, "Top-level hybridMain getter is not a function."); |
There was a problem hiding this comment.
[nit] this usage of 'getter' might not match what some readers expect. I might phrase this as "Top-level hybridMain is not a function."
| /// | ||
| /// **Note**: If you use this API, be sure to add a dependency on the | ||
| /// **`stream_channel` package, since you're using its API as well! | ||
| StreamChannel spawnHybridCode(String dartCode, {Object message}) { |
There was a problem hiding this comment.
DBC: This seems cool – but feel scary.
Is there a strong case for this over just giving folks the URI-based method?
There was a problem hiding this comment.
Can you elaborate on "scary"?
This is useful for the common situation where the hybrid isolate is very simple. It lets users keep the full definition of their test in one file.
| sink.add(message); | ||
| })); | ||
|
|
||
| /// Spawns a VM isolate for the given [uri], which may be a [Uri] or a [String]. |
There was a problem hiding this comment.
Can be URI or String.
We need union types. 😄
This adds
spawnHybridUri()andspawnHybridCode()methods that spawnVM isolates even when called from the browser. It still needs some
additional work to close the isolates when the test ends by default.
See #109