Add 'self_check' runtime for running self-checking unit tests

Add 'self_check' runtime for running self-checking unit tests

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rself_check language co19

This will search the pkg/ folder for files matching *_self_check.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

The 'self_check' runtime is not specifically tied to kernel,
although only kernel is using it at the moment.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All self-check tests are treated as the same test case. If one fails,
  the remaining self-check testers don't run for that input.
- There is no way to run a subset of the self-check tests, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

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

Committed: https://github.com/dart-lang/sdk/commit/2e718613aa61b781d78533049df993fefd307b9d

[asgerf, 2016-12-02 15:27:20]

Patchset #2 (id:20001) has been deleted

[asgerf, 2016-12-02 15:48:33]

Description was changed from

==========
Add a new runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants in the unit under test are
not violated while processing the given data set.

BUG=
==========

to

==========
Add a new runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All bulk testers are treated as the same test case. If one fails,
  the remaining bulk testers don't run for that input.
- There is no way to run a subset of the bulk testers, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

[asgerf, 2016-12-02 15:50:36]

Description was changed from

==========
Add a new runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All bulk testers are treated as the same test case. If one fails,
  the remaining bulk testers don't run for that input.
- There is no way to run a subset of the bulk testers, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

to

==========
Add a new runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All bulk testers are treated as the same test case. If one fails,
  the remaining bulk testers don't run for that input.
- There is no way to run a subset of the bulk testers, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

[asgerf, 2016-12-02 15:51:11]

Description was changed from

==========
Add a new runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All bulk testers are treated as the same test case. If one fails,
  the remaining bulk testers don't run for that input.
- There is no way to run a subset of the bulk testers, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

to

==========
Add 'bulk' runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All bulk testers are treated as the same test case. If one fails,
  the remaining bulk testers don't run for that input.
- There is no way to run a subset of the bulk testers, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

[asgerf, 2016-12-02 15:55:15]

asgerf@google.com changed reviewers:
+ kmillikin@google.com, kustermann@google.com, whesse@google.com

[Kevin Millikin (Google), 2016-12-05 12:17:49]

I like it, but I wish for a more suggestive name.  I can't think of one,
unfortunately.

[asgerf, 2016-12-05 13:59:26]

Description was changed from

==========
Add 'bulk' runtime for running unit tests that rely on test suite data.

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rbulk language co19

This will search the pkg/ folder for files matching *_bulk_tester.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

The 'bulk' runtime is not specifically tied to kernel, although only
kernel is using it at the moment.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All bulk testers are treated as the same test case. If one fails,
  the remaining bulk testers don't run for that input.
- There is no way to run a subset of the bulk testers, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

to

==========
Add 'self_check' runtime for running self-checking unit tests

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rself_check language co19

This will search the pkg/ folder for files matching *_self_check.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

The 'self_check' runtime is not specifically tied to kernel,
although only kernel is using it at the moment.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All self-check tests are treated as the same test case. If one fails,
  the remaining self-check testers don't run for that input.
- There is no way to run a subset of the self-check tests, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

[asgerf, 2016-12-05 14:04:32]

Renamed bulk to self_check.

Runtime argument: -rbulk => -rself_check
File pattern: *_bulk_tester.dart => *_self_check.dart

[kustermann, 2016-12-06 10:39:05]

The main concern is about the changes to the (very complex and convoluted)
test_suite.dart file. Would be nice if we can get rid of the
shouldSkipNegativeTest boolean.

Otherwise: lgtm


> - Tests that are expected to fail in the compiler show up as
>  errors when skipping compilation.

I think the reason is that --skip-compilations is used at only one place and not
all places. When skipping compilations and running a test that is expected to
fail in the compiler then there is no reason to actually run it at all. This
could be changed by filtering out these tests in the enqueing phase if
--skip-compilations is set.

https://codereview.chromium.org/2549793002/diff/60001/pkg/kernel/test/self_ch...
File pkg/kernel/test/self_check_util.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/pkg/kernel/test/self_ch...
pkg/kernel/test/self_check_util.dart:26: runTest(filename);
Maybe document if this [runTest] can ever be async (i.e. return a Future or not)
in the argument type.

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_options.dart:713: validRuntimes = const ['vm',
'self_check', 'none'];
What does -cdartk -rnone do? 

I assume you want to run it in two steps:
./tools/test.py -cdartk -rnone
./tools/test.py -cdartk -rself_check --skip-compilation

?

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:370: class VmBatchCommand extends
ProcessCommand implements VmCommand {
If you make a [VmBatchCommandOutputImpl] as well for this command (see e.g. 
KernelCompilationCommandOutputImpl) then this class can define was is 'success'
and what is 'failure'.

By doing so I think you might be able to get rid of shouldSkipNegativeTests and
the following code in test_suite.dart:

if (testCase.isNegative && runtimeConfiguration.shouldSkipNegativeTests) {
  return;
}

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:387: other is VmBatchCommand &&
The "other is VmBatchCommand" is unnecessary I think, because "operator==" will
take care of it.

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:300: }
I'm not sure if this is always doing the right thing. There are several
different cases of negative tests, some are "_negative_test.dart" (which can be
negative in compiler or runtime AFAIK) and multi tests annotations and co19 test
annotations, sometimes they are only negative in checked mode.

