Test running changes.

Test running changes.

1. Change in the test runner to consider any warning as warning.

After this following was required:

2. Triage language tests.

3. Triage co19 tests.

R=brianwilkerson@google.com, kustermann@google.com
BUG=

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

[Brian Wilkerson, 2013-06-28 22:18:46]

LGTM

[ricow1, 2013-06-30 10:52:10]

+ahe

[kustermann, 2013-07-01 06:24:16]

https://codereview.chromium.org/18174010/diff/1/tools/testing/dart/test_runne...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/18174010/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:852: // ignore all others
This comment seems to be wrong now.

Is there a reason why you switch on the 'ERROR_TYPE'. If you want all warnings
to be reported as warnings, you should do:
  if (fields[ERROR_LEVEL] == 'ERROR') {
    errors.add(fields[FORMATTED_ERROR]);
  } else if (fields[ERROR_LEVEL] == 'WARNING') {
    warnings.add(fields[FORMATTED_ERROR]);
  }

Are there other kinds of warnings we want to ignore?

[scheglov, 2013-07-01 07:04:30]

https://codereview.chromium.org/18174010/diff/1/tools/testing/dart/test_runne...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/18174010/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:852: // ignore all others
On 2013/07/01 06:24:17, kustermann wrote:
> This comment seems to be wrong now.
> 
> Is there a reason why you switch on the 'ERROR_TYPE'. If you want all warnings
> to be reported as warnings, you should do:
>   if (fields[ERROR_LEVEL] == 'ERROR') {
>     errors.add(fields[FORMATTED_ERROR]);
>   } else if (fields[ERROR_LEVEL] == 'WARNING') {
>     warnings.add(fields[FORMATTED_ERROR]);
>   }
> 
> Are there other kinds of warnings we want to ignore?

  Thank you, fixed.

[kustermann, 2013-07-01 11:58:37]

In general your change lgtm.

