Added new style of tests for parser.

Added new style of tests for parser.

R=rnystrom@google.com

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

[Andrei Mouravski, 2013-06-18 23:57:40]

Is 90 lines better?

[Bob Nystrom, 2013-06-19 00:23:18]

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
File pkg/args/test/parse_all_test.dart (right):

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:22: group('unknown argument', () {
"Unknown" is confusing here. "non-option"?

How about:

group("starting with a non-option", () {...})

and then ditch the comment.

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:23: // args: 'A --flag'
Remove this and just make the arg list a list literal. :)

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:29: var results = parser.parse(args);
Just inline the args here:

parser.parse(['A', '--flag']);

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:84: });
I think you could accomplish as much by adding a test or two to the parser suite
like:

      test('ignores options following a non-option-like arg', () {
        var parser = new ArgParser();
        parser.addFlag('woof');
        parser.addOption('meow');

        var results = parser.parse(['arg', '--woof', '--meow']);
        expect(results['woof'], isNull);
        expect(results['meow'], isNull);
        expect(results.rest, orderedEquals(['arg', '--woof', '--meow']));
      });

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:90: expect(() => parser.parse(args), throws);
Unused? Remove.

[Andrei Mouravski, 2013-06-19 19:50:55]

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
File pkg/args/test/parse_all_test.dart (right):

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:22: group('unknown argument', () {
On 2013/06/19 00:23:18, Bob Nystrom wrote:
> "Unknown" is confusing here. "non-option"?
> 
> How about:
> 
> group("starting with a non-option", () {...})
> 
> and then ditch the comment.

Done.

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:23: // args: 'A --flag'
On 2013/06/19 00:23:18, Bob Nystrom wrote:
> Remove this and just make the arg list a list literal. :)

Done.

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:29: var results = parser.parse(args);
I'd rather keep it as a var so I can check against it.

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:84: });
I still think it's valuable to be spartan because it'll make the contrast with
parseAll make more sense.

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:90: expect(() => parser.parse(args), throws);
On 2013/06/19 00:23:18, Bob Nystrom wrote:
> Unused? Remove.

Done.

[Bob Nystrom, 2013-06-19 19:53:04]

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
File pkg/args/test/parse_all_test.dart (right):

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:29: var results = parser.parse(args);
On 2013/06/19 19:50:56, Andrei Mouravski wrote:
> I'd rather keep it as a var so I can check against it.

OK, well at least create it using a literal. The .add() seems needlessly
verbose.

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:84: });
On 2013/06/19 19:50:56, Andrei Mouravski wrote:
> I still think it's valuable to be spartan because it'll make the contrast with
> parseAll make more sense.

I don't understand this. Why do you want to contrast with the other tests.
Wouldn't it be better to be consistent?

[Andrei Mouravski, 2013-06-19 20:00:35]

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
File pkg/args/test/parse_all_test.dart (right):

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:29: var results = parser.parse(args);
I did. You're looking at the wrong patch. :]

https://codereview.chromium.org/17333004/diff/5001/pkg/args/test/parse_all_te...
pkg/args/test/parse_all_test.dart:84: });
I'm not contrasting with the other tests. I'm saying that once my next change
goes in, there will be contrast within these tests for how parseAll=true is
different from parseAll=false.

[Andrei Mouravski, 2013-06-19 21:27:23]

Ping.

[Bob Nystrom, 2013-06-19 22:22:21]

One suggestion. Otherwise LGTM.

https://codereview.chromium.org/17333004/diff/10001/pkg/args/test/parse_all_t...
File pkg/args/test/parse_all_test.dart (right):

https://codereview.chromium.org/17333004/diff/10001/pkg/args/test/parse_all_t...
pkg/args/test/parse_all_test.dart:13: var args;
How about removing these and just making them local to each test?

[Andrei Mouravski, 2013-06-19 23:05:47]

Committed patchset #5 manually as r24207 (presubmit successful).

[Andrei Mouravski, 2013-06-19 23:16:20]

Thanks!

https://codereview.chromium.org/17333004/diff/10001/pkg/args/test/parse_all_t...
File pkg/args/test/parse_all_test.dart (right):

https://codereview.chromium.org/17333004/diff/10001/pkg/args/test/parse_all_t...
pkg/args/test/parse_all_test.dart:13: var args;
On 2013/06/19 22:22:21, Bob Nystrom wrote:
> How about removing these and just making them local to each test?

Done.