Refactor and clean up the status file parsing code.

Refactor and clean up the status file parsing code.

- Make the parser less error tolerant. The expression parser used to
  ignore any unrecognized tokens, which means a status like
  "RuntimeError CompileError" (not the missing comma) was parsed as
  simply "RuntimeError", which seems bad. Now it reports an error.
  Fixed a couple of status files that thought they were setting statuses
  that they weren't (!).

- Separate out parsing a status file from applying the environment to
  determine which sections are active. This makes it possible to, for
  example, generate expectation sets for multiple environments without
  having to reparse each time.

- Simplify expression parsing. Remove set expressions since they weren't
  used for anything useful. A test's expectations are a simple
  comma-separated list and don't need anything beyond that. Merge
  Scanner and Tokenizer since the latter was a glorified function.

- Make more names private so that it's clearer what's used outside of
  various libraries.

- Generally modernize the style.

- Add *lots* of documentation.

Again, there should be no behavioral changes. I ran:

  ./tools/test.py -m release,debug -c none,dart2js,dart2analyzer -r none,vm,d8 corelib

Before and after the change and verified that the output was the same
(aside from timing).

BUG=
R=whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/0b7728da1bef08c1c1e092005d9fd8c8bff5fa6c

[Bob Nystrom, 2017-05-12 23:13:15]

rnystrom@google.com changed reviewers:
+ whesse@google.com

[Bob Nystrom, 2017-05-12 23:13:15]

This was therapeutic. Also, now I understand what the various expectations mean.
And the status file grammar. :)

[Bill Hesse, 2017-05-15 16:15:09]

