Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13464020:
unittest: big cleanup, tightened test semantics (Closed)
|
Created:
7 years, 8 months ago by kevmoo-old Modified:
7 years, 8 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionunittest: big cleanup, tightened test semantics
This ensures that group/test etc are not called when the test system is already running
Committed: https://code.google.com/p/dart/source/detail?r=21707
Patch Set 1 #Patch Set 2 : more better #Patch Set 3 : setup and teardown, too #Patch Set 4 : nits #Patch Set 5 : removed unused private methods while we're at it #
Total comments: 25
Patch Set 6 : fix breaks, remove toplevel _currentTestCaseIndex entirely #
Total comments: 17
Patch Set 7 : sdk updates #Patch Set 8 : using Future correctly #Patch Set 9 : fixed analysis complaints, added docs to _nextBatch #Patch Set 10 : fixed json_test again #Messages
Total messages: 12 (0 generated)
This does some nice things 0) Make it illegal to test(), group(), setup(), tearDown(), filterTests() within a running test block (or any other time once the test system is running) Just asking for crazy behavior. Help folks find correct usage. 1) Centralizes the calculation of the currentTestCase to one location. 2) Minimizes storing/passing around the test case index. Just use the current instance 3) Makes _SpreadArgsHelper strict. If it's used outside of a running test, it throws a descriptive message. Much easier to debug and teach folks proper usage. PTAL
Updates to un-break pub tests. Refactored async run logic to remove top-level _currentTestCaseIndex field. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:577: ensureInitialized(); broke folks who set a custom config after this...which seems a bit broken, but not going to fight that fight now https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:590: ensureInitialized(); broke folks who set a custom config after this...which seems a bit broken, but not going to fight that fight now https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:638: _ensureInitialized(false); broke folks who set a custom config after this...which seems a bit broken, but not going to fight that fight now
These comments are as much for gram as they are for kevmoo. (P.S. everything looks pretty good otherwise.) https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:577: ensureInitialized(); On 2013/04/06 17:56:34, kevmoo wrote: > broke folks who set a custom config after this...which seems a bit broken, but > not going to fight that fight now Whom did you break? If someone is doing something weird, it's worth just patching that code, too. (Perhaps place a TODO here and submit a second CL fixing everyone else's code.) https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:607: if (currentTestCase != null) { How about a private getter: bool get _isCurrentTestCaseValid => _currentTestCaseIndex != null && _currentTestCaseIndex >= 0 && _currentTestCaseIndex < _testCases.length; https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:627: void rerunTests() { Why do we even have this? What makes this different from runTests? If there's a difference, please document it. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:628: assert(_uncaughtErrorMessage == null); Can you make a common "reset()" method somewhere? It seems as if things are set to null/default values in many places, which is just asking for bugs. ensureInitialized, completeTests, and even the top level. These need to all have the same implementation or else bad things may happen. Asserts are quite, good, though! https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:654: assert(_currentTestCaseIndex == null); If this assert is a post-condition on _ensureIntialized, then it should probably go into _ensureIntialized. Otherwise, what is this assert here for? https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:695: void registerException(e, [trace]) { Is there a good reason that this is top-level? https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:771: throw new StateError("A forbidden operation occured " I feel like this error could be useful at other places where you check nullness of the testcase.
Thanks for the notes, Andrei. Please look at the last update. The index has gone away now. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:607: if (currentTestCase != null) { I'm confused. The latest commit eliminated the _currentTestCaseIndex entirely. Explain... https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:627: void rerunTests() { It's legacy. It's used by interactive html configuration. I didn't want to rock the boat more in this CL. More docs: good. Pondering removal: good. Just trying to keep the scope of this CL smaller. It's on my radar. :-) https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:628: assert(_uncaughtErrorMessage == null); Agreed. Things for us to discuss on Tuesday. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:654: assert(_currentTestCaseIndex == null); See latest update. This is now an assert around _currentTestCase == null. _ensureItitilaized actually throws if there is a current test case, so this could be removed. ...just my personal paranoid default https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:695: void registerException(e, [trace]) { It was used by layout unit test random, but that's gone now. Should consider removing it, me thinks. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:771: throw new StateError("A forbidden operation occured " See latest update.
Re-addressing some of Andrei's notes inline with the latest delta https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:573: _requireNotRunning(); Should be a call to ensureInitialized(), but pub tests are doing setUp and then trying to set a custom config. Need to add a TODO here to fix that impl and then make this as strict as test() and group() https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:586: _requireNotRunning(); Should be a call to ensureInitialized(), but pub tests are doing setUp and then trying to set a custom config. Need to add a TODO here to fix that impl and then make this as strict as test() and group() https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:616: assert(_uncaughtErrorMessage == null); Need a TODO about getting rid of this. Perhaps we just need 'run' with the right checks to make sure everything has finished. https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:626: _requireNotRunning(); Should be a call to ensureInitialized(), but pub tests are doing setUp and then trying to set a custom config. Need to add a TODO here to fix that impl and then make this as strict as test() and group() https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:642: assert(_currentTestCase == null); Overly paranoid. _ensureInitialized() throws if this is not the case. Should probably be removed. https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:701: Future _nextBatch([int index = 0]) { FYI: I looked into using Future.forEach here, but it increases the test time for non-async tests by...a lot. This code is inspired by Future.forEach but: 1) uses indexes, since we have a list 2) doesn't spin up unneeded Futures for cases where the test run is synchronous https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:750: void _requireNotRunning() { Once everything has moved to ensureInitialized() ...meaning pub tests have been fixed, then this can be rolled into ensureInit... https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:752: throw new StateError("A forbidden operation occured " I've pondered allowing a string to be passed in here so it's clear which forbidden operation caused this: test(), group() [others in the future]. Thoughts?
I think the unittest library needs some rearchitecting... https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:627: void rerunTests() { On 2013/04/06 23:00:40, kevmoo wrote: > It's legacy. It's used by interactive html configuration. > > I didn't want to rock the boat more in this CL. > More docs: good. > Pondering removal: good. > > Just trying to keep the scope of this CL smaller. It's on my radar. :-) On your radar: good. Add a TODO: better. File a bug to track: best. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:628: assert(_uncaughtErrorMessage == null); On 2013/04/06 23:00:40, kevmoo wrote: > Agreed. Things for us to discuss on Tuesday. I don't think we need to discuss. This is just something that really should be in here. Again, a TODO and file a bug. (Reference bug # in the TODO like: // TODO(kevmoo): Blah blah. Issue #12345) https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:695: void registerException(e, [trace]) { On 2013/04/06 23:00:40, kevmoo wrote: > It was used by layout unit test random, but that's gone now. > > Should consider removing it, me thinks. Consider removing it? Or actually remove it. ;] https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:700: // TODO(kevmoo) we could imagine runTests returning a future... Re-add some doc comments here. Also, this TODO doesn't specify what there is to actually do. https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:701: Future _nextBatch([int index = 0]) { On 2013/04/06 23:10:03, kevmoo wrote: > FYI: I looked into using Future.forEach here, but it increases the test time for > non-async tests by...a lot. > > This code is inspired by Future.forEach but: > > 1) uses indexes, since we have a list > 2) doesn't spin up unneeded Futures for cases where the test run is synchronous If the Future.forEach has poor performance, please file a bug. That's worth fixing. Also, it's worth noting what happens if your sync tests are reaaaaally slow. Perhaps using isolates would speed up performance in all cases. https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:702: for(int i = index; i < _testCases.length; i++) { I'm not entirely clear on what this function is doing, and why it is doing it this way. Firstly, it's not entirely clear what a batch of tests means. Secondly, it's not clear how the program is actually structured. I call nextBatch with an index? But then it can span a future with a call to batch again, but only after the future is done? Where is the future returned by this method actually completed? --- The way unittest.dart is structured makes it very difficult to understand the program flow. What I really want is: 1) initialize test runner 2) for each test, spawn a future that completes to nothing, or error if test failed 3) wait till futures are all done (Future.wait), then display results. I can't really see that from what we have. :[ https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:708: return f.then((_) => _nextBatch(i + 1)); Should probably be whenComplete https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:717: assert(_initialized); Now that I think of it, it's kind of weird to have these asserts. You're presumably running your tests in checked mode, but if unittest.dart itself fails an assert (only in checked mode) then you get ZERO information on your tests. I almost want a way to say that my tests run in checked mode, but the unittest runner does not (but can, if you want it to.) https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:752: throw new StateError("A forbidden operation occured " On 2013/04/06 23:10:03, kevmoo wrote: > I've pondered allowing a string to be passed in here so it's clear which > forbidden operation caused this: test(), group() [others in the future]. > > Thoughts? See my comment about asserts. I sort of want meta-unittest diagnostics to be separate from my tests themselves. That is to say, I would rather some of my tests are marked as invalid if the syntax is wrong, or if the tests were incorrectly written (say if config was run at the wrong time) as opposed to getting a big error at some weird time because of an assert or a state error. I definitely think that unittest.dart is something that's complicated enough and easy to get wrong that we want to make the experience for people writing tests to be as helpful as possible. Instead of throwing errors, I think throwing "UnittestStateException" or something would be useful, and then the running at a higher level can report to the user where they screwed up. --- In short: yes, it should be noted which operation caused the problem.
P.S. Don't worry about the broader changes I suggest here. Just the small stuff. :]
On 2013/04/08 16:48:30, Andrei Mouravski wrote: > P.S. Don't worry about the broader changes I suggest here. Just the small stuff. > :] Any thoughts about the rest of my comments? Feel free to ping me if you want to discuss or have me clarify. P
Addressed many comments. Added docs to _guardAsync Added type to the testCases view to make analyzer happy https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:577: ensureInitialized(); Since I *am* changing the semantics of unittest late in the game, I not trying to fight every fight. I hit most issues in the scheduled_test tests. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:607: if (currentTestCase != null) { _currentTestCaseIndex has been internalized into _nextBatch This cleans up a lot of impl. Code no longer has to key off the index, instead _currentTestCase is 'truth'. Null if a test is not running, otherwise the instance of the running test. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:627: void rerunTests() { It's being used. I've verified the behavior. We can discuss other work if there is an interest. My goal here was to tighten up the logic a bit, be a bit more strict about usage patterns, and level everything else working. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:628: assert(_uncaughtErrorMessage == null); I'm not going to modify the current semantic further, as discussed. The existing behavior works fine for code that uses this method. If we think the asserts are too much, I'm happy to remove. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:695: void registerException(e, [trace]) { scheduled_test uses this https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:700: // TODO(kevmoo) we could imagine runTests returning a future... More doc comments are coming in an update. Should explain the flow. https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:708: return f.then((_) => _nextBatch(i + 1)); guardAsync should never fail. If it returns a future, it should never fail. If it does, whenComplete would swallow the error, or complete with the error, but after running the nested future. Better to fail immediately in this rare case.
lgtm, but make sure you retest after integrating r21702 before submitting. https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/11001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:627: void rerunTests() { On 2013/04/07 19:15:54, Andrei Mouravski wrote: > On 2013/04/06 23:00:40, kevmoo wrote: > > It's legacy. It's used by interactive html configuration. > > > > I didn't want to rock the boat more in this CL. > > More docs: good. > > Pondering removal: good. > > > > Just trying to keep the scope of this CL smaller. It's on my radar. :-) > > On your radar: good. > Add a TODO: better. > File a bug to track: best. We could eliminate this now. Originally there was a distinction IIRC (rerunTests was meant to not clobber an existing test result if the test was disabled) but now it seems superfluous. https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/13464020/diff/16001/pkg/unittest/lib/unittest... pkg/unittest/lib/unittest.dart:702: for(int i = index; i < _testCases.length; i++) { On 2013/04/07 19:15:54, Andrei Mouravski wrote: > I'm not entirely clear on what this function is doing, and why it is doing it > this way. > > Firstly, it's not entirely clear what a batch of tests means. > > Secondly, it's not clear how the program is actually structured. I call > nextBatch with an index? But then it can span a future with a call to batch > again, but only after the future is done? Where is the future returned by this > method actually completed? > > --- > > The way unittest.dart is structured makes it very difficult to understand the > program flow. What I really want is: > 1) initialize test runner > 2) for each test, spawn a future that completes to nothing, or error if test > failed > 3) wait till futures are all done (Future.wait), then display results. > > I can't really see that from what we have. :[ The problem with that is you now have unpredictable ordering of execution. Currently each test must complete before the next starts. There are, I expect, many cases where this is necessary. Running each test in an isolate would help but would probably break some tests. So while I agree that your proposal would be nice, it seems likely that it would break a lot of existing tests. This brings me back to my separate suggestion that it would be better to create a "unit test v2" library rather than radically change this one.
Message was sent while issue was closed.
Committed patchset #10 manually as r21707 (presubmit successful).
Message was sent while issue was closed.
On 2013/04/18 21:34:44, kevmoo wrote: > Committed patchset #10 manually as r21707 (presubmit successful). The dartium builds on the buildbot seem to be broken after this CL. |
