pkg/unittest: cleanup to logMessage

pkg/unittest: cleanup to logMessage

BREAKING
removed Configuration .logMessage
renamed Configuration.logTestCaseMessage to onLogMessage

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

[kevmoo-old, 2013-03-29 02:19:25]

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (left):

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:73: * Can be called by tests to log status.
Tests should use this
Adds no value over the top-level logMessage method plus the logTestCaseMessage
method (which is the only one that should be overridden)

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:122: var success = false;
Not used

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:24: final String name = 'Configuration';
Silly to have const values for getters. final is cleaner IMHO

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:91: for (final t in results) {
Should use the method arg

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/unittest....
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:188: /**
Call the corresponding logTestCaseMessage on the config.

Behavior remains the same with currentTestCase potentially being null

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:793: _config.onSummary(passed, failed, errors,
testCases, _uncaughtErrorMessage);
Tiny nit. Pass onSummary the immutable view of testCases.

[kevmoo-old, 2013-03-29 02:41:43]

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:76: void onLogMessage(TestCase testCase, String
message) {
While we're breaking things, this should look more like a "protected" method.

Designed to be overridden, not called directly.

[Andrei Mouravski, 2013-03-29 04:27:48]

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (left):

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:128: success = true;
Why not instead change the signature and return a boolean for success?

[kevmoo-old, 2013-03-29 13:20:41]

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (left):

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:128: success = true;
Because the caller is the unit test framework. It already knows. :-)

[Andrei Mouravski, 2013-03-29 13:34:09]

lgtm

1 nit

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:24: final String name = 'Configuration';
Also silly to use final instead of const. Also remove type annotations, since
they're inferred. 

On 2013/03/29 02:19:25, kevmoo wrote:
> Silly to have const values for getters. final is cleaner IMHO

[kevmoo-old, 2013-03-29 14:38:36]

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:24: final String name = 'Configuration';
At the moment, I'm playing a game against dart_doc.

name and autoStart are meant to be overridden. const "feels" different. It also
shows up as 'const' in dart_doc.

Likewise with the inferred type. While not needed for optimization, dart_doc
does not show the return type unless it's explicit.

[Andrei Mouravski, 2013-03-29 14:45:39]

Okay. That sounds like a bug in dartdoc, would you file one against me?

[gram, 2013-04-01 17:29:06]

lgtm with a small nit

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/unittest....
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:786: for (TestCase t in _testCases) {
testCases?

[kevmoo-old, 2013-04-01 19:14:36]

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/13261006/diff/2001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:24: final String name = 'Configuration';
Related dart_doc bug: https://code.google.com/p/dart/issues/detail?id=9521

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/unittest....
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/13261006/diff/7001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:786: for (TestCase t in _testCases) {
Same same, right? Readonly access. One less layer of indirection.

[kevmoo-old, 2013-04-01 20:32:55]

Committed patchset #4 manually as r20751 (presubmit successful).