Re-triage mirror test failures on the analyzer.

Re-triage mirror test failures on the analyzer.

R=kustermann@google.com, regis@google.com

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

[rmacnak, 2013-10-28 21:03:24]

+Martin

There are a number of mirror tests that include static warnings without being
multitests, such as mirrors/fake_function_without_call_test. These include the
"/// static type warning" directive, but the test harness still thinks they
should Pass in the analyzer. Is this a bug in the test harness, or am I using it
incorrectly?

[regis, 2013-10-28 21:06:38]

LGTM

I do not know what is the problem with the test harness. Hopefully, Martin can
help.

https://codereview.chromium.org/49083004/diff/30001/tests/lib/mirrors/generic...
File tests/lib/mirrors/generic_interface_test.dart (right):

https://codereview.chromium.org/49083004/diff/30001/tests/lib/mirrors/generic...
tests/lib/mirrors/generic_interface_test.dart:19: class Bienbounded implements
Bounded<int> {}
Bienbounded? :-)

[kustermann, 2013-10-28 21:56:36]

On 2013/10/28 21:06:38, regis wrote:
> LGTM
> 
> I do not know what is the problem with the test harness. Hopefully, Martin can
> help.
> 
>
https://codereview.chromium.org/49083004/diff/30001/tests/lib/mirrors/generic...
> File tests/lib/mirrors/generic_interface_test.dart (right):
> 
>
https://codereview.chromium.org/49083004/diff/30001/tests/lib/mirrors/generic...
> tests/lib/mirrors/generic_interface_test.dart:19: class Bienbounded implements
> Bounded<int> {}
> Bienbounded? :-)

The test harness doesn't understand '/// static type warning'! This is not a
valid multitest annotation. You can find a more detailed description at the top
of tools/testing/dart/multitest.dart.

The general idea is to generate multiple *real tests* out of a multitest. This
makes sure that the test passes for the '/none' case and that an error (or a
warning) will be reported with the '/// [0-9]: ...'  lines. (Otherwise the
warning could occur at any place, not necessarily at the line where it was
marked).

[rmacnak, 2013-10-28 22:29:49]

On 2013/10/28 21:56:36, kustermann wrote:
> 
> The test harness doesn't understand '/// static type warning'! This is not a
> valid multitest annotation.

I note we have several places in the language tests that have this marker.
 
> The general idea is to generate multiple *real tests* out of a multitest. This
> makes sure that the test passes for the '/none' case and that an error (or a
> warning) will be reported with the '/// [0-9]: ...'  lines. (Otherwise the
> warning could occur at any place, not necessarily at the line where it was
> marked).

Rewrote the tests with bounds warnings as numbered multitests (often with very
empty /none's).

Removed bogus marker from the rest and gave rationales in the status file.

[kustermann, 2013-10-28 23:04:17]

lgtm 

Thanks for removing lines from the status files and filing bugs for the other
ones :)

[The other tests you mentioned which have these '/// static type warning'
markers in them may be incorrect.]

https://codereview.chromium.org/49083004/diff/100001/tests/lib/mirrors/reflec...
File tests/lib/mirrors/reflect_class_test.dart (right):

https://codereview.chromium.org/49083004/diff/100001/tests/lib/mirrors/reflec...
tests/lib/mirrors/reflect_class_test.dart:16: Expect.throws(() =>
reflectClass("string"), expectedError);  /// static type warning
what about these?

[rmacnak, 2013-10-29 00:04:15]

https://codereview.chromium.org/49083004/diff/100001/tests/lib/mirrors/reflec...
File tests/lib/mirrors/reflect_class_test.dart (right):

https://codereview.chromium.org/49083004/diff/100001/tests/lib/mirrors/reflec...
tests/lib/mirrors/reflect_class_test.dart:16: Expect.throws(() =>
reflectClass("string"), expectedError);  /// static type warning
On 2013/10/28 23:04:17, kustermann wrote:
> what about these?

Woops. Got 'em. More careful greping says the mirror tests are clean of them
now.

[rmacnak, 2013-10-29 00:16:14]

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