But I'd really like to get ahe's input as well.

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:878: bool hasFatalTypeErrors = false;
I wouldn't be surprised if this complex logic down here has some issues as well.

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:922: }
With your second change, you may want to change this to:
if (isStaticClean &&
    (errors.length > 0 || (staticWarnings.length > 0)) {
  // ...
  return true;
}

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1520: "isStaticClean" : !hasCompileError &&
!hasStaticWarning,
If I look into test_runner.dart:AnalysisCommandOutputImpl:_didStandardTestFail:
The 'isStaticClean' flag is only used in terms of warnings:
> ....
>     if (isStaticClean && staticWarnings.length > 0) {
>       diagnostics.add(
>           "@static-clean annotation found but analyzer returned warnings.");
>       return true;
>    }
> ....

So the change you made has no effect. Or do I miss something?

[Side Note:

If I understand this correctly, we expect all co19 tests to be free of analyzer
warnings/errors except the ones which have @compile-error/@static-warning
annotations.

I don't know if our agreement with the co19 team guarantees that or not.
Here's is what I think:

*Some* co19 tests will test that the analyzer outputs a warning/error in certain
cases.

*Other* co19 tests will test the libraries and I don't know if we expect them to
be free of warnings (they're after all testing library functionality not
analyzer warnings).]

[ahe, 2013-07-01 12:19:38]

On 2013/07/01 11:58:37, kustermann wrote:
> In general your change lgtm.
> 
> But I'd really like to get ahe's input as well.

I'm in the process of writing a longer reply about this, but unfortunately, I
will not be able to finish this before going on vacation tomorrow.  I'll be back
on Tuesday.

Cheers,
Peter

> 
>
https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
> tools/testing/dart/test_runner.dart:878: bool hasFatalTypeErrors = false;
> I wouldn't be surprised if this complex logic down here has some issues as
well.
> 
>
https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
> tools/testing/dart/test_runner.dart:922: }
> With your second change, you may want to change this to:
> if (isStaticClean &&
>     (errors.length > 0 || (staticWarnings.length > 0)) {
>   // ...
>   return true;
> }
> 
>
https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_su...
> File tools/testing/dart/test_suite.dart (right):
> 
>
https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_su...
> tools/testing/dart/test_suite.dart:1520: "isStaticClean" : !hasCompileError &&
> !hasStaticWarning,
> If I look into
test_runner.dart:AnalysisCommandOutputImpl:_didStandardTestFail:
> The 'isStaticClean' flag is only used in terms of warnings:
> > ....
> >     if (isStaticClean && staticWarnings.length > 0) {
> >       diagnostics.add(
> >           "@static-clean annotation found but analyzer returned warnings.");
> >       return true;
> >    }
> > ....
> 
> So the change you made has no effect. Or do I miss something?
> 
> [Side Note:
> 
> If I understand this correctly, we expect all co19 tests to be free of
analyzer
> warnings/errors except the ones which have @compile-error/@static-warning
> annotations.
> 
> I don't know if our agreement with the co19 team guarantees that or not.
> Here's is what I think:
> 
> *Some* co19 tests will test that the analyzer outputs a warning/error in
certain
> cases.
> 
> *Other* co19 tests will test the libraries and I don't know if we expect them
to
> be free of warnings (they're after all testing library functionality not
> analyzer warnings).]

[scheglov, 2013-07-02 21:19:42]

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:922: }
On 2013/07/01 11:58:38, kustermann wrote:
> With your second change, you may want to change this to:
> if (isStaticClean &&
>     (errors.length > 0 || (staticWarnings.length > 0)) {
>   // ...
>   return true;
> }

There is check for errors below, but it is done without message.
I've added this check for warnings here and also generalized message, because it
is not just warnings now.

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/18174010/diff/8001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1520: "isStaticClean" : !hasCompileError &&
!hasStaticWarning,
On 2013/07/01 11:58:38, kustermann wrote:
> If I look into
test_runner.dart:AnalysisCommandOutputImpl:_didStandardTestFail:
> The 'isStaticClean' flag is only used in terms of warnings:
> > ....
> >     if (isStaticClean && staticWarnings.length > 0) {
> >       diagnostics.add(
> >           "@static-clean annotation found but analyzer returned warnings.");
> >       return true;
> >    }
> > ....
> 
> So the change you made has no effect. Or do I miss something?

  "isStaticClean" is IMHO exactly what it says - it should be clean; has no any
problems - errors or warnings - no matter.

> 
> [Side Note:
> 
> If I understand this correctly, we expect all co19 tests to be free of
analyzer
> warnings/errors except the ones which have @compile-error/@static-warning
> annotations.

  I think so.


> I don't know if our agreement with the co19 team guarantees that or not.
> Here's is what I think:
> 
> *Some* co19 tests will test that the analyzer outputs a warning/error in
certain
> cases.
> 
> *Other* co19 tests will test the libraries and I don't know if we expect them
to
> be free of warnings (they're after all testing library functionality not
> analyzer warnings).]

  My impression is that co19 team aims to have only legitimate warnings, which
are supposed to be tested and don't have any extra/random warnings.

[scheglov, 2013-07-12 15:47:37]

ping

[scheglov, 2013-07-15 22:38:36]

On 2013/07/12 15:47:37, scheglov wrote:
> ping

ping

[ahe, 2013-07-16 11:04:58]

Isolated seen, this change is fine.

However, I think that the general approach of parsing output from the compiler
is broken.  For two reasons:

1. In general, the tests are written assuming a boolean test: "was there a
warning or not".  This means that you cannot make assumptions about how many
warnings are emitted in relation to annotations in the test, nor can you make
assumptions about the locations of these warnings.

2. You're testing a different code path than your users use.  It is possible
that printing human readable diagnostics may crash the compiler as they are more
complicated to generate, but you're not testing that code path.

My preferred approach to testing warnings in shared tests is to have a simple
mechanism that is easy to implement.  For example, adding a flag to the
compiler/analyzer that changes its exit code when a warning is emitted.  So to
test if a warning is emitted, the test is compiled/analyzed twice: once without
the flag and once with the flag.

As for testing quality of error messages, a different framework is required, and
this is something that will vary from implementation to implementation.  This is
not something we should necessarily share between implementations, so in my
opinion, implementation specific unit tests are a better way to test number of
diagnostics and locations of diagnostics.  In other words, in my opinion,
quality of diagnostics should be in junit tests for the analyzer and in
tests/compiler/dart2js for dart2js.

That being said, it would make sense for you to submit this CL, but I strongly
encourage you to remove parsing of compiler/analyzer output from test.dart.

[scheglov, 2013-07-16 20:36:02]

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