Introduce a testing framework for use with fasta type inference.

Introduce a testing framework for use with fasta type inference.

The framework will allow test source files to be annotated with
comments indicating where types are expected to be inferred, for
example:

    var /*@type=dart.core::int*/ x = 1;

Other features of the testing framework:

- Normally only those expectations explicitly listed will be checked.
  The special comment "/*@testedFeatures=inference*/" at the top of
  the file turns on extra checking which will cause the test to fail
  if any unexpected type inference occurs.

- Should be easy to extend to support other features, such as checking
  that the proper errors and warnings are generated at the proper
  locations.

- If the test is run with "-DupdateExpectations=true", the
  expectations will be updated in place.

R=ahe@google.com

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

[Paul Berry, 2017-04-19 09:33:38]

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

[ahe, 2017-04-19 11:38:30]

lgtm

Only nits below.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/instrumentation.dart (right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/instrumentation.dart:23: String canonicalize();
Is this a potential gotcha? Perhaps we should just extend the specification of
toString?

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart
(right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:1: import
'dart:async';
Missing copyright.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:14: /// Map
from category names to the property names they are short for.
Here and several other places below: consider using the words "feature" and
"features" instead of "category" and "categories".

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:27: final
_expectations = <Uri, Map<int, List<_Expectation>>>{};
Consider renaming to unsatisfiedExpecations.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:77: //
Remove the trailing \0 that's added by readBytesFromFile.
We probably need to add boolean argument to readBytesFromFile to disable the
trailing \0. This is the second time we've had to do this recently.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:79:
fixes.sort((a, b) => b.offset.compareTo(a.offset));
I assume you sort them in reverse order to avoid having to adjust offsets below.
Perhaps add a comment about that?

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:98:
commentToken = commentToken.next) {
Perhaps we should add a forEachComment to ScannerResult?

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:113: }
I suggest adding:

property = property.trim();
value = value.trim();

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:116: for
(String category in value.split(',')) {
And here:

category = category.trim();

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:174: return
'$uri:$offset: $desc${stackTrace == null ? '' : '\n$stackTrace'}';
Perhaps you can use format from ../messages.dart here.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:188: throw
null;
We now have StackTrace.current.

[ahe, 2017-04-19 13:11:00]

FYI

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart
(right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:77: //
Remove the trailing \0 that's added by readBytesFromFile.
On 2017/04/19 11:38:29, ahe wrote:
> We probably need to add boolean argument to readBytesFromFile to disable the
> trailing \0. This is the second time we've had to do this recently.

I've done that in CL 2827543006.

[Paul Berry, 2017-04-19 13:20:10]

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/instrumentation.dart (right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/instrumentation.dart:23: String canonicalize();
On 2017/04/19 11:38:29, ahe wrote:
> Is this a potential gotcha? Perhaps we should just extend the specification of
> toString?

Good point.  Fixed.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart
(right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:1: import
'dart:async';
On 2017/04/19 11:38:29, ahe wrote:
> Missing copyright.

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:14: /// Map
from category names to the property names they are short for.
On 2017/04/19 11:38:29, ahe wrote:
> Here and several other places below: consider using the words "feature" and
> "features" instead of "category" and "categories".

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:27: final
_expectations = <Uri, Map<int, List<_Expectation>>>{};
On 2017/04/19 11:38:29, ahe wrote:
> Consider renaming to unsatisfiedExpecations.

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:77: //
Remove the trailing \0 that's added by readBytesFromFile.
On 2017/04/19 13:10:59, ahe wrote:
> On 2017/04/19 11:38:29, ahe wrote:
> > We probably need to add boolean argument to readBytesFromFile to disable the
> > trailing \0. This is the second time we've had to do this recently.
> 
> I've done that in CL 2827543006.

Thanks!  I'll wait until you land that and then clean this up.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:79:
fixes.sort((a, b) => b.offset.compareTo(a.offset));
On 2017/04/19 11:38:29, ahe wrote:
> I assume you sort them in reverse order to avoid having to adjust offsets
below.
> Perhaps add a comment about that?

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:98:
commentToken = commentToken.next) {
On 2017/04/19 11:38:29, ahe wrote:
> Perhaps we should add a forEachComment to ScannerResult?

Fair enough.  I will do that as a follow-up CL.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:113: }
On 2017/04/19 11:38:29, ahe wrote:
> I suggest adding:
> 
> property = property.trim();
> value = value.trim();

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:116: for
(String category in value.split(',')) {
On 2017/04/19 11:38:29, ahe wrote:
> And here:
> 
> category = category.trim();

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:174: return
'$uri:$offset: $desc${stackTrace == null ? '' : '\n$stackTrace'}';
On 2017/04/19 11:38:29, ahe wrote:
> Perhaps you can use format from ../messages.dart here.

Done.

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:188: throw
null;
On 2017/04/19 11:38:29, ahe wrote:
> We now have StackTrace.current.

Done.

[Paul Berry, 2017-04-19 13:53:55]

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart
(right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:77: //
Remove the trailing \0 that's added by readBytesFromFile.
On 2017/04/19 13:20:09, Paul Berry wrote:
> On 2017/04/19 13:10:59, ahe wrote:
> > On 2017/04/19 11:38:29, ahe wrote:
> > > We probably need to add boolean argument to readBytesFromFile to disable
the
> > > trailing \0. This is the second time we've had to do this recently.
> > 
> > I've done that in CL 2827543006.
> 
> Thanks!  I'll wait until you land that and then clean this up.

Done.

[Paul Berry, 2017-04-19 13:54:16]

Description was changed from

==========
Introduce a testing framework for use with fasta type inference.

The framework will allow test source files to be annotated with
comments indicating where types are expected to be inferred, for
example:

    var /*@type=dart.core::int*/ x = 1;

Other features of the testing framework:

- Normally only those expectations explicitly listed will be checked.
  The special comment "/*@testedFeatures=inference*/" at the top of
  the file turns on extra checking which will cause the test to fail
  if any unexpected type inference occurs.

- Should be easy to extend to support other features, such as checking
  that the proper errors and warnings are generated at the proper
  locations.

- If the test is run with "-DupdateExpectations=true", the
  expectations will be updated in place.
==========

to

==========
Introduce a testing framework for use with fasta type inference.

The framework will allow test source files to be annotated with
comments indicating where types are expected to be inferred, for
example:

    var /*@type=dart.core::int*/ x = 1;

Other features of the testing framework:

- Normally only those expectations explicitly listed will be checked.
  The special comment "/*@testedFeatures=inference*/" at the top of
  the file turns on extra checking which will cause the test to fail
  if any unexpected type inference occurs.

- Should be easy to extend to support other features, such as checking
  that the proper errors and warnings are generated at the proper
  locations.

- If the test is run with "-DupdateExpectations=true", the
  expectations will be updated in place.

R=ahe@google.com

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

[Paul Berry, 2017-04-19 13:54:17]

Committed patchset #1 (id:1) manually as
a682c6a97ea14dbef3f196973271dab0a678c0ca (presubmit successful).

[Paul Berry, 2017-04-19 15:31:21]

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart
(right):

https://codereview.chromium.org/2824393002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/validating_instrumentation.dart:98:
commentToken = commentToken.next) {
On 2017/04/19 13:20:10, Paul Berry wrote:
> On 2017/04/19 11:38:29, ahe wrote:
> > Perhaps we should add a forEachComment to ScannerResult?
> 
> Fair enough.  I will do that as a follow-up CL.

I looked into this and it's uglier than I expected, because we actually need to
know both the comment token and the non-comment token it's attached to.  Which
means the signature of ScannerResult.forEachComment would have to be something
like:

void forEachComment(void callback(Token commentToken, Token anchorToken));

IMHO it's not worth it so I'm going to leave it as is.