pkg/unittest: config refactor

pkg/unittest: config refactor

Make the configure method a setter (per existing TODO)

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

[kevmoo-old, 2013-03-23 22:44:26]

PTAL

Define a config setter to go along with the getter (per TODO)
Deprecated the configure method.
Throw if config is set more than once.
Also throws if config is set after testing begins, which maintains consistent
state

[Siggi Cherem (dart-lang), 2013-03-24 17:20:08]

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/compact_vm_c...
File pkg/unittest/lib/compact_vm_config.dart (left):

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/compact_vm_c...
pkg/unittest/lib/compact_vm_config.dart:174: if (config != null) return;
Let's keep this part (here and in the other configurations).

It's ok for config = X to throw, but we should still make it easy to compose
test files together. In particular, we commonly write tests in multiple files
like this:

feature1_test.dart:
  main() { useXConfiguration(); ... some tests }

feature2_test.dart:
  main() { useXConfiguration(); ... some tests }

all_features_test.dart:
  import 'feature1_test.dart' as f1;
  import 'feature2_test.dart' as f2;
  main() {
    useXConfiguration();
    f1.main();
    f2.main();
  }


True - people would have the alternative to write the code as follows:

feature1_test.dart:
  main() { if (config == null) useXConfiguration(); ... some tests }

feature2_test.dart:
  main() { if (config == null) useXConfiguration(); ... some tests }

all_features_test.dart:
  import 'feature1_test.dart' as f1;
  import 'feature2_test.dart' as f2;
  main() {
    if (config == null) useXConfiguration();
    f1.main();
    f2.main();
  }

but I think the whole purpose of 'useXConfiguration' is to make these use cases
simple and make life easier for users of unittest. We can add a documentation
comment on each of this useXConfig explaining things, if you like.

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

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:159: import 'package:meta/meta.dart';
unfortunately this library is used within the dart-repo in a way that prevent's
us from using "package:" imports here. I believe you can work around this by
using:

  import '../../../pkg/meta/lib/meta.dart'

But note that it is not enough to use '../../meta/lib/meta.dart'. You need to
get all the way to 'pkg' so that the scripts that publish unittest as a package
can work.

