Retrying the setUp/tearDown chaining change. This is the same as r21819 but with a change to schedu…

Retrying the setUp/tearDown chaining change. This is the same as r21819 but with a change to scheduled_test.dart.

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

[gram, 2013-04-22 23:16:06]

Siggy, this is the same as the last CL except it does have all the context put
into a class so we can reset with a single assignment, as you suggested.

Nathan, please check the changes to scheduled_test.

[nweiz, 2013-04-23 00:04:53]

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_test.dart (left):

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:319: 'still running.');
Why is this being removed? This is important, since async tests can potentially
keep running when you don't expect them to.

[gram, 2013-04-23 00:07:05]

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_test.dart (left):

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:319: 'still running.');
On 2013/04/23 00:04:53, nweiz wrote:
> Why is this being removed? This is important, since async tests can
potentially
> keep running when you don't expect them to.

Because it is completely broken with the chaining of setup calls. You are pretty
much guaranteed to throw if you have groups, because you will have generated a
top level setup that gets called and sets currentSchedule to non-null, and then
a nested setup that gets called and throws.

[nweiz, 2013-04-23 00:20:51]

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_test.dart (left):

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:319: 'still running.');
On 2013/04/23 00:07:05, gram wrote:
> On 2013/04/23 00:04:53, nweiz wrote:
> > Why is this being removed? This is important, since async tests can
> potentially
> > keep running when you don't expect them to.
> 
> Because it is completely broken with the chaining of setup calls. You are
pretty
> much guaranteed to throw if you have groups, because you will have generated a
> top level setup that gets called and sets currentSchedule to non-null, and
then
> a nested setup that gets called and throws.

We should fix that, then, rather than just removing the functionality. We're
already tracking whether we're in a group or not.

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/test/sched...
File pkg/scheduled_test/test/scheduled_test/set_up_test.dart (left):

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test/set_up_test.dart:120: });
These should be replaced by tests that setUp *does* chain properly.

[Siggi Cherem (dart-lang), 2013-04-23 00:55:09]

https://codereview.chromium.org/13973021/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/13973021/diff/1/pkg/pkg.status#newcode35
pkg/pkg.status:35: [ $runtime == d8 || $runtime == jsshell ]
before you had removed the dart2js here, maybe merge both conditions here?

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

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:228: Function parentSetup = null;
I was hoping that here we can use a pointer to the parent GroupContext, rather
than a parentSetup/parentTeardown.

You can still have these implemented as properties. For example:

Function get parentSetup => parent == null ? null : parent.testSetup;


Eventually I was thinking that you could lazily compute this chaining without
having to create new closures for each setup function. For example, by walking
up the parent chain to find each setup function:

Function get setup => testSetup != null ? testSetup : parent.setup;

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:235: String groupName = '';
nit: consider renaming to just 'name'.
also: final?

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:613: // can nest multiple levels; we also have
the global parent variables
I was a bit confused about what you mean here by 'global'?

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:620: oldContext.testTeardown;
With the proposed changes above, this would look more like:

_currentContext = new GroupContext(description, _currentContext);
try {
  ...
} finally {
  _currentContext = _currentContext.parent;
}

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:628: _currentContext.groupName = description;
by making groupName a final, you can do this logic in GroupContext as a derived
property:

  String get fullName =>
    parent == null ? name : ${parent.name}$groupSep$name;

[gram, 2013-04-23 19:42:47]

PTAL

https://codereview.chromium.org/13973021/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/13973021/diff/1/pkg/pkg.status#newcode35
pkg/pkg.status:35: [ $runtime == d8 || $runtime == jsshell ]
On 2013/04/23 00:55:09, Siggi Cherem (dart-lang) wrote:
> before you had removed the dart2js here, maybe merge both conditions here?

