Sharing metatest logic between unittest and scheduled_test

Sharing metatest logic between unittest and scheduled_test

R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=40475

[kevmoo, 2014-08-30 20:11:49]

kevmoo@google.com changed reviewers:
+ nweiz@google.com

[kevmoo, 2014-08-30 20:11:51]

I've ported some of the code from scheduled_test here. 

Two big changes:

1) I created a bit of a function pass-around dance so I can run tests on both
browser and console without 1145 fixed
2) I didn't want to add a dependency on scheduled_test, so there are some safe
guards I had to remove – most notably your timeout check.

[nweiz, 2014-09-02 18:27:58]

Before I look through this whole thing, what do you think about extracting this
into a metatest package? I feel pretty squicky about copy/pasting so much code.

[kevmoo, 2014-09-06 23:37:00]

PTAL

Also, look at scheduled_test/test/scheduled_server_test – doesn't seem to ever
pass

[nweiz, 2014-09-08 22:44:41]

I've just reviewed metatest.dart so far; I want to get that API ironed out
before I start looking at code that uses it.

Metatest also needs a README.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:17: Future defer(void fn()) {
What is this for? Where is it used? Why not just call Future.sync manually? Why
isn't this documented? Why is it a publicly-exposed function of this package?

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:22: /// overwritten if the test itself calls
[setUp].
While you're in here, can you fix the comments to have the first sentence take
up its own paragraph?

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:27: void expectTestResults(String description,
void body(),
Document public functions.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:30: var list = resultsMap['results'] as List;
Don't type annotate local variables. "as" counts.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:31: expect(list,
hasLength(expectedResults.length));
Why don't these [expect] calls have reasons anymore? if this fails, it's going
to be very unclear what happened.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:45: body()) {
Nit: don't split the type and variable name across lines.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:49: throw 'Expected only one test to run';
Don't throw strings.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:76: (t) => t['result'] == 'pass').map((t) =>
t['description']));
Why did you change the formattin ghere? This looks much worse.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:84: 'Expected exactly ${stringify(shouldPass)} to
pass, but '
This also doesn't seem like a good reformatting at all.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:134: Function _testBody;
Document these fields.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:141: /// whether this test is in the parent
isolate or a child isolate.
Document [timeout].

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:142: void initMetatest(Map message, {Duration
timeout}) {
I don't like type-annotating [message] here. The fact that it's a map is an
implementation detail. Metatest should be able to change it to e.g. a SendPort
if it wants.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:156: void initTests(void testBody(message)) {
Why does the user have to call both [initTests] and [initMetatest]? They should
only have to call one initialization function. Since you have the body here, you
can automatically wrap it and have it call [initMetatest].

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:165: throw new StateError('initTests was not
called.');
Explain how to use initTests here.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:173: return replyPort.first;
Nit: Use "=>" here

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:181: results['uncaughtError'] != null ||
Fix all these gratuitous formatting changes.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:261: String _prefixLines(String text, {String
prefix: '| ', String firstPrefix}) {
This should go in a utils file.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/pubspec.yaml
File pkg/metatest/pubspec.yaml (right):

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/pubspec.yam...
pkg/metatest/pubspec.yaml:2: version: 0.1.0
Are you planning on iterating more on this? If not, make it 1.0.0.

[kevmoo, 2014-09-10 03:17:29]

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:17: Future defer(void fn()) {
On 2014/09/08 22:44:40, nweiz wrote:
> What is this for? Where is it used? Why not just call Future.sync manually?
Why
> isn't this documented? Why is it a publicly-exposed function of this package?

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:22: /// overwritten if the test itself calls
[setUp].
On 2014/09/08 22:44:41, nweiz wrote:
> While you're in here, can you fix the comments to have the first sentence take
> up its own paragraph?

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:27: void expectTestResults(String description,
void body(),
On 2014/09/08 22:44:40, nweiz wrote:
> Document public functions.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:30: var list = resultsMap['results'] as List;
On 2014/09/08 22:44:40, nweiz wrote:
> Don't type annotate local variables. "as" counts.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:31: expect(list,
hasLength(expectedResults.length));
On 2014/09/08 22:44:41, nweiz wrote:
> Why don't these [expect] calls have reasons anymore? if this fails, it's going
> to be very unclear what happened.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:45: body()) {
On 2014/09/08 22:44:40, nweiz wrote:
> Nit: don't split the type and variable name across lines.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:49: throw 'Expected only one test to run';
On 2014/09/08 22:44:40, nweiz wrote:
> Don't throw strings.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:76: (t) => t['result'] == 'pass').map((t) =>
t['description']));
On 2014/09/08 22:44:41, nweiz wrote:
> Why did you change the formattin ghere? This looks much worse.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:84: 'Expected exactly ${stringify(shouldPass)} to
pass, but '
On 2014/09/08 22:44:40, nweiz wrote:
> This also doesn't seem like a good reformatting at all.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:134: Function _testBody;
On 2014/09/08 22:44:40, nweiz wrote:
> Document these fields.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:141: /// whether this test is in the parent
isolate or a child isolate.
On 2014/09/08 22:44:40, nweiz wrote:
> Document [timeout].

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:142: void initMetatest(Map message, {Duration
timeout}) {
On 2014/09/08 22:44:40, nweiz wrote:
> I don't like type-annotating [message] here. The fact that it's a map is an
> implementation detail. Metatest should be able to change it to e.g. a SendPort
> if it wants.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:156: void initTests(void testBody(message)) {
On 2014/09/08 22:44:40, nweiz wrote:
> Why does the user have to call both [initTests] and [initMetatest]? They
should
> only have to call one initialization function. Since you have the body here,
you
> can automatically wrap it and have it call [initMetatest].

I wish I could figure out how to do this. _body is passed to Isolate.spawn. I
couldn't find a way to do this.

I cranked on this for a while and got nowhere. Would love if you could prove me
wrong.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:165: throw new StateError('initTests was not
called.');
On 2014/09/08 22:44:40, nweiz wrote:
> Explain how to use initTests here.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:173: return replyPort.first;
On 2014/09/08 22:44:41, nweiz wrote:
> Nit: Use "=>" here

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:181: results['uncaughtError'] != null ||
On 2014/09/08 22:44:40, nweiz wrote:
> Fix all these gratuitous formatting changes.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:261: String _prefixLines(String text, {String
prefix: '| ', String firstPrefix}) {
On 2014/09/08 22:44:40, nweiz wrote:
> This should go in a utils file.

Done.

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/pubspec.yaml
File pkg/metatest/pubspec.yaml (right):

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/pubspec.yam...
pkg/metatest/pubspec.yaml:2: version: 0.1.0
On 2014/09/08 22:44:41, nweiz wrote:
> Are you planning on iterating more on this? If not, make it 1.0.0.

Actually, yes. In my infinite free time, I'm evolving several things in our test
universe towards a cleaner architecture. I don't want to stamp this as v1 yet.

[nweiz, 2014-09-10 21:04:43]

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/60001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:156: void initTests(void testBody(message)) {
On 2014/09/10 03:17:28, kevmoo wrote:
> On 2014/09/08 22:44:40, nweiz wrote:
> > Why does the user have to call both [initTests] and [initMetatest]? They
> should
> > only have to call one initialization function. Since you have the body here,
> you
> > can automatically wrap it and have it call [initMetatest].
> 
> I wish I could figure out how to do this. _body is passed to Isolate.spawn. I
> couldn't find a way to do this.
> 
> I cranked on this for a while and got nowhere. Would love if you could prove
me
> wrong.

Oh, I see, it's the top-level-function-only thing. Gross.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:17: import 'src/util.dart';
Nit: We use "utils", not "util".

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:30: final _singleton = new _MetaConfiguration();
"singleton" isn't a good name for this: it only communicates that there's only
one instance of this variable, not what the variable actually is.
"metaConfiguration" would be a better name.

Alternately, keep it "_singleton" and make it a static variable of
"_MetaConfiguration" that's set/accessed in a factory constructor.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:51: /// with values in `expectedResults`.
This still doesn't explain what the format of [expectedResults] is.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:55: var list = resultsMap['results'] as List;
This still uses "as".

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:56: expect(list,
hasLength(expectedResults.length));
These expect calls still don't have descriptions.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:64: });
Can't you just do expect(map, equals(expectedMap))?

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:70: /// match the values generated by the
finished test.
What are the semantics of "result" and "message"?

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:79: reason: 'The test result did not meet
expectations.');
"Unexpected test result."

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:81: reason: 'The test message did not meet
expectations.');
"Unexpected test message."

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:111: '${stringify(didPass)} passed.\n'
'${_summarizeTests(results)}');
There's still gratuitous reformatting here.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:223: '${results['errors']} ERRORS');
There's still reformatting all through this method.

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/descriptor/async_test.dart:8: import
'package:metatest/metatest.dart';
Move "package:" imports with other "package:" imports.

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:18:
expectTestResults('a scheduled test with an out-of-band error should fail', () {
Long line.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
File pkg/unittest/test/async_exception_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
pkg/unittest/test/async_exception_test.dart:22: throw "an error!";
This doesn't actually run asynchronously; Future.sync runs its contents
synchronously, as the name indicates.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
pkg/unittest/test/async_exception_test.dart:28: }]);
Un-indent these lines two spaces. Same goes for other tests.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
File pkg/unittest/test/async_setup_teardown_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
pkg/unittest/test/async_setup_teardown_test.dart:18: expectSingleTest('good
setup/good teardown', 'pass', '', () {
This interface is really hard to read. Why is the status a string? Why not a
boolean or, if it can have more than two values, an enum?

I'd also make the status and description named parameters. Having three unnamed
strings in a row is really confusing.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
File pkg/unittest/test/completion_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
pkg/unittest/test/completion_test.dart:38: }]);
Why are you explicitly checking the test results for tests that don't care about
the test messages? Why not just use [expectTestsPass]?

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/utils....
File pkg/unittest/test/utils.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/utils....
pkg/unittest/test/utils.dart:8: return new Future.sync(fn);
This name is misleading, since [fn] is executed synchronously. Also, why not
just use [new Future]/[new Future.sync] directly?

[kevmoo, 2014-09-17 21:16:29]

PTAL

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:17: import 'src/util.dart';
On 2014/09/10 21:04:42, nweiz wrote:
> Nit: We use "utils", not "util".

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:30: final _singleton = new _MetaConfiguration();
On 2014/09/10 21:04:42, nweiz wrote:
> "singleton" isn't a good name for this: it only communicates that there's only
> one instance of this variable, not what the variable actually is.
> "metaConfiguration" would be a better name.
> 
> Alternately, keep it "_singleton" and make it a static variable of
> "_MetaConfiguration" that's set/accessed in a factory constructor.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:51: /// with values in `expectedResults`.
On 2014/09/10 21:04:42, nweiz wrote:
> This still doesn't explain what the format of [expectedResults] is.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:55: var list = resultsMap['results'] as List;
On 2014/09/10 21:04:41, nweiz wrote:
> This still uses "as".

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:56: expect(list,
hasLength(expectedResults.length));
On 2014/09/10 21:04:42, nweiz wrote:
> These expect calls still don't have descriptions.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:64: });
On 2014/09/10 21:04:42, nweiz wrote:
> Can't you just do expect(map, equals(expectedMap))?

I want to match a subset. There may be more in 'map' than 'expectedMap' which is
fine.

expectedMap does not need to match everything in map

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:70: /// match the values generated by the
finished test.
On 2014/09/10 21:04:42, nweiz wrote:
> What are the semantics of "result" and "message"?

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:79: reason: 'The test result did not meet
expectations.');
On 2014/09/10 21:04:42, nweiz wrote:
> "Unexpected test result."

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:81: reason: 'The test message did not meet
expectations.');
On 2014/09/10 21:04:42, nweiz wrote:
> "Unexpected test message."

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:111: '${stringify(didPass)} passed.\n'
'${_summarizeTests(results)}');
On 2014/09/10 21:04:41, nweiz wrote:
> There's still gratuitous reformatting here.

Done, I think...

https://codereview.chromium.org/524153002/diff/80001/pkg/metatest/lib/metates...
pkg/metatest/lib/metatest.dart:223: '${results['errors']} ERRORS');
On 2014/09/10 21:04:42, nweiz wrote:
> There's still reformatting all through this method.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/descriptor/async_test.dart:8: import
'package:metatest/metatest.dart';
On 2014/09/10 21:04:42, nweiz wrote:
> Move "package:" imports with other "package:" imports.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/524153002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:18:
expectTestResults('a scheduled test with an out-of-band error should fail', () {
On 2014/09/10 21:04:42, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
File pkg/unittest/test/async_exception_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
pkg/unittest/test/async_exception_test.dart:22: throw "an error!";
On 2014/09/10 21:04:42, nweiz wrote:
> This doesn't actually run asynchronously; Future.sync runs its contents
> synchronously, as the name indicates.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
pkg/unittest/test/async_exception_test.dart:28: }]);
On 2014/09/10 21:04:42, nweiz wrote:
> Un-indent these lines two spaces. Same goes for other tests.

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
File pkg/unittest/test/async_setup_teardown_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/async_...
pkg/unittest/test/async_setup_teardown_test.dart:18: expectSingleTest('good
setup/good teardown', 'pass', '', () {
On 2014/09/10 21:04:43, nweiz wrote:
> This interface is really hard to read. Why is the status a string? Why not a
> boolean or, if it can have more than two values, an enum?
> 
> I'd also make the status and description named parameters. Having three
unnamed
> strings in a row is really confusing.

I eliminated expectSingleTest. Fewer methods == better

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
File pkg/unittest/test/completion_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
pkg/unittest/test/completion_test.dart:38: }]);
On 2014/09/10 21:04:43, nweiz wrote:
> Why are you explicitly checking the test results for tests that don't care
about
> the test messages? Why not just use [expectTestsPass]?

