Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12213079:
setUp/tearDown functions can now be asynchronous. (Closed)
|
Created:
7 years, 10 months ago by gram Modified:
7 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionsetUp/tearDown functions can now be asynchronous.
See https://code.google.com/p/dart/issues/detail?id=7670
As part of this change:
- you can now provide an ID in calls to expectAsyncN; if a callback gets called too many times the ID will now be included in the message
- each test case is responsible for kicking off the next test case once it is complete. This is cleaner than the old code which had various tests scattered around that would kick off the next test case.
Committed: https://code.google.com/p/dart/source/detail?r=18360
Patch Set 1 #
Total comments: 18
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #Patch Set 4 : #
Total comments: 15
Patch Set 5 : #
Messages
Total messages: 10 (0 generated)
This is great! First pass. I'm still trying to grok the internal api of unittest, but I think I found some issues to look at now. https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... File pkg/unittest/lib/src/test_case.dart (right): https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:89: .catchError((e) { Make sure you always call catchError() before then() https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:90: _prepTest(); why call _prepTest() when there's an error? https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:100: _prepTest(); Looks like for async setup _prepTest() and test() are called twice. https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:134: .catchError((e) { Make sure you always call catchError() before then() https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dart File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dar... pkg/unittest/lib/unittest.dart:672: // is an asyn teardown (which would have already advanced asyn -> async https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dar... pkg/unittest/lib/unittest.dart:675: _tests[testNum].error("${id}Unexpected extra callbacks", ''); add space between ${id} and Unexpected? https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dar... pkg/unittest/lib/unittest.dart:695: /** Advance to the next test case. */ Maybe explain that the test is deferred? When I saw calls to _nextTestCase() I wondered if they were happening too soon, before an async tearDown() was finished
https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... File pkg/unittest/lib/src/test_case.dart (right): https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:89: .catchError((e) { On 2013/02/08 01:36:48, justinfagnani wrote: > Make sure you always call catchError() before then() Why? https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:90: _prepTest(); On 2013/02/08 01:36:48, justinfagnani wrote: > why call _prepTest() when there's an error? We are going to treat this like a regular test error, so I think it is still a good idea to call config.ontestStart (in the error handling we will call onTestResult with the error result; calling that without having called ontestStart may have unintended consequences depending on the configuration). https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:100: _prepTest(); On 2013/02/08 01:36:48, justinfagnani wrote: > Looks like for async setup _prepTest() and test() are called twice. No, there is a return statement in the block. https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dart File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dar... pkg/unittest/lib/unittest.dart:672: // is an asyn teardown (which would have already advanced On 2013/02/08 01:36:48, justinfagnani wrote: > asyn -> async Done. https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dar... pkg/unittest/lib/unittest.dart:675: _tests[testNum].error("${id}Unexpected extra callbacks", ''); On 2013/02/08 01:36:48, justinfagnani wrote: > add space between ${id} and Unexpected? id already has a space if it is non-empty; this allows me to avoid conditional formatting logic here. https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/unittest.dar... pkg/unittest/lib/unittest.dart:695: /** Advance to the next test case. */ On 2013/02/08 01:36:48, justinfagnani wrote: > Maybe explain that the test is deferred? When I saw calls to _nextTestCase() I > wondered if they were happening too soon, before an async tearDown() was > finished I don't believe _nextTestCase can be called before an async teardown is complete. It certainly isn't supposed to with this CL. Can you explain what made you think that?
https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... File pkg/unittest/lib/src/test_case.dart (right): https://codereview.chromium.org/12213079/diff/1/pkg/unittest/lib/src/test_cas... pkg/unittest/lib/src/test_case.dart:89: .catchError((e) { On 2013/02/08 17:52:24, gram wrote: > On 2013/02/08 01:36:48, justinfagnani wrote: > > Make sure you always call catchError() before then() > > Why? to clarify: as long as the futures are chained, the order doesn't matter anymore (that used to be important with handleException, but not anymore). the only distinction is, what is it that you are trapping. catchError should go before 'then' if you don't want to catch any errors in the body of 'then'. In this case, I think Justin concern is valid: test() will throw 'expect' errors, and those will be caught by 'catchError' here. Is that what we want? https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/src/test_... File pkg/unittest/lib/src/test_case.dart (right): https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/src/test_... pkg/unittest/lib/src/test_case.dart:85: rtn.then(expectAsync1((_) { I thought you were going to use expect async inside setup (let the user handle this correctly). Alternatively, we can get rid of all expectAsync, and instead tell users that they need to use futures for asynchronous test? (if test returns a future, then unittest will wait until it's done?) https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/src/test_... pkg/unittest/lib/src/test_case.dart:86: _prepTest(); style nit: unindent this a bit. The style guide is to use only 2 spaces: rtn.then(expectAsync((_) { _prepTest(); test(); }, id: '...')) .catchError((e) { body... }); https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/src/test_... pkg/unittest/lib/src/test_case.dart:94: // seems to be the more conservative approach. might be worth mentioning the rationale here. Unittest will not stop at a test failure, it will try to run all tests independently of each other. That got me thinking. When a single test fails, it makes sense to continue running the next test, but setup/teardown will be repeated over and over again, is it worth continuing? https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/src/test_... pkg/unittest/lib/src/test_case.dart:103: _nextTestCase(); I'd like to explore is whether we can make TestCase standalone, without knowledge about the unittest library. To make that work, what I'm thinking is that every TestCase would expose a Future. The future completes when the test is done (we will use a normal complete for a passing test, and a completeError for failing tests). Then unittest can simply use the future API to schedule the next test: testcase.run(); testcase.future .then(_testPassed) // mark test as passing .catchError(_testFailed) // mark test as failing .whenComplete(_nextTestCase); // either way, schedule the next test https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/src/test_... pkg/unittest/lib/src/test_case.dart:122: if (result == null) { style nit: only 2 spaces indent https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/unittest.... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/12213079/diff/3001/pkg/unittest/lib/unittest.... pkg/unittest/lib/unittest.dart:674: id != '[Async tearDown completion handler] ') { this feels really strange. By making nextTestCase async (using _defer) doesn't that make tearDown not increment the counter by the time we get here? (so this check is unnecessary)?
https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/src/t... File pkg/unittest/lib/src/test_case.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:89: .catchError((e) { On 2013/02/08 18:02:22, Siggi Cherem (dart-lang) wrote: > On 2013/02/08 17:52:24, gram wrote: > > On 2013/02/08 01:36:48, justinfagnani wrote: > > > Make sure you always call catchError() before then() > > > > Why? > > to clarify: as long as the futures are chained, the order doesn't matter anymore > (that used to be important with handleException, but not anymore). the only > distinction is, what is it that you are trapping. catchError should go before > 'then' if you don't want to catch any errors in the body of 'then'. In this > case, I think Justin concern is valid: test() will throw 'expect' errors, and > those will be caught by 'catchError' here. Is that what we want? Good catch. I was mentally using the old pattern, but your point is important. https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:100: _prepTest(); On 2013/02/08 17:52:24, gram wrote: > On 2013/02/08 01:36:48, justinfagnani wrote: > > Looks like for async setup _prepTest() and test() are called twice. > > No, there is a return statement in the block. Oops https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/unitt... File pkg/unittest/lib/unittest.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:695: /** Advance to the next test case. */ On 2013/02/08 17:52:24, gram wrote: > On 2013/02/08 01:36:48, justinfagnani wrote: > > Maybe explain that the test is deferred? When I saw calls to _nextTestCase() I > > wondered if they were happening too soon, before an async tearDown() was > > finished > > I don't believe _nextTestCase can be called before an async teardown is > complete. It certainly isn't supposed to with this CL. Can you explain what made > you think that? I missed a return statement in test_case. Curious: why _defer() now?
https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/unitt... File pkg/unittest/lib/unittest.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/1/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:695: /** Advance to the next test case. */ On 2013/02/08 18:36:57, justinfagnani wrote: > On 2013/02/08 17:52:24, gram wrote: > > On 2013/02/08 01:36:48, justinfagnani wrote: > > > Maybe explain that the test is deferred? When I saw calls to _nextTestCase() > I > > > wondered if they were happening too soon, before an async tearDown() was > > > finished > > > > I don't believe _nextTestCase can be called before an async teardown is > > complete. It certainly isn't supposed to with this CL. Can you explain what > made > > you think that? > > I missed a return statement in test_case. Curious: why _defer() now? Without _defer, we can build up a very deep stack. _defer gives the stack a chance to unwind.
Changed to have TestCase.run() return a Future, to decouple TestCase from unittest.dart. This may make non-async tests run faster too, as in this case we return null and run the tests in a tight loop, instead of using defer. https://chromiumcodereview.appspot.com/12213079/diff/3001/pkg/unittest/lib/sr... File pkg/unittest/lib/src/test_case.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/3001/pkg/unittest/lib/sr... pkg/unittest/lib/src/test_case.dart:85: rtn.then(expectAsync1((_) { On 2013/02/08 18:02:23, Siggi Cherem (dart-lang) wrote: > I thought you were going to use expect async inside setup (let the user handle > this correctly). > > Alternatively, we can get rid of all expectAsync, and instead tell users that > they need to use futures for asynchronous test? (if test returns a future, then > unittest will wait until it's done? I think using Futures for everything is a great idea, but that should be done separately from this change. https://chromiumcodereview.appspot.com/12213079/diff/3001/pkg/unittest/lib/sr... pkg/unittest/lib/src/test_case.dart:86: _prepTest(); On 2013/02/08 18:02:23, Siggi Cherem (dart-lang) wrote: > style nit: unindent this a bit. The style guide is to use only 2 spaces: > > rtn.then(expectAsync((_) { > _prepTest(); > test(); > }, id: '...')) > .catchError((e) { > body... > }); Done. https://chromiumcodereview.appspot.com/12213079/diff/3001/pkg/unittest/lib/sr... pkg/unittest/lib/src/test_case.dart:122: if (result == null) { On 2013/02/08 18:02:23, Siggi Cherem (dart-lang) wrote: > style nit: only 2 spaces indent Done.
lgtm, just a few minor style comments below https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... File pkg/unittest/lib/src/test_case.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:85: return null; if this always returns null, shouldn't this function be 'void'? https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:97: } fyi - john and I have grown to like the style where you the braces if the condition has no else and fits in a single line, especially for 'fast-exit' code like here. For instance: if (!enabled) return null; (see the exception in this section: http://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-flo...) https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:103: var rtn = _setUp(); nit: consider merging a few of these branches, for instance: var rtn = _setUp == null ? null : _setUp(); if (rtn is Future) { ... } else { _setUp(); } https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:105: rtn.then(expectAsync1((_) { nit: consider using => rtn.then(expectAsync1((_) => _runTest(), id: '....'); https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:130: void _nextTest() { nit: rename, '_markComplete'? https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... File pkg/unittest/lib/unittest.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:674: id != '[Async tearDown completion handler] ') { I'm still curious about why we need this check (see the comment in the previous patch) https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:822: break; nit: now that the tight loop is back, I have a slight prefernce for the old style you had before: while (_currentTest < _test.length) { ... } _completeTests(); https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:828: }, null, _currentTest); is this equivalent to: var f = guardAsync(testCase.run, null, _currentTest); ?
https://chromiumcodereview.appspot.com/12213079/diff/3001/pkg/unittest/lib/un... File pkg/unittest/lib/unittest.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/3001/pkg/unittest/lib/un... pkg/unittest/lib/unittest.dart:674: id != '[Async tearDown completion handler] ') { On 2013/02/08 18:02:23, Siggi Cherem (dart-lang) wrote: > this feels really strange. By making nextTestCase async (using _defer) doesn't > that make tearDown not increment the counter by the time we get here? (so this > check is unnecessary)? No, because we can still put a passed test into an error state if there are late extraneous callbacks. https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... File pkg/unittest/lib/src/test_case.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:85: return null; On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > if this always returns null, shouldn't this function be 'void'? Yes, my bad, it had some other code in before that got moved to run(). Fixed. https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:97: } On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > fyi - john and I have grown to like the style where you the braces if the > condition has no else and fits in a single line, especially for 'fast-exit' code > like here. > > For instance: > > if (!enabled) return null; > > (see the exception in this section: > http://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-flo...) Done. https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:105: rtn.then(expectAsync1((_) { On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > nit: consider using => > > rtn.then(expectAsync1((_) => _runTest(), > id: '....'); Done. https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/src/t... pkg/unittest/lib/src/test_case.dart:130: void _nextTest() { On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > nit: rename, '_markComplete'? I went for _notifyComplete https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... File pkg/unittest/lib/unittest.dart (right): https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:674: id != '[Async tearDown completion handler] ') { On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > I'm still curious about why we need this check (see the comment in the previous > patch) Actually I think the expectAsync was spurious, so we don't need this, but for different reasons :-) https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:822: break; On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > nit: now that the tight loop is back, I have a slight prefernce for the old > style you had before: > > while (_currentTest < _test.length) { > ... > } > _completeTests(); That won't work, as there is an async loop break too where completeTests shouldn't be called. https://chromiumcodereview.appspot.com/12213079/diff/6/pkg/unittest/lib/unitt... pkg/unittest/lib/unittest.dart:828: }, null, _currentTest); On 2013/02/11 23:14:18, Siggi Cherem (dart-lang) wrote: > is this equivalent to: > var f = guardAsync(testCase.run, null, _currentTest); > ? Yes! :-)
lgtm! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