I meant to remove the first one, as d8 will imply dart2js so the dart2js part is
redundant. Removed it now.

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_test.dart (left):

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:319: 'still running.');
On 2013/04/23 00:20:52, nweiz wrote:
> On 2013/04/23 00:07:05, gram wrote:
> > On 2013/04/23 00:04:53, nweiz wrote:
> > > Why is this being removed? This is important, since async tests can
> > potentially
> > > keep running when you don't expect them to.
> > 
> > Because it is completely broken with the chaining of setup calls. You are
> pretty
> > much guaranteed to throw if you have groups, because you will have generated
a
> > top level setup that gets called and sets currentSchedule to non-null, and
> then
> > a nested setup that gets called and throws.
> 
> We should fix that, then, rather than just removing the functionality. We're
> already tracking whether we're in a group or not.

Done.

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/test/sched...
File pkg/scheduled_test/test/scheduled_test/set_up_test.dart (left):

https://codereview.chromium.org/13973021/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test/set_up_test.dart:120: });
On 2013/04/23 00:20:52, nweiz wrote:
> These should be replaced by tests that setUp *does* chain properly.

Done.

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

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:235: String groupName = '';
On 2013/04/23 00:55:09, Siggi Cherem (dart-lang) wrote:
> nit: consider renaming to just 'name'.
> also: final?

Done.

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:613: // can nest multiple levels; we also have
the global parent variables
On 2013/04/23 00:55:09, Siggi Cherem (dart-lang) wrote:
> I was a bit confused about what you mean here by 'global'? 

Done.

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:620: oldContext.testTeardown;
On 2013/04/23 00:55:09, Siggi Cherem (dart-lang) wrote:
> With the proposed changes above, this would look more like:
> 
> _currentContext = new GroupContext(description, _currentContext);
> try {
>   ...
> } finally {
>   _currentContext = _currentContext.parent;
> }

Done.

https://codereview.chromium.org/13973021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:628: _currentContext.groupName = description;
On 2013/04/23 00:55:09, Siggi Cherem (dart-lang) wrote:
> by making groupName a final, you can do this logic in GroupContext as a
derived
> property:
> 
>   String get fullName =>
>     parent == null ? name : ${parent.name}$groupSep$name;

Done.

[Siggi Cherem (dart-lang), 2013-04-23 19:52:00]

lgtm!

[nweiz, 2013-04-23 20:09:43]

https://codereview.chromium.org/13973021/diff/8001/pkg/scheduled_test/test/sc...
File pkg/scheduled_test/test/scheduled_test/set_up_test.dart (right):

https://codereview.chromium.org/13973021/diff/8001/pkg/scheduled_test/test/sc...
pkg/scheduled_test/test/scheduled_test/set_up_test.dart:121: 
Also test that if there's setUp in the parent and child, they're both run in the
correct order.

[gram, 2013-04-23 20:43:58]

https://codereview.chromium.org/13973021/diff/8001/pkg/scheduled_test/test/sc...
File pkg/scheduled_test/test/scheduled_test/set_up_test.dart (right):

https://codereview.chromium.org/13973021/diff/8001/pkg/scheduled_test/test/sc...
pkg/scheduled_test/test/scheduled_test/set_up_test.dart:121: 
On 2013/04/23 20:09:43, nweiz wrote:
> Also test that if there's setUp in the parent and child, they're both run in
the
> correct order.

Done.

[nweiz, 2013-04-23 20:50:43]

lgtm

https://codereview.chromium.org/13973021/diff/16001/pkg/scheduled_test/lib/sc...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/13973021/diff/16001/pkg/scheduled_test/lib/sc...
pkg/scheduled_test/lib/scheduled_test.dart:340: if (_setUpFn != null) {
Nit: it would be cleaner if this were "else if".

[gram, 2013-04-23 21:04:37]

https://codereview.chromium.org/13973021/diff/16001/pkg/scheduled_test/lib/sc...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/13973021/diff/16001/pkg/scheduled_test/lib/sc...
pkg/scheduled_test/lib/scheduled_test.dart:340: if (_setUpFn != null) {
On 2013/04/23 20:50:43, nweiz wrote:
> Nit: it would be cleaner if this were "else if".

Done.

[gram, 2013-04-23 21:09:48]

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