Could we get rid of this? 

The test_suite.dart file is already very complex and this makes it even worse.

Why is it even needed in the first place?

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:1358: '$tempDir/$fileName.js', compiler,
tempDir, optionsFromFile));
This is another path on how to compute compilation commands. Here we don't
respect the --skip-compilations' flag.

[asgerf, 2016-12-06 13:42:09]

I've spent over a week on getting our infrastructure to just run tests that are
equivalent to those we had before the merge, and for all its faults this is the
only solution I found that's actually seems feasible. If there is nothing
critically wrong with it can please just move on?

https://codereview.chromium.org/2549793002/diff/60001/pkg/kernel/test/self_ch...
File pkg/kernel/test/self_check_util.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/pkg/kernel/test/self_ch...
pkg/kernel/test/self_check_util.dart:26: runTest(filename);
On 2016/12/06 10:39:05, kustermann wrote:
> Maybe document if this [runTest] can ever be async (i.e. return a Future or
not)
> in the argument type.

Good point. I've changed it to be asynchronous.

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_options.dart:713: validRuntimes = const ['vm',
'self_check', 'none'];
On 2016/12/06 10:39:05, kustermann wrote:
> What does -cdartk -rnone do? 
> 
> I assume you want to run it in two steps:
> ./tools/test.py -cdartk -rnone
> ./tools/test.py -cdartk -rself_check --skip-compilation
> 
> ?

-rnone just compiles the code. I found it to be useful sometimes, usually with
--verify-ir.

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:370: class VmBatchCommand extends
ProcessCommand implements VmCommand {
On 2016/12/06 10:39:05, kustermann wrote:
> If you make a [VmBatchCommandOutputImpl] as well for this command (see e.g. 
> KernelCompilationCommandOutputImpl) then this class can define was is
'success'
> and what is 'failure'.
> 
> By doing so I think you might be able to get rid of shouldSkipNegativeTests
and
> the following code in test_suite.dart:
> 
> if (testCase.isNegative && runtimeConfiguration.shouldSkipNegativeTests) {
>   return;
> }

I gave this a shot but couldn't make it work. The VmBatchCommandOutputImpl only
gets to choose the outcome if it actually executes, which it doesn't if the
preceding compiler command fails.

Also the VmBatchCommand was intended to run Dart programs in batch mode without
being tied to the self_check runtime, but it could of course just be renamed to
SelfCheckCommand.

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:387: other is VmBatchCommand &&
On 2016/12/06 10:39:05, kustermann wrote:
> The "other is VmBatchCommand" is unnecessary I think, because "operator=="
will
> take care of it.

Done.

I also added 'checked' to the comparison because I noticed it was missing
(although not strictly necessary at this time, it ought to be there).

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:300: }
On 2016/12/06 10:39:05, kustermann wrote:
> I'm not sure if this is always doing the right thing. There are several
> different cases of negative tests, some are "_negative_test.dart" (which can
be
> negative in compiler or runtime AFAIK) and multi tests annotations and co19
test
> annotations, sometimes they are only negative in checked mode.
> 
> Could we get rid of this? 
> 
> The test_suite.dart file is already very complex and this makes it even worse.
> 
> Why is it even needed in the first place?

If the test expects a compile-time error:
There is no data for the self checking unit test to consume, so there is nothing
to do.

If the test expects a runtime error:
The self_check runtime configuration does not execute the program, and the
expectations associated with the test case describe what should happen if the
code is executed, and are thus not relevant this runtime.

This could be handled in a more fine-grained way, but exactly because the code
is so unwieldy I have a preference for simplest solution which is to just ignore
the negative tests altogether. That way, the code for comparing test
expectations against concrete output does not get more complicated.

I don't really like this style either, but I think it's par for the course in
this codebase. Considering the functionality that was added I think it's a very
mild change.

The isNegative flag is the aggregate of many cases, e.g. in checked mode it
returns true for tests that are negative in checked mode. And this point is the
pretty much the first program point where this value has been computed, so it
cannot be filtered out sooner without refactoring/duplication.

https://codereview.chromium.org/2549793002/diff/60001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:1358: '$tempDir/$fileName.js', compiler,
tempDir, optionsFromFile));
On 2016/12/06 10:39:05, kustermann wrote:
> This is another path on how to compute compilation commands. Here we don't
> respect the --skip-compilations' flag.

I can't make it work for browsers.

[Bill Hesse, 2016-12-06 14:46:03]

LGTM.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/runt...
File tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/runt...
tools/testing/dart/runtime_configuration.dart:20: // test_options.dart.  We
probably want to store an instance of
Notice here he points out that the instance can even go into configuration, not
into the TestSuite instance.

