Refactor and clean up the status file parsing code. - Make the parser less error tolerant. The expr…

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).

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/92267afc6643ed1aa9e7958cb4e4262525353aaf

[Bob Nystrom, 2017-05-17 21:04:09]

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

[Bob Nystrom, 2017-05-17 21:04:09]

I guess there are actually some tests for test.dart, hiding under standalone/io.
Who knew?

Get them passing again.

The first patch is the same one previously committed, so you just need to look
at the second one.

[Bob Nystrom, 2017-05-18 17:10:22]

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

[Bob Nystrom, 2017-05-18 17:10:23]

Siggy, everything in the first patch was reviewed by Bill in a previous review.
The only change here is the test stuff in the second patch.

[Siggi Cherem (dart-lang), 2017-05-18 17:19:25]

lgtm

[Bob Nystrom, 2017-05-18 19:42:55]

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).
==========

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).

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/92267afc6643ed1aa9e7958cb4e4262525353aaf
==========

[Bob Nystrom, 2017-05-18 19:42:57]

Committed patchset #2 (id:20001) manually as
92267afc6643ed1aa9e7958cb4e4262525353aaf (presubmit successful).

[ahe, 2017-05-23 13:34:56]

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

[ahe, 2017-05-23 13:34:57]

https://codereview.chromium.org/2891753003/diff/20001/tools/testing/dart/expe...
File tools/testing/dart/expectation.dart (right):

https://codereview.chromium.org/2891753003/diff/20001/tools/testing/dart/expe...
tools/testing/dart/expectation.dart:148: new Expectation._('VerificationError');
Why do you need to parse these status files?

[Bob Nystrom, 2017-05-24 01:17:39]

On 2017/05/23 13:34:57, ahe wrote:
>
https://codereview.chromium.org/2891753003/diff/20001/tools/testing/dart/expe...
> File tools/testing/dart/expectation.dart (right):
> 
>
https://codereview.chromium.org/2891753003/diff/20001/tools/testing/dart/expe...
> tools/testing/dart/expectation.dart:148: new
Expectation._('VerificationError');
> Why do you need to parse these status files?

Good question!

When I was changing the code, I wrote a little test script that found every
.status file in the repo and tried to parse it. It stumbled on the files under
pkg/front_end/test/fasta and then failed when it hit an expectation it didn't
understand. (The old status file parser would only lazily look up expectations
by name where the new one is eager.)

I figured it made sense to be able to parse those even though it doesn't use the
expectation itself. If you think it will never need to handle those files, I can
remove this expectation type.

[ahe, 2017-05-24 09:35:14]

On 2017/05/24 01:17:39, Bob Nystrom wrote:
> On 2017/05/23 13:34:57, ahe wrote:
> >
>
https://codereview.chromium.org/2891753003/diff/20001/tools/testing/dart/expe...
> > File tools/testing/dart/expectation.dart (right):
> > 
> >
>
https://codereview.chromium.org/2891753003/diff/20001/tools/testing/dart/expe...
> > tools/testing/dart/expectation.dart:148: new
> Expectation._('VerificationError');
> > Why do you need to parse these status files?
> 
> Good question!
> 
> When I was changing the code, I wrote a little test script that found every
> .status file in the repo and tried to parse it. It stumbled on the files under
> pkg/front_end/test/fasta and then failed when it hit an expectation it didn't
> understand. (The old status file parser would only lazily look up expectations
> by name where the new one is eager.)
> 
> I figured it made sense to be able to parse those even though it doesn't use
the
> expectation itself. If you think it will never need to handle those files, I
can
> remove this expectation type.

That makes sense. Perhaps you want to look at how I've made status markers
configurable in package:testing.

See ExpectationSet.fromJsonList in pkg/testing/lib/src/expectation.dart and
pkg/front_end/test/fasta/testing/suite.dart for an example of how
VerificationError is specified by "user" code. In
pkg/front_end/lib/src/fasta/testing/kernel_chain.dart, you can find an example
of how the error is reported back to the package:testing framework without it
being defined by package:testing.

My hope with package:testing is that we can move in a direction where our test
runner can be configured externally instead of relying on everyone adding
extensions to tools/testing/dart/, including status markers.

[Bob Nystrom, 2017-05-24 23:41:14]

On 2017/05/24 09:35:14, ahe wrote:
> That makes sense. Perhaps you want to look at how I've made status markers
> configurable in package:testing.

I have looked the docs for package:testing. It looks nice!

> My hope with package:testing is that we can move in a direction where our test
> runner can be configured externally instead of relying on everyone adding
> extensions to tools/testing/dart/, including status markers.

I don't have a strong preference either way. I do like that configuring them in
code gives all of the benefits of static analysis. But, of course, hacking on
test.dart is currently not very easy since the codebase hasn't been kept up to
date. I'm hoping to make that somewhat better, since it is currently a piece of
infrastructure that we heavily rely on. If it ends up being replaced entirely by
package:testing, I wouldn't mind. But, until then, I figure it's worth keeping
it well-maintained.

– bob

[ahe, 2017-06-01 14:04:01]

On 2017/05/24 23:41:14, Bob Nystrom wrote:
> On 2017/05/24 09:35:14, ahe wrote:
> > That makes sense. Perhaps you want to look at how I've made status markers
> > configurable in package:testing.
> 
> I have looked the docs for package:testing. It looks nice!
> 
> > My hope with package:testing is that we can move in a direction where our
test
> > runner can be configured externally instead of relying on everyone adding
> > extensions to tools/testing/dart/, including status markers.
> 
> I don't have a strong preference either way. I do like that configuring them
in
> code gives all of the benefits of static analysis. But, of course, hacking on
> test.dart is currently not very easy since the codebase hasn't been kept up to
> date. I'm hoping to make that somewhat better, since it is currently a piece
of
> infrastructure that we heavily rely on. If it ends up being replaced entirely
by
> package:testing, I wouldn't mind. But, until then, I figure it's worth keeping
> it well-maintained.

I share your preference for static analysis, so one of the first features I
added to package:testing was that it automatically runs the analyzer on all
plugins and tests you have specified in testing.json.