Take Condition for async nesting expectations #1896
Conversation
Avoid the awkwardness of awaiting before chaining conditions, or needing a `which` utility. This introduces a discrepancy between chaining for sync vs async expectations, but it makes many common async expectations nicer to read.
|
@lrhn @jakemac53 - What are your thoughts on this change? Do you think this makes a positive impact in |
| @@ -15,7 +15,7 @@ extension FutureChecks<T> on Subject<Future<T>> { | |||
| /// future completes. | |||
| /// | |||
| /// Fails if the future completes as an error. | |||
| Future<Subject<T>> completes() => | |||
| Future<void> completes([Condition<T>? completionCondition]) => | |||
There was a problem hiding this comment.
I prefer the original here. It's explicit.
I assume that if the future has an error, the returned future has a test-failure error. (Which becomes an uncaught error if you don't do anything, which is fine, because we run tests in error zone.)
The alternative here seems to be, effectively doing a then for you, so
check(myFuture).completes(conditon);is equivalent to the original
check(myFuture).completes().then(condition.applyAsync);
I don't think I'd want to use a Condition object directly if I can avoid it.
I see three options here:
Left: Return Future<Subject<T>>, and I assume it's a test failure if the future had an error.
Right: Pass in test
Alternative: Return an "AsynchronousSubject" which accepts all the same tests as a normal Subject, but won't apply them until the future completes. (I think I tried that, and it got very complicated very quickly.)
There was a problem hiding this comment.
I assume that if the future has an error, the returned future has a test-failure error. (Which becomes an uncaught error if you don't do anything, which is fine, because we run tests in error zone.)
Correct.
Alternative: Return an "AsynchronousSubject" which accepts all the same tests as a normal
Subject, but won't apply them until the future completes. (I think I tried that, and it got very complicated very quickly.)
This also wouldn't allow for waiting for the expectation to pass before doing more work or checking more expectations. Any existing test doing await expectLater today would not have a migration pattern if we lost the ability to return a Future for async expectations.
There was a problem hiding this comment.
Right: Pass in test
The condition is optional.
In both cases .completes() behave identically - cause a failure if the future has en error, otherwise succeed.
With the current code, chaining has multiple choices for how to write it. In order of preference for how I would write it:
await check(actual).completes().which(it()..furtherCondition());
(await check(actual).completes()).furtherCondition();
await check(actual).completes().then((result) => result.furtherCondition());
final result = await check(actual).completes();
result.furtherCondition();After this PR, this would collapse to one choice
await check(actual).completes(it()..furtherCondition));I like
- That there is only 1 option.
- That the 1 option reads nicer than any of the multiple choice options IMO.
I'm less sure that we can confidently say there are no valid use cases for choosing one of the other patterns. I'm also less sure that everyone will have the distaste for (await check(actual).completes()).furtherCondition() that I picked up while working on this package.
The main benefit this brings is it brings more alignment with the `clause` arguments for `expect` calls. The docs will be able to focus on the difference in how the value is use (preceding "that" in the case of labels, standing on its own in a list in the case of clauses) and can use a consistent description for how it is passed. A secondary benefit is that it allows multiline labels and avoid workaround like joining with `r'\n'`. A final benefit is that it saves some unnecessary String formatting since the callback isn't called if no expectations fail on the Subject, or when used as a soft check where the failure details are ignored. - Make the `label` arguments to `nest` and `nestAsync`, and the _label field in `_TestContext` an `Iterable<String> Function()`. - Wrap strings that had been passed to `String` arguments with callbacks that return the string in a list. - When writing the label in a failure, write all lines, and use a postfix " that:". - Update some `Map` expectations which had manually joined with literal slash-n to keep the label or clause to a single line to take advantage of the multiline allowance. Split tests for the changed implementations and add tests for the descriptions with multiline examples. Some of these could have used multiline clauses before.
This is the main point of interaction for authors of extensions, so docs cover a wide range of information, including how the descriptions will be used in failure output.
Add a doc on the `_label` member. Add doc on `_child` constructor.
Expand description of label a bit. Remove reference to "single line" A future PR will make the line-per-element behavior more clear at the class level.
Start with more general information, and then get more specific. Add some dartdoc templates for bits that are useful to repeat in the docs for individual methods.
- Add changelog. - Remove the mention of the async `which` from the README. - Describe the difference in signature between `nest` and `nestAsync` in the `nestAsync` doc. - Add more docs for `Condition`. - Update docs for changing async expectation extensions. - Make `LazyCondition` private and add a doc.
natebosch
changed the base branch from
master
to
label-callback--context-docs
|
I do think we should land this. If we hear from people that it's too limiting, we can change it back. |
They don't need to be read externally, the callers only care about the two constructors, and these details are good to hide because the two nullable field pattern is ugly. This also fixes dartdoc references to the constructors with [Extracted.rejection]. Otherwise it would link to the field.
Had done a tweak in `has` that I noticed when copy/pasting it as an example, but then I decided to go with creating fake examples for all the operations because we don't have any nice small example for `expectAsync`.
Link to the migration guide so that there is a link from the pub and docs sites. Fix an example usage of `completes` after #1896
Link to the migration guide so that there is a link from the pub and docs sites. Fix an example usage of `completes` after #1896
Avoid the awkwardness of awaiting before chaining conditions, or
needing a
whichutility.This introduces a discrepancy between chaining for sync vs async
expectations, but it makes many common async expectations nicer to read.