Perhaps put into the comments that a single instance per test suite (or per
configuration) exists, so no mutable data should be changed in these instances
after construction.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/runt...
tools/testing/dart/runtime_configuration.dart:305: for (var entry in new
Directory('pkg').listSync(recursive: true)) {
Please use the test utils to specify the full path to 'pkg':

TestUtils.dartDirUri.resolve('pkg').toFilePath()
or
TestUtils.dartDir.append('pkg').toNativePath()
instead of just 'pkg'.

also show TestUtils from test_suite.
We shouldn't assume the current directory.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
tools/testing/dart/test_options.dart:490: 'Skip the compilation step, using the
output from the previous run.',
I don't understand what "using the output from the previous run" means.  Please
write more.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2691: .runCommand(name, command, timeout,
command.arguments);
We only create a bounded number of batch runners per key (the argument to
_getBatchRunner), so if there were too many different dartFile values, that may
cause a problem with having too many batch runners running, and overloading the
hardware.

[asgerf, 2016-12-06 15:41:49]

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/runt...
File tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/runt...
tools/testing/dart/runtime_configuration.dart:20: // test_options.dart.  We
probably want to store an instance of
On 2016/12/06 14:46:03, Bill Hesse wrote:
> Notice here he points out that the instance can even go into configuration,
not
> into the TestSuite instance.
> 
> Perhaps put into the comments that a single instance per test suite (or per
> configuration) exists, so no mutable data should be changed in these instances
> after construction.

Good point. Done.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/runt...
tools/testing/dart/runtime_configuration.dart:305: for (var entry in new
Directory('pkg').listSync(recursive: true)) {
On 2016/12/06 14:46:03, Bill Hesse wrote:
> Please use the test utils to specify the full path to 'pkg':
> 
> TestUtils.dartDirUri.resolve('pkg').toFilePath()
> or
> TestUtils.dartDir.append('pkg').toNativePath()
> instead of just 'pkg'.
> 
> also show TestUtils from test_suite.
> We shouldn't assume the current directory.

Done.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
tools/testing/dart/test_options.dart:490: 'Skip the compilation step, using the
output from the previous run.',
On 2016/12/06 14:46:03, Bill Hesse wrote:
> I don't understand what "using the output from the previous run" means. 
Please
> write more.

I've added some more text.

If this flag is a serious grief for you I can take out again. I just found it
useful.

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2549793002/diff/80001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2691: .runCommand(name, command, timeout,
command.arguments);
On 2016/12/06 14:46:03, Bill Hesse wrote:
> We only create a bounded number of batch runners per key (the argument to
> _getBatchRunner), so if there were too many different dartFile values, that
may
> cause a problem with having too many batch runners running, and overloading
the
> hardware.

I'm aware it doesn't scale well to a large number of checkers, also because all
checker are bundled into the same test case.

I expect there will be a few more checker files, but not that many, and they can
easily be bundled to side-step the issue. Adding a --self-checker-pattern option
to select specific checkers would also help.

[kustermann, 2016-12-09 12:22:52]

@asger: Are you waiting for another lgtm?

I think you should just land it.

[asgerf, 2016-12-15 12:07:08]

Description was changed from

==========
Add 'self_check' runtime for running self-checking unit tests

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rself_check language co19

This will search the pkg/ folder for files matching *_self_check.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

The 'self_check' runtime is not specifically tied to kernel,
although only kernel is using it at the moment.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All self-check tests are treated as the same test case. If one fails,
  the remaining self-check testers don't run for that input.
- There is no way to run a subset of the self-check tests, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

BUG=
==========

to

==========
Add 'self_check' runtime for running self-checking unit tests

Some of the kernel unit tests can now be run using:

  tools/test.py -cdartk -rself_check language co19

This will search the pkg/ folder for files matching *_self_check.dart
and run each program with the compiled output as argument. If there is
no compiler, the test case itself is given as argument. These testers
are always run in batch-mode.

This type of test has no expected output, but is intended to check
itself by testing that certain invariants are not violated while
processing the given data set.

The 'self_check' runtime is not specifically tied to kernel,
although only kernel is using it at the moment.

There is also a new option --skip-compilation which skips the
compiler step.  It doesn't interact nicely with the status files,
but can still be useful for a quick offline test.

Current limitations:
- All self-check tests are treated as the same test case. If one fails,
  the remaining self-check testers don't run for that input.
- There is no way to run a subset of the self-check tests, or filter
  them based on what compiler was used.
- Tests that are expected to fail in the compiler show up as
  errors when skipping compilation.

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

Committed:
https://github.com/dart-lang/sdk/commit/2e718613aa61b781d78533049df993fefd307b9d
==========

[asgerf, 2016-12-15 12:07:12]

Committed patchset #5 (id:100001) manually as
2e718613aa61b781d78533049df993fefd307b9d (presubmit successful).