feature: tag tests; choose tags on command line

feature: tag tests; choose tags on command line

BUG=

[nweiz, 2015-10-13 23:28:11]

nweiz@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2015-10-13 23:28:13]

I know you're already planning on making tags a Set and adding tests. I'd also
like an --exclude-tags flag to mirror --tags.

Looking over #16, it looks like the plan is to require tags to be defined in the
config file or on the command line. The former is obviously blocked on having a
config file, but if you're eager to land this soon you can make it always warn
for tags that aren't on the command line.

https://codereview.chromium.org/1405633004/diff/1/lib/src/backend/metadata.dart
File lib/src/backend/metadata.dart (right):

https://codereview.chromium.org/1405633004/diff/1/lib/src/backend/metadata.da...
lib/src/backend/metadata.dart:101: this.tags = tags ?? const <String>[];
Unfortunately "??" isn't usable here. Anything that gets loaded by Dartium,
which is basically everything in backend/, needs to work on an extra-wide range
of Dart versions since there's no good way to constrain the Dartium version.

https://codereview.chromium.org/1405633004/diff/1/lib/src/backend/metadata.da...
lib/src/backend/metadata.dart:204: 'tags': tags.isEmpty ? null : tags,
Just pass an empty list here rather than adding a special case to null it out.

https://codereview.chromium.org/1405633004/diff/1/lib/src/runner/configuratio...
File lib/src/runner/configuration.dart (right):

