pkg/unittest: locking down TestCase

pkg/unittest: locking down TestCase

Strip out most of the public API, leaving only read-only properties that might
be interesting for users of unittest.

This class is not designed to be instantiated or reused outside of unitttest.

The only constructor for TestCase makes strict assumptions about the unittest
environment, specifically _currentGroup, _testSetup, _testTeardown.

This enables maximum flexibility to evolve and improve the unittest library
without concern for supporting random use cases invented by 3rd-parties trying to be clever

Before: http://cl.ly/image/41230W032h1d
After: http://cl.ly/image/0K0E2S3g1327

All unittest via ./tools/bots/pub.py -n pub-mac pass

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

[gram, 2013-03-05 18:07:41]

lgtm except for my concern in the comments. We need to make sure that
pass/fail/error are not used anywhere else; that includes the DOM layout tests
so I'm adding Vijay.

https://codereview.chromium.org/12452004/diff/2001/pkg/unittest/lib/src/test_...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/12452004/diff/2001/pkg/unittest/lib/src/test_...
pkg/unittest/lib/src/test_case.dart:196: if (result != null) {
I know that somewhere I have seen code like currentTestCase.fail(...). I'm not
sure if that was in one of the configs or if it was a test case, but I think it
may have been the latter. We will need to verify that this is not an issue
before this change.

[gram, 2013-03-05 23:25:53]

On 2013/03/05 18:07:41, gram wrote:
> lgtm except for my concern in the comments. We need to make sure that
> pass/fail/error are not used anywhere else; that includes the DOM layout tests
> so I'm adding Vijay.
> 
>
https://codereview.chromium.org/12452004/diff/2001/pkg/unittest/lib/src/test_...
> File pkg/unittest/lib/src/test_case.dart (right):
> 
>
https://codereview.chromium.org/12452004/diff/2001/pkg/unittest/lib/src/test_...
> pkg/unittest/lib/src/test_case.dart:196: if (result != null) {
> I know that somewhere I have seen code like currentTestCase.fail(...). I'm not
> sure if that was in one of the configs or if it was a test case, but I think
it
> may have been the latter. We will need to verify that this is not an issue
> before this change.

We do make use of error() in the configs, and fail() in the tests. So I'm going
to keep those public.

[kevmoo-old, 2013-03-05 23:28:25]

> We do make use of error() in the configs, and fail() in the tests. So I'm
going
> to keep those public.

To get this commit moving, not a problem.

2 questions:

1) Would you like me to do that clean-up? Which ever is faster/easier for you.
2) can you point me to examples? I'd love to see if these could/should be
refactored to use top-level features in unittest.

[gram, 2013-03-05 23:42:04]

On 2013/03/05 23:28:25, kevmoo wrote:
> > We do make use of error() in the configs, and fail() in the tests. So I'm
> going
> > to keep those public.
> 
> To get this commit moving, not a problem.
> 
> 2 questions:
> 
> 1) Would you like me to do that clean-up? Which ever is faster/easier for you.
> 2) can you point me to examples? I'd love to see if these could/should be
> refactored to use top-level features in unittest.

fail() is used in unittest_test.dart. error() is used in some of the
xx_config.dart test configurations. There may be other uses too.

I see a couple of options:

- leave TestCase.error/fail as public
- hide TestCase.fail/error but add fail() and error() as top-level functions in
unit test that act on the current test
- optionally also have test() return a test number that can be passed as an
optional or named parameter  to these top  level fail/error functions; this is
the most flexible but it is a bit ugly and defeats part of the purpose of your
CL

The second option may be the best.I do think it is useful to have (at least)
fail(), as it provides a way for a test to fail itself and pass a stack trace of
its choosing, as opposed to having to throw and not controlling the stack trace.

[kevmoo-old, 2013-03-05 23:54:41]

> fail() is used in unittest_test.dart. 

This is calling top-level matcher.fail(). No instance of TestCase there.

> error() is used in some of the
> xx_config.dart test configurations.

