Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8418013:
Huge internal clean-up of unittestsuite. (Closed)
|
Created:
9 years, 1 month ago by Bob Nystrom Modified:
9 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionHuge internal clean-up of unittestsuite:
- Get rid of UnitTestSuite class completely.
- Replace bool flags with a state machine.
- Remove some unneeded functions.
- Make some stuff private.
- Clean up generated HTML a bit.
Committed: https://code.google.com/p/dart/source/detail?r=910
Patch Set 1 #
Total comments: 18
Patch Set 2 : Rebase and respond to review. #
Total comments: 2
Patch Set 3 : Rename constant. #Messages
Total messages: 8 (0 generated)
Note: this patch includes the JSON changes from this patch: http://codereview.chromium.org/8392022/ Don't worry about those. I'll submit that patch first and then rebase this one. This depends on those changes, which is why you see them here. All you need to worry about here is unittestsuite.dart.
looks really nice! One thing I'd follow up is how this will affect total/Shauvik DARTest code. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:26: final _stateUninitialized = 0; should we use ALL CAPS for enum-like constants like this? Then I'd also drop the 'state' prefix: _UNINITIALIZED _READY _RUNNING_TEST _UNCAUGHT_ERROR
lgtm This looks strictly better to me than the version before. However, there are still some issues - particularly around window.dynamic that I hope can be cleaned up in subsequent commits. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:26: final _stateUninitialized = 0; I like Siggi's suggestion here. +1 On 2011/10/28 01:33:57, sigmund wrote: > should we use ALL CAPS for enum-like constants like this? > > Then I'd also drop the 'state' prefix: > > _UNINITIALIZED > _READY > _RUNNING_TEST > _UNCAUGHT_ERROR http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:31: * Whether an undetected error occurred while running the last test. This Nit: These http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:43: void expectThrow(function) { Sorrow: I'd like to ask for an optional expected exception type here - but without first class types I think this would be way too ugly to be worth the effort... http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:202: // TODO(jacobr): remove this horrible hack to work around dartc bugs. Is this hack still needed? Both here and below. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:285: window.dynamic.on.contentLoaded.add(listener); Hmm. I really don't like this window.dynamic.foo pattern either here or below. If these are still needed I would complain loudly to Jacob. This seems to occur in many other places in this code - so that same comment for all of them. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:389: bool get isComplete() => success || fail || error; Properties are good. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:423: typedef void TestFunction(); 64 fewer lines with no loss of functionality? Yay!
LGTM http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:12: List<TestCase> _tests; maybe pack all those fields into one class?
Thanks! http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:12: List<TestCase> _tests; On 2011/10/28 15:54:01, antonmuhin wrote: > maybe pack all those fields into one class? I actually specifically unpacked them (they all used to be in UnitTestSuite). I want to try a flatter, less everything-must-be-in-a-class feel for the library to see how it goes. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:26: final _stateUninitialized = 0; On 2011/10/28 15:50:44, jimhug wrote: > I like Siggi's suggestion here. +1 > > On 2011/10/28 01:33:57, sigmund wrote: > > should we use ALL CAPS for enum-like constants like this? > > > > Then I'd also drop the 'state' prefix: > > > > _UNINITIALIZED > > _READY > > _RUNNING_TEST > > _UNCAUGHT_ERROR > Done. We don't have a convention for constants yet, so I wanted to try out nonScreamingCaps to see if I liked it. I think ALL_CAPS looks better too, especially without the STATE_ prefix. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:31: * Whether an undetected error occurred while running the last test. This On 2011/10/28 15:50:44, jimhug wrote: > Nit: These Done. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:43: void expectThrow(function) { On 2011/10/28 15:50:44, jimhug wrote: > Sorrow: I'd like to ask for an optional expected exception type here - but > without first class types I think this would be way too ugly to be worth the > effort... I didn't implement it yet because I didn't have any tests that need it, but my plan is to have expectThrow() return an expectation-like object that you can use to validate the exception itself. You would use it something like: expectThrow(() { throw 'err!'; }).equals('err'); or: expectThrow(() { throw new SomeException(); }).that((e) => e is SomeException); http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:202: // TODO(jacobr): remove this horrible hack to work around dartc bugs. On 2011/10/28 15:50:44, jimhug wrote: > Is this hack still needed? Both here and below. I only investigated briefly, but it looks like it's still needed. Dartc doesn't see that window has an onerror. I don't know if it's a DOM API bug or a Dartc one, but I can investigate further. This code will likely change soon anyway because I'm going to get rid of the current hybrid dart:html/dart:dom support this thing uses. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:285: window.dynamic.on.contentLoaded.add(listener); On 2011/10/28 15:50:44, jimhug wrote: > Hmm. I really don't like this window.dynamic.foo pattern either here or below. > If these are still needed I would complain loudly to Jacob. This seems to occur > in many other places in this code - so that same comment for all of them. I don't like it either. I'll get it cleaned up in a later CL when I clean this up to only use dart:dom. http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:389: bool get isComplete() => success || fail || error; On 2011/10/28 15:50:44, jimhug wrote: > Properties are good. :D
Still LGTM, just trolling :) http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:12: List<TestCase> _tests; On 2011/10/28 18:16:15, Bob Nystrom wrote: > On 2011/10/28 15:54:01, antonmuhin wrote: > > maybe pack all those fields into one class? > > I actually specifically unpacked them (they all used to be in UnitTestSuite). I > want to try a flatter, less everything-must-be-in-a-class feel for the library > to see how it goes. I do understand that. But (YMMV of course), I would still keep a class, w/o any methods, but w/ all the data, just not to search for the stuff around.
lgtm (addressing the last constant below) http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/1/client/testing/unittest/unittes... client/testing/unittest/unittestsuite.dart:285: window.dynamic.on.contentLoaded.add(listener); On 2011/10/28 18:16:15, Bob Nystrom wrote: > On 2011/10/28 15:50:44, jimhug wrote: > > Hmm. I really don't like this window.dynamic.foo pattern either here or > below. > > If these are still needed I would complain loudly to Jacob. This seems to > occur > > in many other places in this code - so that same comment for all of them. > > I don't like it either. I'll get it cleaned up in a later CL when I clean this > up to only use dart:dom. Sounds good - alternatively if we really want to keep 2 implementations, we could factor out this portion of the API that is inconsistent. I personally prefer just having 1 library for now. http://codereview.chromium.org/8418013/diff/5001/client/testing/unittest/unit... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/5001/client/testing/unittest/unit... client/testing/unittest/unittestsuite.dart:35: final _stateUncaughtError = 3; this one too?
http://codereview.chromium.org/8418013/diff/5001/client/testing/unittest/unit... File client/testing/unittest/unittestsuite.dart (right): http://codereview.chromium.org/8418013/diff/5001/client/testing/unittest/unit... client/testing/unittest/unittestsuite.dart:35: final _stateUncaughtError = 3; On 2011/10/28 18:21:42, sigmund wrote: > this one too? Oops! Missed that. Thanks! |