https://codereview.chromium.org/1405633004/diff/1/lib/src/runner/configuratio...
lib/src/runner/configuration.dart:80: parser.addOption("tags",
Add a "t" abbreviation, too.

https://codereview.chromium.org/1405633004/diff/1/lib/src/runner/configuratio...
lib/src/runner/configuration.dart:84: 
Add a hidden --tag flag too, to help prevent frustration.

https://codereview.chromium.org/1405633004/diff/1/lib/test.dart
File lib/test.dart (right):

https://codereview.chromium.org/1405633004/diff/1/lib/test.dart#newcode162
lib/test.dart:162: List<String> tags}) => _declarer.group(description, body,
I'd like to just have one dynamic-typed parameter "tag". See skip for a similar
example.

[yjbanov, 2015-10-30 20:14:00]

Apologies for taking so long.

https://codereview.chromium.org/1405633004/diff/1/lib/src/backend/metadata.dart
File lib/src/backend/metadata.dart (right):

https://codereview.chromium.org/1405633004/diff/1/lib/src/backend/metadata.da...
lib/src/backend/metadata.dart:101: this.tags = tags ?? const <String>[];
On 2015/10/13 23:28:12, nweiz wrote:
> Unfortunately "??" isn't usable here. Anything that gets loaded by Dartium,
> which is basically everything in backend/, needs to work on an extra-wide
range
> of Dart versions since there's no good way to constrain the Dartium version.

Done.

https://codereview.chromium.org/1405633004/diff/1/lib/src/backend/metadata.da...
lib/src/backend/metadata.dart:204: 'tags': tags.isEmpty ? null : tags,
On 2015/10/13 23:28:12, nweiz wrote:
> Just pass an empty list here rather than adding a special case to null it out.

Done.

https://codereview.chromium.org/1405633004/diff/1/lib/src/runner/configuratio...
File lib/src/runner/configuration.dart (right):

https://codereview.chromium.org/1405633004/diff/1/lib/src/runner/configuratio...
lib/src/runner/configuration.dart:80: parser.addOption("tags",
On 2015/10/13 23:28:12, nweiz wrote:
> Add a "t" abbreviation, too.

Done.

https://codereview.chromium.org/1405633004/diff/1/lib/src/runner/configuratio...
lib/src/runner/configuration.dart:84: 
On 2015/10/13 23:28:12, nweiz wrote:
> Add a hidden --tag flag too, to help prevent frustration.

Done.

https://codereview.chromium.org/1405633004/diff/1/lib/test.dart
File lib/test.dart (right):

https://codereview.chromium.org/1405633004/diff/1/lib/test.dart#newcode162
lib/test.dart:162: List<String> tags}) => _declarer.group(description, body,
On 2015/10/13 23:28:12, nweiz wrote:
> I'd like to just have one dynamic-typed parameter "tag". See skip for a
similar
> example.

Done.

[nweiz, 2015-11-03 00:43:16]

Don't forget --exclude-tags!

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
File lib/src/backend/metadata.dart (right):

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:101: this.tags = _makeSetOfTags(tags);
The type-normalization should only happen in [new Metadata.parse]. [new
Metadata] should just take an Iterable<String>, and [new Metadata.deserialize]
should assume it will deserialize to a list.

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:237: if (tag.trim().isEmpty) {
I'd rather not trim here. If users want to use whitespace tags or even empty
tags, I don't see why they shouldn't.

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:248: tags.split(",").map((tag) =>
tag.trim()).where((tag) => tag.isNotEmpty));
I'd rather require users to use a real list if they want multiple tags. The less
parsing we do, the less users need to remember how the specific parsing works.

https://codereview.chromium.org/1405633004/diff/20001/test/runner/runner_test...
File test/runner/runner_test.dart (right):

https://codereview.chromium.org/1405633004/diff/20001/test/runner/runner_test...
test/runner/runner_test.dart:676: group("tags", () {
I'm trying to keep runner_test.dart from getting more bloated. Can you move this
to its own file?

[yjbanov, 2015-11-11 06:40:20]

--exclude-tags also done

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
File lib/src/backend/metadata.dart (right):

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:101: this.tags = _makeSetOfTags(tags);
On 2015/11/03 00:43:16, nweiz wrote:
> The type-normalization should only happen in [new Metadata.parse]. [new
> Metadata] should just take an Iterable<String>, and [new Metadata.deserialize]
> should assume it will deserialize to a list.

Done.

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:237: if (tag.trim().isEmpty) {
On 2015/11/03 00:43:16, nweiz wrote:
> I'd rather not trim here. If users want to use whitespace tags or even empty
> tags, I don't see why they shouldn't.

I thought it would be more practical given we accept tags as command-line
arguments, where one has to take a moment to figure out how to properly pass
leading/trailing whitespace, and because we do not lose any generality either
way. However, I'll remove it if you still think leading/trailing whitespace is
useful somehow.

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:248: tags.split(",").map((tag) =>
tag.trim()).where((tag) => tag.isNotEmpty));
On 2015/11/03 00:43:16, nweiz wrote:
> I'd rather require users to use a real list if they want multiple tags. The
less
> parsing we do, the less users need to remember how the specific parsing works.

I thought it would make it consistent with the command-line arguments where we
do accept comma-separated tags, and again, one would have to think longer to
figure out how to pass a tag with a comma in the name without having ArgParser
split it into multiple tags.

https://codereview.chromium.org/1405633004/diff/20001/test/runner/runner_test...
File test/runner/runner_test.dart (right):

https://codereview.chromium.org/1405633004/diff/20001/test/runner/runner_test...
test/runner/runner_test.dart:676: group("tags", () {
On 2015/11/03 00:43:16, nweiz wrote:
> I'm trying to keep runner_test.dart from getting more bloated. Can you move
this
> to its own file?

Done.

[nweiz, 2015-11-16 21:59:44]

I can't believe I forgot this until now, but another thing that needs to happen
for any new piece of metadata is adding an annotation for it that can be used at
the suite level. This shouldn't be too hard—just do the same thing for a @Tags
annotation that's done for e.g. @Skip.

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
File lib/src/backend/metadata.dart (right):

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:237: if (tag.trim().isEmpty) {
On 2015/11/11 06:40:20, yjbanov wrote:
> On 2015/11/03 00:43:16, nweiz wrote:
> > I'd rather not trim here. If users want to use whitespace tags or even empty
> > tags, I don't see why they shouldn't.
> 
> I thought it would be more practical given we accept tags as command-line
> arguments, where one has to take a moment to figure out how to properly pass
> leading/trailing whitespace, and because we do not lose any generality either
> way. However, I'll remove it if you still think leading/trailing whitespace is
> useful somehow.

I'd like to keep the semantics on the Dart side independent of the specifics of
parsing command-line arguments.

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
File lib/src/runner/configuration.dart (right):

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
lib/src/runner/configuration.dart:87: help: 'Same as --tags',
There's no need for help on a hidden option.

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
lib/src/runner/configuration.dart:91: abbr: 'e',
Let's make this "x".

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
lib/src/runner/configuration.dart:197:
excludeTags.addAll(options['exclude-tag']);
It's safe to use ?? here, which should clean this up some.

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
File test/backend/metadata_test.dart (right):

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
test/backend/metadata_test.dart:27: unorderedEquals(expected));
I don't think we should explicitly test the serialized representation. At most,
we should test that deserialize can consume a serialized representation of a few
representative tag lists.

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
test/backend/metadata_test.dart:31: void expectArgumentError(ArgumentError
error, tags, String message) {
We usually don't test argument errors' values in this level of depth. Just
throwsArgumentError should be sufficient.

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
test/backend/metadata_test.dart:45: } on ArgumentError catch(e) {
Use the throwsArgumentError matcher here.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.dart
File test/runner/tag_test.dart (right):

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:42: test = runTest(["test.dart"]);
Each of these invocations of the test runner should be in a separate test. Then
you can define [expectTagWarnings] as a top-level function, with [test] as a
top-level field created in setUp.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:92: var test = runTest(["test.dart"]);
You don't need to test what happens if no flags are passed again.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:194:
test.stderr.expect(consumeThrough(contains("Included and excluded tag sets may
not intersect. Found intersection: a, b")));
Long line.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:213: }
Also test that you exclude tags specified on the group.

[yjbanov, 2015-11-26 06:30:27]

@Tags annotation added also

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
File lib/src/backend/metadata.dart (right):

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:237: if (tag.trim().isEmpty) {
On 2015/11/16 21:59:44, nweiz wrote:
> On 2015/11/11 06:40:20, yjbanov wrote:
> > On 2015/11/03 00:43:16, nweiz wrote:
> > > I'd rather not trim here. If users want to use whitespace tags or even
empty
> > > tags, I don't see why they shouldn't.
> > 
> > I thought it would be more practical given we accept tags as command-line
> > arguments, where one has to take a moment to figure out how to properly pass
> > leading/trailing whitespace, and because we do not lose any generality
either
> > way. However, I'll remove it if you still think leading/trailing whitespace
is
> > useful somehow.
> 
> I'd like to keep the semantics on the Dart side independent of the specifics
of
> parsing command-line arguments.

Done.

https://codereview.chromium.org/1405633004/diff/20001/lib/src/backend/metadat...
lib/src/backend/metadata.dart:248: tags.split(",").map((tag) =>
tag.trim()).where((tag) => tag.isNotEmpty));
On 2015/11/11 06:40:20, yjbanov wrote:
> On 2015/11/03 00:43:16, nweiz wrote:
> > I'd rather require users to use a real list if they want multiple tags. The
> less
> > parsing we do, the less users need to remember how the specific parsing
works.
> 
> I thought it would make it consistent with the command-line arguments where we
> do accept comma-separated tags, and again, one would have to think longer to
> figure out how to pass a tag with a comma in the name without having ArgParser
> split it into multiple tags.

Done.

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
File lib/src/runner/configuration.dart (right):

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
lib/src/runner/configuration.dart:87: help: 'Same as --tags',
On 2015/11/16 21:59:44, nweiz wrote:
> There's no need for help on a hidden option.

Done.

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
lib/src/runner/configuration.dart:91: abbr: 'e',
On 2015/11/16 21:59:44, nweiz wrote:
> Let's make this "x".

Done.

https://codereview.chromium.org/1405633004/diff/40001/lib/src/runner/configur...
lib/src/runner/configuration.dart:197:
excludeTags.addAll(options['exclude-tag']);
On 2015/11/16 21:59:44, nweiz wrote:
> It's safe to use ?? here, which should clean this up some.

Done.

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
File test/backend/metadata_test.dart (right):

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
test/backend/metadata_test.dart:27: unorderedEquals(expected));
On 2015/11/16 21:59:44, nweiz wrote:
> I don't think we should explicitly test the serialized representation. At
most,
> we should test that deserialize can consume a serialized representation of a
few
> representative tag lists.

Done.

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
test/backend/metadata_test.dart:31: void expectArgumentError(ArgumentError
error, tags, String message) {
On 2015/11/16 21:59:44, nweiz wrote:
> We usually don't test argument errors' values in this level of depth. Just
> throwsArgumentError should be sufficient.

Done.

https://codereview.chromium.org/1405633004/diff/40001/test/backend/metadata_t...
test/backend/metadata_test.dart:45: } on ArgumentError catch(e) {
On 2015/11/16 21:59:44, nweiz wrote:
> Use the throwsArgumentError matcher here.

Done.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.dart
File test/runner/tag_test.dart (right):

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:42: test = runTest(["test.dart"]);
On 2015/11/16 21:59:44, nweiz wrote:
> Each of these invocations of the test runner should be in a separate test.
Then
> you can define [expectTagWarnings] as a top-level function, with [test] as a
> top-level field created in setUp.

Done.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:92: var test = runTest(["test.dart"]);
On 2015/11/16 21:59:44, nweiz wrote:
> You don't need to test what happens if no flags are passed again.

I was sanity checking the test itself, but done.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:194:
test.stderr.expect(consumeThrough(contains("Included and excluded tag sets may
not intersect. Found intersection: a, b")));
On 2015/11/16 21:59:44, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/1405633004/diff/40001/test/runner/tag_test.da...
test/runner/tag_test.dart:213: }
On 2015/11/16 21:59:44, nweiz wrote:
> Also test that you exclude tags specified on the group.

Done.

[nweiz, 2015-12-01 00:34:23]

LGTM! I'll make a few style tweaks and then merge it in.

Thanks for working on this :).

[nweiz, 2015-12-01 03:42:08]

On 2015/12/01 00:34:23, nweiz wrote:
> LGTM! I'll make a few style tweaks and then merge it in.
> 
> Thanks for working on this :).

Merged to master:
https://github.com/dart-lang/test/commit/90b76116bd5915219d2bdad8022e70adf778...