The two cases are html_layout_config and interactive_html_config, both of which
are full of deprecated HTML API uses. Are these being maintained?

> There may be other uses too.

This is true. I'm all for being careful.

> 
> I see a couple of options:
> 
> - leave TestCase.error/fail as public

Perhaps deprecated to see if folks get broken?

> - hide TestCase.fail/error but add fail() and error() as top-level functions
in
> unit test that act on the current test

fail is already top-level on matcher (exported by unittest). Good enough?

> - optionally also have test() return a test number that can be passed as an
> optional or named parameter  to these top  level fail/error functions; this is
> the most flexible but it is a bit ugly and defeats part of the purpose of your
> CL
> 
> The second option may be the best.I do think it is useful to have (at least)
> fail(), as it provides a way for a test to fail itself and pass a stack trace
of
> its choosing, as opposed to having to throw and not controlling the stack
trace.

What does it mean for a test to fail itself? I really like to think of TestCase
as just a placeholder for the TestFunction. We need to make sure the right
things are exposed (via top-level members) for that function.

Thoughts?

[kevmoo-old, 2013-03-05 23:57:34]

Scratch deprecated, html_layout_config and interactive_html_config are using
eliminated HTML APIs. I don't think they'll work at all as they currently exist.

[kevmoo-old, 2013-03-13 04:38:18]

Updated CL to not break layout or interactive Configurations

...still eliminating a lot of unnecessary exposure of implementation details

PTAL

[gram, 2013-03-13 16:35:58]

lgtm apart from the simple getters/setters. Bob, can you verify that my
interpretation of the style guide is correct in this context?

https://codereview.chromium.org/12452004/diff/19001/pkg/unittest/lib/src/test...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/12452004/diff/19001/pkg/unittest/lib/src/test...
pkg/unittest/lib/src/test_case.dart:29: set tearDown(Function value) =>
_tearDown = value;
I think I have been guilty of this myself, but according to the style guide we
shouldn't do this. See
http://www.dartlang.org/articles/style-guide/#avoid-wrapping-fields-in-getter...

[kevmoo-old, 2013-03-13 16:45:55]

On 2013/03/13 16:35:58, gram wrote:
> lgtm apart from the simple getters/setters. Bob, can you verify that my
> interpretation of the style guide is correct in this context?
> 
>
https://codereview.chromium.org/12452004/diff/19001/pkg/unittest/lib/src/test...
> File pkg/unittest/lib/src/test_case.dart (right):
> 
>
https://codereview.chromium.org/12452004/diff/19001/pkg/unittest/lib/src/test...
> pkg/unittest/lib/src/test_case.dart:29: set tearDown(Function value) =>
> _tearDown = value;
> I think I have been guilty of this myself, but according to the style guide we
> shouldn't do this. See
>
http://www.dartlang.org/articles/style-guide/#avoid-wrapping-fields-in-getter...

Happy to fix those up. I reverted the change I had where those are fixed to
minimize the delta. Agree that setter/getter is silly.

I'll tweak the CL in a bit here.

[Siggi Cherem (dart-lang), 2013-03-13 16:46:04]

https://codereview.chromium.org/12452004/diff/19001/pkg/unittest/lib/src/test...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/12452004/diff/19001/pkg/unittest/lib/src/test...
pkg/unittest/lib/src/test_case.dart:29: set tearDown(Function value) =>
_tearDown = value;
On 2013/03/13 16:35:58, gram wrote:
> I think I have been guilty of this myself, but according to the style guide we
> shouldn't do this. See
>
http://www.dartlang.org/articles/style-guide/#avoid-wrapping-fields-in-getter...

yep - if you expose both read and write access, it should simply be a public
field.

[kevmoo-old, 2013-03-13 19:10:04]

Added fix for redundant getter/setter in TestCase

PTAL

[gram, 2013-03-13 21:06:43]

lgtm

[kevmoo-old, 2013-03-13 21:19:13]

Committed patchset #6 manually as r19972 (presubmit successful).