Multitest annotations now accept multiple annotations on one line.

Multitest annotations now accept multiple annotations on one line.

Many times a test case involves a static type warning for the
static analyzer and a dynamic type error for the VM or dart 
compiled to JS, but not always.  Thus, this change introduces
the annotations as allowing multiple outcomes for a test.

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

[zundel, 2012-03-07 20:30:39]

Hi Bill,

The VM and static analyzer encounter interference when trying to evaluate a test
case in a multitest that returns both a static warning and a dynamic type error
when executed in checked mode.  We discussed it and decided that we would try
annotating the test to allow more than one outcome.   This CL shows two test
cases where Regis and I were unable to make the tests effective for both
implementations.

[regis, 2012-03-07 23:38:31]

LGTM, but I will let Bill review the test harness code, since I am not familiar
with it.

Just a minor comment in the test itself, which you can ignore for now.

-- Regis

https://chromiumcodereview.appspot.com/9621015/diff/1/tests/language/src/Type...
File tests/language/src/TypeVariableBoundsTest.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/1/tests/language/src/Type...
tests/language/src/TypeVariableBoundsTest.dart:31: <String> /// 02: static type
error
This one should also be a "static type error, dynamic type error".
My next cl in the vm will generate the dynamic type error.

[Bill Hesse, 2012-03-08 10:21:48]

Would it be better to just add a new single error type, rather than shifting to
a list of errors?

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
tools/testing/dart/multitest.dart:106: outcomes[key].add(nextOutcome.trim());
This can be replaced by:
outcomes.putIfAbsent(key, ()=> new Set<String>())
     .add(nextOutcome)