Done.

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/utils....
File pkg/unittest/test/utils.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/utils....
pkg/unittest/test/utils.dart:8: return new Future.sync(fn);
On 2014/09/10 21:04:43, nweiz wrote:
> This name is misleading, since [fn] is executed synchronously. Also, why not
> just use [new Future]/[new Future.sync] directly?

Done.

[nweiz, 2014-09-17 22:39:16]

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
File pkg/unittest/test/completion_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
pkg/unittest/test/completion_test.dart:38: }]);
On 2014/09/17 21:16:29, kevmoo wrote:
> On 2014/09/10 21:04:43, nweiz wrote:
> > Why are you explicitly checking the test results for tests that don't care
> about
> > the test messages? Why not just use [expectTestsPass]?
> 
> Done.

There are still a number of tests that do this.

https://codereview.chromium.org/524153002/diff/120001/pkg/metatest/lib/metate...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/120001/pkg/metatest/lib/metate...
pkg/metatest/lib/metatest.dart:55: /// case: "description", "message", "result",
and "stackTrace".
Explain what the possible formats of "result" are. Also explain what format
"stackTrace" is (native? package:stack_trace?).

https://codereview.chromium.org/524153002/diff/120001/pkg/metatest/lib/metate...
pkg/metatest/lib/metatest.dart:75: reason: 'The number of tests run does not
match the numbef of expected'
"numbef" -> "number"