I'm not sure if just fixing the import is enough, so I'm adding Andrei and Dan
to see if any of them can share any light. In particular, I recall trying to add
a deprecation warning in unittest before and it was painful to get it working
=(... I even gave up on it at some point, but can't recall why.

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:829: config = new Configuration();
let's keep this with _config (you already checked for null anyways)

[kevmoo-old, 2013-03-24 18:00:02]

Sound reasonable?

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/compact_vm_c...
File pkg/unittest/lib/compact_vm_config.dart (left):

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/compact_vm_c...
pkg/unittest/lib/compact_vm_config.dart:174: if (config != null) return;
How about Configuration classes implement

bool operator ==

And the setter checks to see if the new value and old value are equal. If yes,
then it's a no-op.

We get the best of both:

Composition of related tests: goodness.
Errors if someone tries to set multiple, incompatible configs.

[Siggi Cherem (dart-lang), 2013-03-24 18:18:41]

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/compact_vm_c...
File pkg/unittest/lib/compact_vm_config.dart (left):

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/compact_vm_c...
pkg/unittest/lib/compact_vm_config.dart:174: if (config != null) return;
On 2013/03/24 18:00:02, kevmoo wrote:
> How about Configuration classes implement
> 
> bool operator ==
> 
> And the setter checks to see if the new value and old value are equal. If yes,
> then it's a no-op.
> 
> We get the best of both:
> 
> Composition of related tests: goodness.
> Errors if someone tries to set multiple, incompatible configs.

Could be. One reason I'm not entirely sure is that I can imagine users wanting
to use a more detailed configuration when running a subset of the test, and a
more compact config when running all tests together.

Note that even if we implement 'operator ==', we can't replace the
configuration. If we do, then we might get to an inconsistent state inside
unittest (it relies on calling init and start on the same configuration,
otherwise bad things happen).

Here is another (similar) idea, instead of overriding '==', define a singleton
for each configuration. That way, if you call useXConfiguration twice, you are
basically assigning exactly the same instance and everything just works.

final _singleton = new CompactVMConfiguration();
void useCompactVMConfiguraiton() {
  config = _singleton;
}


Then, config's setter can check:
set config(newConfig) {
  if (_config != null || !identical(_config, newConfig)) { error... }
  ...
}

[Andrei Mouravski, 2013-03-24 18:49:23]

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

https://codereview.chromium.org/12770017/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:159: import 'package:meta/meta.dart';
On 2013/03/24 17:20:09, Siggi Cherem (dart-lang) wrote:
> unfortunately this library is used within the dart-repo in a way that
prevent's
> us from using "package:" imports here. I believe you can work around this by
> using:
> 
>   import '../../../pkg/meta/lib/meta.dart'
> 
> But note that it is not enough to use '../../meta/lib/meta.dart'. You need to
> get all the way to 'pkg' so that the scripts that publish unittest as a
package
> can work.
> 
> I'm not sure if just fixing the import is enough, so I'm adding Andrei and Dan
> to see if any of them can share any light. In particular, I recall trying to
add
> a deprecation warning in unittest before and it was painful to get it working
> =(... I even gave up on it at some point, but can't recall why.

From what I remember, this wasn't working because dartdoc, and by extension,
apidoc, was breaking on trying to find the library itself. I'm pretty sure this
is actually fixed, but the the test is something like:
rm -rf out/ReleaseIA32/api_docs && build.py -m release
or equivalent on Mac/Win

[kevmoo-old, 2013-03-25 14:54:59]

> From what I remember, this wasn't working because dartdoc, and by extension,
> apidoc, was breaking on trying to find the library itself. I'm pretty sure
this
> is actually fixed, but the the test is something like:
> rm -rf out/ReleaseIA32/api_docs && build.py -m release
> or equivalent on Mac/Win

apidoc reports warnings, but docs get generated fine.

Analyzing libraries...
../../xcodebuild/ReleaseIA32/packages/meta/meta.dart:10:9: Warning: duplicated
library name "meta".
library meta;
        ^^^^
../../pkg/meta/lib/meta.dart:10:9: Warning: duplicated library name "meta".
library meta;
        ^^^^

[kevmoo-old, 2013-03-25 15:15:19]

Updated w/ Siggi's singelton recommendation.

PTAL

[Emily Fortuna, 2013-03-25 17:00:15]

looks good to me, but I'd wait until you get confirmation from someone else,
(gram? dgrove?) since this corner of the codebase is not my expertise. :-)

[gram, 2013-03-25 17:09:19]

lgtm with one possible issue.

https://codereview.chromium.org/12770017/diff/9001/pkg/unittest/test/unittest...
File pkg/unittest/test/unittest_test.dart (right):

https://codereview.chromium.org/12770017/diff/9001/pkg/unittest/test/unittest...
pkg/unittest/test/unittest_test.dart:76: config = _testconfig;
I wonder if config is not too generic a name and might result in collisions
(yes, you can argue the same about configure() but I intuitively feel that is
less likely a problem). Maybe we should call this something else like
testConfig.

[kevmoo-old, 2013-03-25 17:11:52]

https://codereview.chromium.org/12770017/diff/9001/pkg/unittest/test/unittest...
File pkg/unittest/test/unittest_test.dart (right):

https://codereview.chromium.org/12770017/diff/9001/pkg/unittest/test/unittest...
pkg/unittest/test/unittest_test.dart:76: config = _testconfig;
or go all the way and call it unittestConfiguration

It's not like this is referenced a lot.

or testConfiguration

I have a paranoia about abbreviations in public API, but I'm open to discussion.

[kevmoo-old, 2013-03-25 17:32:58]

Updated. PTAL.

[gram, 2013-03-25 17:39:11]

lgtm

[kevmoo-old, 2013-03-25 18:03:25]

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

[Siggi Cherem (dart-lang), 2013-03-27 21:05:58]

lgtm

sorry for the slow response, I was out of the office the last couple days.

https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/html_ind...
File pkg/unittest/lib/html_individual_config.dart (right):

https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/html_ind...
pkg/unittest/lib/html_individual_config.dart:51: unittest.unittestConfiguration
= isLayoutTest ? _singletonLayout : _singletonNotLayout;
nit: 80 col

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

https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:171: Configuration get unittestConfiguration =>
_config;
I wouldn't be too scared about external name collisions: if someone runs into
one, they can include the library with a prefix. But I agree with Kevin: full
names is better than abbreviations on public APIs.

I have a slight preference for just calling it "configuration"
(unittestConfiguration feels redundant), but I'm OK submitting as it is.

[Siggi Cherem (dart-lang), 2013-03-27 21:06:43]

On 2013/03/27 21:05:58, Siggi Cherem (dart-lang) wrote:
> lgtm
> 
> sorry for the slow response, I was out of the office the last couple days.
> 
>
https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/html_ind...
> File pkg/unittest/lib/html_individual_config.dart (right):
> 
>
https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/html_ind...
> pkg/unittest/lib/html_individual_config.dart:51:
unittest.unittestConfiguration
> = isLayoutTest ? _singletonLayout : _singletonNotLayout;
> nit: 80 col
> 
>
https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/unittest...
> File pkg/unittest/lib/unittest.dart (right):
> 
>
https://codereview.chromium.org/12770017/diff/19001/pkg/unittest/lib/unittest...
> pkg/unittest/lib/unittest.dart:171: Configuration get unittestConfiguration =>
> _config;
> I wouldn't be too scared about external name collisions: if someone runs into
> one, they can include the library with a prefix. But I agree with Kevin: full
> names is better than abbreviations on public APIs.
> 
> I have a slight preference for just calling it "configuration"
> (unittestConfiguration feels redundant), but I'm OK submitting as it is.

or should I say: I'm OK leaving it as it is submitted =)

PS: thanks for all the cleanup!