test.py: Support for CompileTimeError,RuntimeError,MissingRuntimeError,MissingCompiletimeError mark…

test.py: Support for CompileTimeError,RuntimeError,MissingRuntimeError,MissingCompiletimeError markers in status files

R=ricow@google.com

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

[ricow1, 2013-09-23 12:07:32]

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
File tools/testing/dart/status_file_parser.dart (right):

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
tools/testing/dart/status_file_parser.dart:21: static Expectation
COMPILETIME_ERROR = byName('CompileTimeError');
nit, but there seems to be some difference in how we handle Compile{t||T}ime.
Here we call it CompileTimeError, below we call it MissingCompiletimeError

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
tools/testing/dart/status_file_parser.dart:43: static final _AllExpectations =
new Map<String, Expectation>();
just let this be null and branch on that in _initialize? then you can remove the
_initialized variable above and just create the map lazily as well

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
tools/testing/dart/status_file_parser.dart:75: // Indicates whether this
expectation can actually be the outcome of a test.
well it actually indicates the opposite

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

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:548: //print("Could narrow Expected:
$expectation to $outcome");
you don't do anything + no FIXME

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1140: // FIXME(kustermann): Do we consider a
"runtimeError" only an uncaught
FIXME->TODO

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1142: if (exitCode != 0) {
well I guess at least DART_VM_EXITCODE_COMPILE_TIME_ERROR should not be allowed
here, Ivan can probably give additional info here

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1186: exitCode ==
VmCommandOutputImpl.DART_VM_EXITCODE_COMPILE_TIME_ERROR ||
If seems a little strange to reference constants in another implementation class
here, could we move them out?

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1195: // FIXME(kustermann): Do we have a
special exit code in that case???
fixme->todo

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1202: // FIXME(kustermann): this is a hack,
remove it
fixme -> todo

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1214: if
(outcome.canBeOutcomeOf(Expectation.FAIL)) {
this code is duplicated here, above and below, could we extract it and just call
that function

[kustermann, 2013-09-23 15:31:33]

PTAL

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
File tools/testing/dart/status_file_parser.dart (right):

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
tools/testing/dart/status_file_parser.dart:21: static Expectation
COMPILETIME_ERROR = byName('CompileTimeError');
On 2013/09/23 12:07:32, ricow1 wrote:
> nit, but there seems to be some difference in how we handle Compile{t||T}ime.
> Here we call it CompileTimeError, below we call it MissingCompiletimeError

Changed it to *CompileTime*

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
tools/testing/dart/status_file_parser.dart:43: static final _AllExpectations =
new Map<String, Expectation>();
On 2013/09/23 12:07:32, ricow1 wrote:
> just let this be null and branch on that in _initialize? then you can remove
the
> _initialized variable above and just create the map lazily as well

Done.

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/status_...
tools/testing/dart/status_file_parser.dart:75: // Indicates whether this
expectation can actually be the outcome of a test.
On 2013/09/23 12:07:32, ricow1 wrote:
> well it actually indicates the opposite 

I clarified it a bit.

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

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:548: //print("Could narrow Expected:
$expectation to $outcome");
On 2013/09/23 12:07:32, ricow1 wrote:
> you don't do anything + no FIXME

This was actually intentional -- I left this comment to remind me about it (I
can't do it in this CL).

Though I can remove the comment.

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1140: // FIXME(kustermann): Do we consider a
"runtimeError" only an uncaught
On 2013/09/23 12:07:32, ricow1 wrote:
> FIXME->TODO

Done.

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1142: if (exitCode != 0) {
On 2013/09/23 12:07:32, ricow1 wrote:
> well I guess at least DART_VM_EXITCODE_COMPILE_TIME_ERROR should not be
allowed
> here, Ivan can probably give additional info here

I can't do that because some tests will start failing then.

I'll send the VM guys a patch how to enable it (and a list of affected tests),
then they can fix the exitcode issues.

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1186: exitCode ==
VmCommandOutputImpl.DART_VM_EXITCODE_COMPILE_TIME_ERROR ||
On 2013/09/23 12:07:32, ricow1 wrote:
> If seems a little strange to reference constants in another implementation
class
> here, could we move them out?

IMHO it's not strange at all. The VmCommandOutputImpl is the class responsible
for determining what the outcome of running a VM was. So having the constants
there seems natural.

Why should we not keep it this way?

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1195: // FIXME(kustermann): Do we have a
special exit code in that case???
On 2013/09/23 12:07:32, ricow1 wrote:
> fixme->todo

Done.

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1202: // FIXME(kustermann): this is a hack,
remove it
On 2013/09/23 12:07:32, ricow1 wrote:
> fixme -> todo

Done.

https://codereview.chromium.org/23702055/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1214: if
(outcome.canBeOutcomeOf(Expectation.FAIL)) {
On 2013/09/23 12:07:32, ricow1 wrote:
> this code is duplicated here, above and below, could we extract it and just
call
> that function

I thought about this as well. I moved it now up to the superclass.

[ricow1, 2013-09-24 09:02:31]

LGTM

https://codereview.chromium.org/23702055/diff/22001/tools/testing/dart/test_r...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/23702055/diff/22001/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:781: if (testCase.isNegative) {
use _negateOutcomeIfNegativeTest

[kustermann, 2013-09-24 10:21:31]

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

[kustermann, 2013-09-25 12:12:39]

https://codereview.chromium.org/23702055/diff/22001/tools/testing/dart/test_r...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/23702055/diff/22001/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:781: if (testCase.isNegative) {
On 2013/09/24 09:02:31, ricow1 wrote:
> use _negateOutcomeIfNegativeTest

Forgot about this one, I'll include it in another CL.