https://codereview.chromium.org/524153002/diff/120001/pkg/unittest/test/excep...
File pkg/unittest/test/exception_test.dart (right):

https://codereview.chromium.org/524153002/diff/120001/pkg/unittest/test/excep...
pkg/unittest/test/exception_test.dart:23: }]);
Similarly, this should use [expectTestsFail].

[kevmoo, 2014-09-18 22:11:33]

PTAL

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
File pkg/unittest/test/completion_test.dart (right):

https://codereview.chromium.org/524153002/diff/80001/pkg/unittest/test/comple...
pkg/unittest/test/completion_test.dart:38: }]);
On 2014/09/17 22:39:16, nweiz wrote:
> On 2014/09/17 21:16:29, kevmoo wrote:
> > On 2014/09/10 21:04:43, nweiz wrote:
> > > Why are you explicitly checking the test results for tests that don't care
> > about
> > > the test messages? Why not just use [expectTestsPass]?
> > 
> > Done.
> 
> There are still a number of tests that do this.

Done.

https://codereview.chromium.org/524153002/diff/120001/pkg/metatest/lib/metate...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/120001/pkg/metatest/lib/metate...
pkg/metatest/lib/metatest.dart:55: /// case: "description", "message", "result",
and "stackTrace".
On 2014/09/17 22:39:16, nweiz wrote:
> Explain what the possible formats of "result" are. Also explain what format
> "stackTrace" is (native? package:stack_trace?).