You trimmed already.

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
tools/testing/dart/multitest.dart:121: 
Either the no-error case is an empty set, or it is a set containing ''.  You
create the empty set here, and the set containing '' above, and test for '' in
the place where this is used.

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:340: Set<String> outcome =
testCase.info.multitestOutcome;
It should be checked that we never reach here with a null multitestOutcome -
this is only enforced by control flow.

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:341: if ((outcome.contains('') ||
outcome.contains('compile-time error'))
Isn't this check about not containing 'run-time error', not about not containing
the other two?  '' shouldn't really be an element of the set anyway - you may
want to use the empty set instead.

[zundel, 2012-03-08 13:18:00]

https://chromiumcodereview.appspot.com/9621015/diff/1/tests/language/src/Type...
File tests/language/src/TypeVariableBoundsTest.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/1/tests/language/src/Type...
tests/language/src/TypeVariableBoundsTest.dart:31: <String> /// 02: static type
error
On 2012/03/07 23:38:31, regis wrote:
> This one should also be a "static type error, dynamic type error".
> My next cl in the vm will generate the dynamic type error.

I merged up and didn't see it yet. I will resolve this at commit time.

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
tools/testing/dart/multitest.dart:106: outcomes[key].add(nextOutcome.trim());
On 2012/03/08 10:21:49, Bill Hesse wrote:
> This can be replaced by:
> outcomes.putIfAbsent(key, ()=> new Set<String>())
>      .add(nextOutcome)

nice
> 
> You trimmed already.

done.

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
tools/testing/dart/multitest.dart:121: 
On 2012/03/08 10:21:49, Bill Hesse wrote:
> Either the no-error case is an empty set, or it is a set containing ''.  You
> create the empty set here, and the set containing '' above, and test for '' in
> the place where this is used.

I'm going with the empty set.

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:340: Set<String> outcome =
testCase.info.multitestOutcome;
On 2012/03/08 10:21:49, Bill Hesse wrote:
> It should be checked that we never reach here with a null multitestOutcome -
> this is only enforced by control flow.
Added assertion

https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:341: if ((outcome.contains('') ||
outcome.contains('compile-time error'))
On 2012/03/08 10:21:49, Bill Hesse wrote:
> Isn't this check about not containing 'run-time error', not about not
containing
> the other two?  '' shouldn't really be an element of the set anyway - you may
> want to use the empty set instead.

Besides runtime-error, we also don't want 'errors' populated for dynamic-type
errors and static-type errors. I think the root of the problem is that a test
annotated with 'compile-time' error is supposed to come back marked as 'failed',
even though this is the expected outcome.  If we get rid of the 'negative test'
concept, I think this might be simpler.

[zundel, 2012-03-08 13:19:05]

To answer the 'add a new status' question, we did consider that, but though it
is more verbose, we thought it better to just go with a list - it more clear
what's going on.


On 2012/03/08 13:18:00, zundel wrote:
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tests/language/src/Type...
> File tests/language/src/TypeVariableBoundsTest.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tests/language/src/Type...
> tests/language/src/TypeVariableBoundsTest.dart:31: <String> /// 02: static
type
> error
> On 2012/03/07 23:38:31, regis wrote:
> > This one should also be a "static type error, dynamic type error".
> > My next cl in the vm will generate the dynamic type error.
> 
> I merged up and didn't see it yet. I will resolve this at commit time.
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
> File tools/testing/dart/multitest.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
> tools/testing/dart/multitest.dart:106: outcomes[key].add(nextOutcome.trim());
> On 2012/03/08 10:21:49, Bill Hesse wrote:
> > This can be replaced by:
> > outcomes.putIfAbsent(key, ()=> new Set<String>())
> >      .add(nextOutcome)
> 
> nice
> > 
> > You trimmed already.
> 
> done.
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/mult...
> tools/testing/dart/multitest.dart:121: 
> On 2012/03/08 10:21:49, Bill Hesse wrote:
> > Either the no-error case is an empty set, or it is a set containing ''.  You
> > create the empty set here, and the set containing '' above, and test for ''
in
> > the place where this is used.
> 
> I'm going with the empty set.
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
> tools/testing/dart/test_runner.dart:340: Set<String> outcome =
> testCase.info.multitestOutcome;
> On 2012/03/08 10:21:49, Bill Hesse wrote:
> > It should be checked that we never reach here with a null multitestOutcome -
> > this is only enforced by control flow.
> Added assertion
> 
>
https://chromiumcodereview.appspot.com/9621015/diff/1/tools/testing/dart/test...
> tools/testing/dart/test_runner.dart:341: if ((outcome.contains('') ||
> outcome.contains('compile-time error'))
> On 2012/03/08 10:21:49, Bill Hesse wrote:
> > Isn't this check about not containing 'run-time error', not about not
> containing
> > the other two?  '' shouldn't really be an element of the set anyway - you
may
> > want to use the empty set instead.
> 
> Besides runtime-error, we also don't want 'errors' populated for dynamic-type
> errors and static-type errors. I think the root of the problem is that a test
> annotated with 'compile-time' error is supposed to come back marked as
'failed',
> even though this is the expected outcome.  If we get rid of the 'negative
test'
> concept, I think this might be simpler.

[Bill Hesse, 2012-03-08 13:52:57]

LGTM.

https://chromiumcodereview.appspot.com/9621015/diff/7001/tools/testing/dart/m...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/7001/tools/testing/dart/m...
tools/testing/dart/multitest.dart:101: nextOutcome = nextOutcome.trim();
Expect(nextOutcome != '')?
Is it illegal to have a multiline comment without an error?
if not, we will get '' in the set?

https://chromiumcodereview.appspot.com/9621015/diff/7001/tools/testing/dart/m...
tools/testing/dart/multitest.dart:105: "Invalid test directive '$nextOutcome' on
line ${lineCount}: $rest ");
Is this line too long?

https://chromiumcodereview.appspot.com/9621015/diff/7001/tools/testing/dart/m...
tools/testing/dart/multitest.dart:105: "Invalid test directive '$nextOutcome' on
line ${lineCount}: $rest ");
$lineCount

[zundel, 2012-03-09 00:34:22]

https://chromiumcodereview.appspot.com/9621015/diff/7001/tools/testing/dart/m...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/9621015/diff/7001/tools/testing/dart/m...
tools/testing/dart/multitest.dart:101: nextOutcome = nextOutcome.trim();
The value in the comment has to match a constrained set of values (test is
'validMulittestOutcomes' below).  I am removing '' from the list.

On 2012/03/08 13:52:57, Bill Hesse wrote:
> Expect(nextOutcome != '')?
> Is it illegal to have a multiline comment without an error?
> if not, we will get '' in the set?