lgtm

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/expectat...
File tools/testing/dart/expectation.dart (right):

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/expectat...
tools/testing/dart/expectation.dart:125: // TODO(rnystrom): Why not use timeout
for these?
Because they then use a process for the amount of time it takes to timeout, and
increase the running time of the test suite.  We should not have many (or any)
tests marked Timeout, they should be SkipSlow instead.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/expectat...
tools/testing/dart/expectation.dart:144: // not appear to be understood by
test.dart.
I don't know.  Please find the author who added it to the status files, and
assign to them.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
File tools/testing/dart/status_expression.dart (right):

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
tools/testing/dart/status_expression.dart:188: new
RegExp(r"^([()$\w\s,]|(\&\&)|(\|\|)|(\=\=)|(\!\=))+$");
I think you can remove ',' from the two regexps, if we aren't handling input
with ',' in it.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
tools/testing/dart/status_expression.dart:209: .allMatches(expression)
I had no idea that this was non-overlapping.  You learn something every day.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_f...
File tools/testing/dart/status_file.dart (right):

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_f...
tools/testing/dart/status_file.dart:105: List<Expectation>
_parseExpectations(String text) {
We do a lot of work in ExpectationSet to produce a single cached object for
identical sets.  We could also do something here to produce a single cached list
for identical lines, or use the cache in ExpectationSet (it exists by the time
this is called, but isn't passed in yet).

But this may not be worth it, because it is bounded by the size of the status
files, while the later results (Set<Expectation> from ExpectationSet) are stored
in TestCase objects, one for each test, and they live until the end of the
program.  The StatusFile objects created here may be dropped once the test
suites are finished creating all the test cases, so we don't need to optimize
their memory use as much.

This also applies to the paths in these lines, which are optimized to cached
regexps in ExpectationSet, but can be expected to be distinct here.

[kevmoo, 2017-05-15 21:06:47]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2017-05-15 21:06:48]

DB C/Q

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
File tools/testing/dart/status_expression.dart (right):

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
tools/testing/dart/status_expression.dart:6: abstract class Expression {
Possible? https://github.com/dart-lang/boolean_selector

[Bob Nystrom, 2017-05-15 22:42:22]

Description was changed from

==========
Refactor and clean up the status file parsing code.

- Make the parser less error tolerant. The expression parser used to
  ignore any unrecognized tokens, which means a status like
  "RuntimeError CompileError" (not the missing comma) was parsed as
  simply "RuntimeError", which seems bad. Now it reports an error.
  Fixed a couple of status files that thought they were setting statuses
  that they weren't (!).

- Separate out parsing a status file from applying the environment to
  determine which sections are active. This makes it possible to, for
  example, generate expectation sets for multiple environments without
  having to reparse each time.

- Simplify expression parsing. Remove set expressions since they weren't
  used for anything useful. A test's expectations are a simple
  comma-separated list and don't need anything beyond that. Merge
  Scanner and Tokenizer since the latter was a glorified function.

- Make more names private so that it's clearer what's used outside of
  various libraries.

- Generally modernize the style.

- Add *lots* of documentation.

Again, there should be no behavioral changes. I ran:

  ./tools/test.py -m release,debug -c none,dart2js,dart2analyzer -r none,vm,d8
corelib

Before and after the change and verified that the output was the same
(aside from timing).

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

to

==========
Refactor and clean up the status file parsing code.

- Make the parser less error tolerant. The expression parser used to
  ignore any unrecognized tokens, which means a status like
  "RuntimeError CompileError" (not the missing comma) was parsed as
  simply "RuntimeError", which seems bad. Now it reports an error.
  Fixed a couple of status files that thought they were setting statuses
  that they weren't (!).

- Separate out parsing a status file from applying the environment to
  determine which sections are active. This makes it possible to, for
  example, generate expectation sets for multiple environments without
  having to reparse each time.

- Simplify expression parsing. Remove set expressions since they weren't
  used for anything useful. A test's expectations are a simple
  comma-separated list and don't need anything beyond that. Merge
  Scanner and Tokenizer since the latter was a glorified function.

- Make more names private so that it's clearer what's used outside of
  various libraries.

- Generally modernize the style.

- Add *lots* of documentation.

Again, there should be no behavioral changes. I ran:

  ./tools/test.py -m release,debug -c none,dart2js,dart2analyzer -r none,vm,d8
corelib

Before and after the change and verified that the output was the same
(aside from timing).

BUG=
R=whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0b7728da1bef08c1c1e092005d9fd8c8bff5fa6c
==========

[Bob Nystrom, 2017-05-15 22:42:23]

Committed patchset #2 (id:20001) manually as
0b7728da1bef08c1c1e092005d9fd8c8bff5fa6c (presubmit successful).

[Bob Nystrom, 2017-05-15 22:42:51]

Thanks for the review!

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/expectat...
File tools/testing/dart/expectation.dart (right):

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/expectat...
tools/testing/dart/expectation.dart:125: // TODO(rnystrom): Why not use timeout
for these?
On 2017/05/15 16:15:08, Bill Hesse wrote:
> Because they then use a process for the amount of time it takes to timeout,
and
> increase the running time of the test suite.  We should not have many (or any)
> tests marked Timeout, they should be SkipSlow instead.

Good to know! Updated the comment.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/expectat...
tools/testing/dart/expectation.dart:144: // not appear to be understood by
test.dart.
On 2017/05/15 16:15:08, Bill Hesse wrote:
> I don't know.  Please find the author who added it to the status files, and
> assign to them.

Looks like a pkg/front_end/lib/src/fasta/testing thing. Added a comment.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
File tools/testing/dart/status_expression.dart (right):

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
tools/testing/dart/status_expression.dart:6: abstract class Expression {
On 2017/05/15 21:06:48, kevmoo wrote:
> Possible? https://github.com/dart-lang/boolean_selector

For test.dart, we try not to rely on any external dependencies since this is so
fundamental to the infrastructure. Also, switching this out might subtly change
the grammar in ways we don't expect.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
tools/testing/dart/status_expression.dart:188: new
RegExp(r"^([()$\w\s,]|(\&\&)|(\|\|)|(\=\=)|(\!\=))+$");
On 2017/05/15 16:15:08, Bill Hesse wrote:
> I think you can remove ',' from the two regexps, if we aren't handling input
> with ',' in it.

Nice catch! Done.

https://codereview.chromium.org/2875203005/diff/1/tools/testing/dart/status_e...
tools/testing/dart/status_expression.dart:209: .allMatches(expression)
On 2017/05/15 16:15:09, Bill Hesse wrote:
> I had no idea that this was non-overlapping.  You learn something every day.

Guess so. I did too! (This was in the original code as well, it just moved down
here.)