Done, although I was not specific about where the StackTrace comes from.
Depending on options set in unitest (formatStacks, etc) it may be a system stack
trace or one from pkg/stack_trace

https://codereview.chromium.org/524153002/diff/120001/pkg/metatest/lib/metate...
pkg/metatest/lib/metatest.dart:75: reason: 'The number of tests run does not
match the numbef of expected'
On 2014/09/17 22:39:16, nweiz wrote:
> "numbef" -> "number"

Done.

https://codereview.chromium.org/524153002/diff/120001/pkg/unittest/test/excep...
File pkg/unittest/test/exception_test.dart (right):

https://codereview.chromium.org/524153002/diff/120001/pkg/unittest/test/excep...
pkg/unittest/test/exception_test.dart:23: }]);
On 2014/09/17 22:39:16, nweiz wrote:
> Similarly, this should use [expectTestsFail].

Nope. This is an error, not a fail.

[nweiz, 2014-09-18 22:16:03]

lgtm

https://codereview.chromium.org/524153002/diff/140001/pkg/metatest/lib/metate...
File pkg/metatest/lib/metatest.dart (right):

https://codereview.chromium.org/524153002/diff/140001/pkg/metatest/lib/metate...
pkg/metatest/lib/metatest.dart:57: /// The value of "result" can be one of:
'pass', 'fail', or 'error'
Add a period at the end of the sentence.

https://codereview.chromium.org/524153002/diff/140001/pkg/metatest/lib/metate...
pkg/metatest/lib/metatest.dart:60: /// on the source test case. Otherwise, it is
the output of `toString`.
"the output of [toString]" is very unclear. What's [toString] being called on?

If this may be either a native StackTrace or a stack_trace trace, say so.
Alternately, just say that the format isn't guaranteed.

[kevmoo, 2014-09-18 22:40:37]

Committed patchset #11 (id:200001) manually as 40475 (presubmit successful).