Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
709903004:
Enhance the Dart unittest library to support multiple invocations in the same application run. (Closed)
|
Created:
6 years, 1 month ago by wibling Modified:
6 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionEnhance the Dart unittest library to support multiple invocations in the same application run.
I am creating e2e tests for dart on App Engine and want to avoid redeploying the app each time I need to run a specific set of tests. Instead I have enhanced the unittest library with a method which creates a new "global" test environment which can be used for this specific run of tests.
Before I had to redeploy the app each time (takes ~2-4mins) to get a fresh test environment.
BUG=
R=kevmoo@google.com, kustermann@google.com
Committed: https://code.google.com/p/dart/source/detail?r=41741
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : review feedback #
Total comments: 2
Patch Set 4 : make TestEnvironment private #
Total comments: 4
Patch Set 5 : review feedback #
Total comments: 12
Patch Set 6 : more review feedback #
Total comments: 53
Patch Set 7 : nweiz feedback #Patch Set 8 : #
Total comments: 7
Messages
Total messages: 26 (1 generated)
wibling@google.com changed reviewers: + kevmoo@google.com, kustermann@google.com, nweiz@google.com, sgjesse@chromium.org
https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:158: TestEnvironment _defaultEnvironment = new TestEnvironment(); Make this final. https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:217: (_environment.currentTestCaseIndex >= 0 && _environment.currentTestCaseIndex < testCases.length) long line https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:478: _config.onSummary(passed, failed, errors, testCases, _environment.uncaughtErrorMessage); long line https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:561: print("Creating new test env!"); left-over print? https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:564: onError: (error, stack) => print("$error\nStack:\n$stack")); Maybe there is a better way then printing the errors here? Could be reported as an error for running tests (there is a distinction between errors and failures I think)
You should also add some kind of test to pkg/unittest/test for this.
On 2014/11/10 11:38:06, kustermann wrote: > You should also add some kind of test to pkg/unittest/test for this. Yes, please add tests for this.
On 2014/11/10 11:38:06, kustermann wrote: > You should also add some kind of test to pkg/unittest/test for this. Will do.
Thanks for the reviews! I have incorporated the feedback and added a test. https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:158: TestEnvironment _defaultEnvironment = new TestEnvironment(); On 2014/11/10 11:37:37, kustermann wrote: > Make this final. Done. https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:217: (_environment.currentTestCaseIndex >= 0 && _environment.currentTestCaseIndex < testCases.length) On 2014/11/10 11:37:37, kustermann wrote: > long line Done. https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:478: _config.onSummary(passed, failed, errors, testCases, _environment.uncaughtErrorMessage); On 2014/11/10 11:37:37, kustermann wrote: > long line Done. https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:561: print("Creating new test env!"); On 2014/11/10 11:37:37, kustermann wrote: > left-over print? Indeed:) Removed. https://codereview.chromium.org/709903004/diff/20001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:564: onError: (error, stack) => print("$error\nStack:\n$stack")); On 2014/11/10 11:37:37, kustermann wrote: > Maybe there is a better way then printing the errors here? Could be reported as > an error for running tests (there is a distinction between errors and failures I > think) I removed the onError handler and used the default instead.
LGTM with comments https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... pkg/unittest/lib/src/test_environment.dart:11: class TestEnvironment { Does this class need to be public? Is it exposed anywhere? Either make it private or make it a library that is imported into unittest.dart, but not exported
New diff with private TestEnvironment. https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... pkg/unittest/lib/src/test_environment.dart:11: class TestEnvironment { On 2014/11/11 15:09:16, kevmoo wrote: > Does this class need to be public? Is it exposed anywhere? > > Either make it private or make it a library that is imported into unittest.dart, > but not exported Good point. I have made it private.
On 2014/11/11 15:16:35, wibling wrote: > New diff with private TestEnvironment. > > https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... > File pkg/unittest/lib/src/test_environment.dart (right): > > https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... > pkg/unittest/lib/src/test_environment.dart:11: class TestEnvironment { > On 2014/11/11 15:09:16, kevmoo wrote: > > Does this class need to be public? Is it exposed anywhere? > > > > Either make it private or make it a library that is imported into > unittest.dart, > > but not exported > > Good point. I have made it private. One last paranoid check: have you run all pkg tests? Including scheduled_test? Just for sanity...
On 2014/11/11 15:42:17, kevmoo wrote: > On 2014/11/11 15:16:35, wibling wrote: > > New diff with private TestEnvironment. > > > > > https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... > > File pkg/unittest/lib/src/test_environment.dart (right): > > > > > https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... > > pkg/unittest/lib/src/test_environment.dart:11: class TestEnvironment { > > On 2014/11/11 15:09:16, kevmoo wrote: > > > Does this class need to be public? Is it exposed anywhere? > > > > > > Either make it private or make it a library that is imported into > > unittest.dart, > > > but not exported > > > > Good point. I have made it private. > > One last paranoid check: have you run all pkg tests? Including scheduled_test? > > Just for sanity... Nope, how do I run them?
On 2014/11/11 16:16:39, wibling wrote: > On 2014/11/11 15:42:17, kevmoo wrote: > > On 2014/11/11 15:16:35, wibling wrote: > > > New diff with private TestEnvironment. > > > > > > > > > https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... > > > File pkg/unittest/lib/src/test_environment.dart (right): > > > > > > > > > https://codereview.chromium.org/709903004/diff/40001/pkg/unittest/lib/src/tes... > > > pkg/unittest/lib/src/test_environment.dart:11: class TestEnvironment { > > > On 2014/11/11 15:09:16, kevmoo wrote: > > > > Does this class need to be public? Is it exposed anywhere? > > > > > > > > Either make it private or make it a library that is imported into > > > unittest.dart, > > > > but not exported > > > > > > Good point. I have made it private. > > > > One last paranoid check: have you run all pkg tests? Including scheduled_test? > > > > Just for sanity... > > Nope, how do I run them? Just ran the specific tests. Seems to pass. wibling@wibling-desktop /work/bleeding_edge/dart (unittest) $ ./tools/test.py -mrelease pkg/unittest Test configuration: none_vm_release_ia32 [00:02 | 100% | + 29 | - 0] wibling@wibling-desktop /work/bleeding_edge/dart (unittest) $ ./tools/test.py -mrelease pkg/scheduled_test Test configuration: none_vm_release_ia32 [00:06 | 100% | + 30 | - 0]
LGTM -- great change to fix unittest, getting rid of the global state :) Please also update Changelog.md and increase minor version in pubspec.yaml so we can release it. https://codereview.chromium.org/709903004/diff/60001/pkg/unittest/test/with_t... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/60001/pkg/unittest/test/with_t... pkg/unittest/test/with_test_environment_test.dart:16: return config.done; You never use the returned future as far as I can tell. Maybe just get rid of this (+ the UnittestConfiguration) ? https://codereview.chromium.org/709903004/diff/60001/pkg/unittest/test/with_t... pkg/unittest/test/with_test_environment_test.dart:59: withTestEnvironment(() => runUnittests(runTests1)); You could add a nested test as well withTestEnvironment(() { runUnittests(runTests); withTestEnvironment(() { runUnittests(runTests); }); });
Thanks for the review! Updated as suggested. https://codereview.chromium.org/709903004/diff/60001/pkg/unittest/test/with_t... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/60001/pkg/unittest/test/with_t... pkg/unittest/test/with_test_environment_test.dart:16: return config.done; On 2014/11/11 16:46:24, kustermann wrote: > You never use the returned future as far as I can tell. Maybe just get rid of > this (+ the UnittestConfiguration) ? Done. https://codereview.chromium.org/709903004/diff/60001/pkg/unittest/test/with_t... pkg/unittest/test/with_test_environment_test.dart:59: withTestEnvironment(() => runUnittests(runTests1)); On 2014/11/11 16:46:24, kustermann wrote: > You could add a nested test as well > > withTestEnvironment(() { > runUnittests(runTests); > withTestEnvironment(() { > runUnittests(runTests); > }); > }); Done.
https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/CHANGELOG.md File pkg/unittest/CHANGELOG.md (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/CHANGELOG.m... pkg/unittest/CHANGELOG.md:1: ##0.11.0+7 Please make this 0.11.1 – we are adding new features. https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:173: void set _config(var config) { _environment.config = config; } var -> Configuration https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:209: List<TestCase> get testCases => new UnmodifiableListView<TestCase>(_testCases); Put a read-only property on _TestEnvironment - maybe .testCasesRO or similar - that wraps the instance list as this code used to. So folks who access testCases are no constantly creating new collections https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:561: /// application instance. Include more details here. When would this be used? https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/pubspec.yaml File pkg/unittest/pubspec.yaml (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/pubspec.yam... pkg/unittest/pubspec.yaml:2: version: 0.11.0+7 0.11.1 - new features https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/test/with_t... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/test/with_t... pkg/unittest/test/with_test_environment_test.dart:9: void runUnittests(Function callback) { Please use follow the convention of other unittest tests use metatest test failure conditions, etc.
Thanks for the review! I have updated the diff accordingly. @nweiz, do you have any comments? https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/CHANGELOG.md File pkg/unittest/CHANGELOG.md (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/CHANGELOG.m... pkg/unittest/CHANGELOG.md:1: ##0.11.0+7 On 2014/11/11 21:27:53, kevmoo wrote: > Please make this 0.11.1 – we are adding new features. Done. https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:173: void set _config(var config) { _environment.config = config; } On 2014/11/11 21:27:53, kevmoo wrote: > var -> Configuration Done. https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:209: List<TestCase> get testCases => new UnmodifiableListView<TestCase>(_testCases); On 2014/11/11 21:27:53, kevmoo wrote: > Put a read-only property on _TestEnvironment - maybe .testCasesRO or similar - > that wraps the instance list as this code used to. > > So folks who access testCases are no constantly creating new collections Done. https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/lib/unittes... pkg/unittest/lib/unittest.dart:561: /// application instance. On 2014/11/11 21:27:53, kevmoo wrote: > Include more details here. > > When would this be used? Done. https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/pubspec.yaml File pkg/unittest/pubspec.yaml (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/pubspec.yam... pkg/unittest/pubspec.yaml:2: version: 0.11.0+7 On 2014/11/11 21:27:53, kevmoo wrote: > 0.11.1 - new features Done. https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/test/with_t... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/80001/pkg/unittest/test/with_t... pkg/unittest/test/with_test_environment_test.dart:9: void runUnittests(Function callback) { On 2014/11/11 21:27:53, kevmoo wrote: > Please use follow the convention of other unittest tests > > use metatest > > test failure conditions, etc. I am not sure how I should do that? I am currently testing that I can setup a new test environment and run only the tests specified in this environment. I am not really interested in whether the tests pass or not, but rather that I can setup and run tests in isolation when using this new method. Do you have any suggestions?
There are some style changes, but this is your first CL so you don't need to worry about those. Can you just make the pubspec version 0.11.1-dev to give me a chance to clean them up?
On 2014/11/12 20:37:12, nweiz wrote: > There are some style changes, but this is your first CL so you don't need to > worry about those. Can you just make the pubspec version 0.11.1-dev to give me a > chance to clean them up? Style changes will not impact the external interface of package:unittest and not the semantics of the implementation. They are therefore not a blocker for getting this change released. You can do style changes for the next release if you insist on doing so. Besides: Since Gustav is new to Dart, why not help him learn/improve his dart skills / tell him what your style preferences are and comment on this CL?
On 2014/11/12 20:57:59, kustermann wrote: > On 2014/11/12 20:37:12, nweiz wrote: > > There are some style changes, but this is your first CL so you don't need to > > worry about those. Can you just make the pubspec version 0.11.1-dev to give me > a > > chance to clean them up? > > Style changes will not impact the external interface of package:unittest and not > the semantics of the implementation. > They are therefore not a blocker for getting this change released. > > You can do style changes for the next release if you insist on doing so. There's no harm in waiting a couple hours to release this. > Besides: Since Gustav is new to Dart, why not help him learn/improve his dart > skills / tell him what your style preferences are and comment on this CL? We want to make the process of submitting patches as painless as possible, which means (among other things) limiting the amount of cleanup work we ask users to do. If he wants I'm happy to explain the context of the changes to him, but it shouldn't block getting his change landed.
On 2014/11/12 21:07:10, nweiz wrote: > On 2014/11/12 20:57:59, kustermann wrote: > > On 2014/11/12 20:37:12, nweiz wrote: > > > There are some style changes, but this is your first CL so you don't need to > > > worry about those. Can you just make the pubspec version 0.11.1-dev to give > me > > a > > > chance to clean them up? > > > > Style changes will not impact the external interface of package:unittest and > not > > the semantics of the implementation. > > They are therefore not a blocker for getting this change released. > > > > You can do style changes for the next release if you insist on doing so. > > There's no harm in waiting a couple hours to release this. > > > Besides: Since Gustav is new to Dart, why not help him learn/improve his dart > > skills / tell him what your style preferences are and comment on this CL? > > We want to make the process of submitting patches as painless as possible, which > means (among other things) limiting the amount of cleanup work we ask users to > do. If he wants I'm happy to explain the context of the changes to him, but it > shouldn't block getting his change landed. Hi Natalie, I am happy to do style changes, just let me know what they are and I will apply them before committing. I might as well learn the "right" way:) /gustav
Nitpicks incoming... https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:7: /// Class for encapsulating test environment state. The first sentence of a documentation comment should be its own paragraph, here and elsewhere. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:8: /// This is used by the withTestEnvironment method to support multiple withTestEnvironment -> [withTestEnironment]. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:9: /// invocations of the unittest library with the same application with -> within https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:12: Configuration config = null; "= null" is unnecessary here and elsewhere. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:16: // support top-level setUp/tearDown functions as well. setUp -> [setUp], tearDown -> [tearDown]. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:20: /// Represents the index of the currently running test case Use complete sentences here, including periods at the end. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:26: /// Whether the framework is in an initialized state. "is in an initialized state" -> "has been initialized". https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:29: /// Time since we last gave non-sync code a chance to be scheduled. "non-sync" -> "asynchronous" https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:34: /// As groups can be nested we use a counter to keep track of the nest level "nest level" -> "nesting level" https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:39: final List<TestCase> testCases; Document this. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:40: final UnmodifiableListView<TestCase> testCasesRO; We don't usually use suffixes like "RO". In general, we also dynamically create unmodifiable views rather than storing a single instance. I'd change this back to wrapping [testCases] in unittest.dart. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:42: String uncaughtErrorMessage = null; Document this. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:46: _TestEnvironment._(List<TestCase> testCases) These two constructors are confusing; what's the point of having a constructor named "_" when the class itself is private anyway? I'd just make a single constructor that takes an optional List argument. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:156: const Symbol _TEST_ENVIRONMENT = #test_environment; I don't think it's worth making a constant when the constant name doesn't provide any more information than the raw symbol name. Also, if you're using the zone environment, it's worth being very careful about namespacing your constants. A good convention is to always start with the package name, so here it would be #unittest.environment. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:166: return _defaultEnvironment; We usually use bracketless ifs if they fit on a single line. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:173: void set _config(Configuration config) { _environment.config = config; } We don't usually do single-line methods without =>. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:313: _environment.currentContext = new _GroupContext(_environment.currentContext, description); Long line. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:566: /// a given set of test. Explain that the new environment is zone-scoped. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:569: zoneValues: <Symbol, Object>{_TEST_ENVIRONMENT: new _TestEnvironment()}); We don't type generic objects within method bodies, especially for literals. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:24: void main() { Why doesn't this use metatest to isolate its test cases? https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:32: } catch(error) { Add a space after "catch". Also, if you write "on StateError catch (error)" you can avoid the is check below. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:34: throw error; If you're using a bracketless if, you must put the body on the same line.
Thanks for the detailed review. It is useful to know the general guidelines we have! I have applied your feedback in most cases see more details below. Most of the comments were copied from elsewhere so there might still be additional work to do in the unittest module in general to fix other comments' style to match. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:7: /// Class for encapsulating test environment state. On 2014/11/13 19:34:27, nweiz wrote: > The first sentence of a documentation comment should be its own paragraph, here > and elsewhere. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:8: /// This is used by the withTestEnvironment method to support multiple On 2014/11/13 19:34:27, nweiz wrote: > withTestEnvironment -> [withTestEnironment]. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:9: /// invocations of the unittest library with the same application On 2014/11/13 19:34:27, nweiz wrote: > with -> within Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:9: /// invocations of the unittest library with the same application On 2014/11/13 19:34:27, nweiz wrote: > with -> within Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:12: Configuration config = null; On 2014/11/13 19:34:27, nweiz wrote: > "= null" is unnecessary here and elsewhere. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:12: Configuration config = null; On 2014/11/13 19:34:27, nweiz wrote: > "= null" is unnecessary here and elsewhere. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:16: // support top-level setUp/tearDown functions as well. On 2014/11/13 19:34:26, nweiz wrote: > setUp -> [setUp], tearDown -> [tearDown]. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:20: /// Represents the index of the currently running test case On 2014/11/13 19:34:26, nweiz wrote: > Use complete sentences here, including periods at the end. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:26: /// Whether the framework is in an initialized state. On 2014/11/13 19:34:27, nweiz wrote: > "is in an initialized state" -> "has been initialized". Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:29: /// Time since we last gave non-sync code a chance to be scheduled. On 2014/11/13 19:34:27, nweiz wrote: > "non-sync" -> "asynchronous" Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:34: /// As groups can be nested we use a counter to keep track of the nest level On 2014/11/13 19:34:27, nweiz wrote: > "nest level" -> "nesting level" Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:39: final List<TestCase> testCases; On 2014/11/13 19:34:27, nweiz wrote: > Document this. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:40: final UnmodifiableListView<TestCase> testCasesRO; On 2014/11/13 19:34:27, nweiz wrote: > We don't usually use suffixes like "RO". In general, we also dynamically create > unmodifiable views rather than storing a single instance. I'd change this back > to wrapping [testCases] in unittest.dart. Okay, I will change it back to my previous version. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:42: String uncaughtErrorMessage = null; On 2014/11/13 19:34:27, nweiz wrote: > Document this. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:46: _TestEnvironment._(List<TestCase> testCases) On 2014/11/13 19:34:27, nweiz wrote: > These two constructors are confusing; what's the point of having a constructor > named "_" when the class itself is private anyway? I'd just make a single > constructor that takes an optional List argument. The list of tests is not optional. The reason for having the deferring constructor is to enable setting both the testCases and the testCasesRO field as final. Since I am moving the testCasesRO back to unittest.dart I can revert it back to my previous version. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:156: const Symbol _TEST_ENVIRONMENT = #test_environment; On 2014/11/13 19:34:28, nweiz wrote: > I don't think it's worth making a constant when the constant name doesn't > provide any more information than the raw symbol name. > > Also, if you're using the zone environment, it's worth being very careful about > namespacing your constants. A good convention is to always start with the > package name, so here it would be #unittest.environment. I prefer keeping the constant since it is used both when looking up the environment and when setting it further down in the withTestEnvironment method. It avoids someone accidentally changing it in one place and not another. I have renamed it to _UNITTEST_ENVIRONMENT=#unittest.environment https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:166: return _defaultEnvironment; On 2014/11/13 19:34:27, nweiz wrote: > We usually use bracketless ifs if they fit on a single line. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:173: void set _config(Configuration config) { _environment.config = config; } On 2014/11/13 19:34:27, nweiz wrote: > We don't usually do single-line methods without =>. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:313: _environment.currentContext = new _GroupContext(_environment.currentContext, description); On 2014/11/13 19:34:27, nweiz wrote: > Long line. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:566: /// a given set of test. On 2014/11/13 19:34:28, nweiz wrote: > Explain that the new environment is zone-scoped. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:569: zoneValues: <Symbol, Object>{_TEST_ENVIRONMENT: new _TestEnvironment()}); On 2014/11/13 19:34:27, nweiz wrote: > We don't type generic objects within method bodies, especially for literals. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:24: void main() { On 2014/11/13 19:34:28, nweiz wrote: > Why doesn't this use metatest to isolate its test cases? I am not sure what the benefit of metatests would be in this case? As I mentioned in my reply to Kevin please let me know how this should be tested using meta tests and I will be happy to change it. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:32: } catch(error) { On 2014/11/13 19:34:28, nweiz wrote: > Add a space after "catch". > > Also, if you write "on StateError catch (error)" you can avoid the is check > below. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:34: throw error; On 2014/11/13 19:34:28, nweiz wrote: > If you're using a bracketless if, you must put the body on the same line. Done.
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 41741 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:156: const Symbol _TEST_ENVIRONMENT = #test_environment; On 2014/11/14 10:09:49, wibling wrote: > On 2014/11/13 19:34:28, nweiz wrote: > > I don't think it's worth making a constant when the constant name doesn't > > provide any more information than the raw symbol name. > > > > Also, if you're using the zone environment, it's worth being very careful > about > > namespacing your constants. A good convention is to always start with the > > package name, so here it would be #unittest.environment. > > I prefer keeping the constant since it is used both when looking up the > environment and when setting it further down in the withTestEnvironment method. > It avoids someone accidentally changing it in one place and not another. Tests are for ensuring that changes don't break functionality; constants are for documentation and readability. > I have renamed it to _UNITTEST_ENVIRONMENT=#unittest.environment https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:166: return _defaultEnvironment; On 2014/11/14 10:09:49, wibling wrote: > On 2014/11/13 19:34:27, nweiz wrote: > > We usually use bracketless ifs if they fit on a single line. > > Done. Move the return to the previous line. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:24: void main() { On 2014/11/14 10:09:49, wibling wrote: > On 2014/11/13 19:34:28, nweiz wrote: > > Why doesn't this use metatest to isolate its test cases? > > I am not sure what the benefit of metatests would be in this case? metatest allows you to create isolated test run with the normal unittest infrasturcture when testing unittest code. Having a file that just throws an error when it fails rather than printing useful information isn't friendly to future people working on the code. > As I mentioned in my reply to Kevin please let me know how this should be tested > using meta tests and I will be happy to change it. You can look at other unittest test files as examples. Basically, within each [expectTestResults] you put code that would otherwise be run at the top level of the test and that code is run in isolation. https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:26: /// test has completed. Don't start sentences with "=="; use something like "If this is -1, that means the test system isn't running." https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:45: final List<TestCase> testCases = new List<TestCase>(); There's no need to type annotate this, since the type is clear from the initializing expression. https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/unitte... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:213: final List<TestCase> testCases = This should be a getter, not a field. As a field it won't change if [_environment.testCases] changes.
Message was sent while issue was closed.
Thanks for the additional feedback. I have replied inline below and send out a new diff with the changes at: https://codereview.chromium.org/734343002/ https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:156: const Symbol _TEST_ENVIRONMENT = #test_environment; On 2014/11/17 23:02:54, nweiz wrote: > On 2014/11/14 10:09:49, wibling wrote: > > On 2014/11/13 19:34:28, nweiz wrote: > > > I don't think it's worth making a constant when the constant name doesn't > > > provide any more information than the raw symbol name. > > > > > > Also, if you're using the zone environment, it's worth being very careful > > about > > > namespacing your constants. A good convention is to always start with the > > > package name, so here it would be #unittest.environment. > > > > I prefer keeping the constant since it is used both when looking up the > > environment and when setting it further down in the withTestEnvironment > method. > > It avoids someone accidentally changing it in one place and not another. > > Tests are for ensuring that changes don't break functionality; constants are for > documentation and readability. I agree that tests are for testing functionality and making sure the code is not broken. However keeping code as easy to maintain as possible is IMO also a good thing. Having a constant in this case is IMO not just for documentation, but also provides more maintainability. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:166: return _defaultEnvironment; On 2014/11/17 23:02:54, nweiz wrote: > On 2014/11/14 10:09:49, wibling wrote: > > On 2014/11/13 19:34:27, nweiz wrote: > > > We usually use bracketless ifs if they fit on a single line. > > > > Done. > > Move the return to the previous line. Done. https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:24: void main() { On 2014/11/17 23:02:54, nweiz wrote: > On 2014/11/14 10:09:49, wibling wrote: > > On 2014/11/13 19:34:28, nweiz wrote: > > > Why doesn't this use metatest to isolate its test cases? > > > > I am not sure what the benefit of metatests would be in this case? > > metatest allows you to create isolated test run with the normal unittest > infrasturcture when testing unittest code. Having a file that just throws an > error when it fails rather than printing useful information isn't friendly to > future people working on the code. > > > As I mentioned in my reply to Kevin please let me know how this should be > tested > > using meta tests and I will be happy to change it. > > You can look at other unittest test files as examples. Basically, within each > [expectTestResults] you put code that would otherwise be run at the top level of > the test and that code is run in isolation. I did look at the other tests and also at the metatest code. The problem as I see it is that the metatest code is tailored to determine if the tests being run are finishing with the expected results whereas this change is not about the results of the specific tests, but whether the setup of the unittest library succeeds or not. Given that I am not validating the test results I don't understand the value of running this in a metatest? https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:26: /// test has completed. On 2014/11/17 23:02:55, nweiz wrote: > Don't start sentences with "=="; use something like "If this is -1, that means > the test system isn't running." Sure. I have fixed it. https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:45: final List<TestCase> testCases = new List<TestCase>(); On 2014/11/17 23:02:55, nweiz wrote: > There's no need to type annotate this, since the type is clear from the > initializing expression. I am getting mixed feedback here. As far as I have heard and also my personal preference we should type annotate fields that are public. Even in a private class. https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/unitte... File pkg/unittest/lib/unittest.dart (right): https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/unitte... pkg/unittest/lib/unittest.dart:213: final List<TestCase> testCases = On 2014/11/17 23:02:55, nweiz wrote: > This should be a getter, not a field. As a field it won't change if > [_environment.testCases] changes. Thanks for catching this. I missed that when changing it back.
Message was sent while issue was closed.
https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... File pkg/unittest/test/with_test_environment_test.dart (right): https://codereview.chromium.org/709903004/diff/100001/pkg/unittest/test/with_... pkg/unittest/test/with_test_environment_test.dart:24: void main() { On 2014/11/18 10:16:49, wibling wrote: > On 2014/11/17 23:02:54, nweiz wrote: > > On 2014/11/14 10:09:49, wibling wrote: > > > On 2014/11/13 19:34:28, nweiz wrote: > > > > Why doesn't this use metatest to isolate its test cases? > > > > > > I am not sure what the benefit of metatests would be in this case? > > > > metatest allows you to create isolated test run with the normal unittest > > infrasturcture when testing unittest code. Having a file that just throws an > > error when it fails rather than printing useful information isn't friendly to > > future people working on the code. > > > > > As I mentioned in my reply to Kevin please let me know how this should be > > tested > > > using meta tests and I will be happy to change it. > > > > You can look at other unittest test files as examples. Basically, within each > > [expectTestResults] you put code that would otherwise be run at the top level > of > > the test and that code is run in isolation. > > I did look at the other tests and also at the metatest code. The problem as I > see it is that the metatest code is tailored to determine if the tests being run > are finishing with the expected results whereas this change is not about the > results of the specific tests, but whether the setup of the unittest library > succeeds or not. > Given that I am not validating the test results I don't understand the value of > running this in a metatest? Oh, I see, that's a good point. It's probably possible to add functionality to metatest that would support this, but that's not necessary to do now. https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... File pkg/unittest/lib/src/test_environment.dart (right): https://codereview.chromium.org/709903004/diff/140001/pkg/unittest/lib/src/te... pkg/unittest/lib/src/test_environment.dart:45: final List<TestCase> testCases = new List<TestCase>(); On 2014/11/18 10:16:49, wibling wrote: > On 2014/11/17 23:02:55, nweiz wrote: > > There's no need to type annotate this, since the type is clear from the > > initializing expression. > > I am getting mixed feedback here. As far as I have heard and also my personal > preference we should type annotate fields that are public. Even in a private > class. The code in the repo isn't entirely consistent, but across most of the widely-used Dart-team-authored packages the convention is to include types on *most* instance variables but to omit them when there's an initializer that makes the type clear. Basically, the minimum amount of verbosity should be used that still makes the type clear from the declaration. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
