make checker_test, inferred_type_test easier to debug

make checker_test, inferred_type_test easier to debug

[Jennifer Messerly, 2016-01-26 00:05:33]

jmesserly@google.com changed reviewers:
+ rnystrom@google.com, vsm@google.com

[Jennifer Messerly, 2016-01-26 00:05:33]

Thoughts?

Formatting is a bit odd, but it actually helps us here, as you can put a
breakpoint on the () => line.

Open to other ideas to restructure these. In the short term I wanted to make it
easier to put a breakpoint on when the test is called.

[Paul Berry, 2016-02-05 15:23:16]

DBC

Note: you may also want to explore using the "reflectiveTest" capability in
pkg/analyzer/test/reflective_tests.dart; we use this in most of the other
analyzer tests to allow us to write our tests in JUnit style, with state stored
in class members, helper methods in a base class, and setup/teardown in
setUp/tearDown methods (which can also be in a shared base class).

With reflectiveTest it's a little easier to re-use code in a free-form way,
because rather than create a big data structure and pass it to a helper function
that's prepared for anything, you can make use of a lot of little helper
methods.  And those little helper methods are easier to navigate around in your
IDE.  For example, the tests affected by this CL might wind up looking something
like this:

main() {
  runReflectiveTests(CheckerTests);
}

abstract class StrongTest {
  AnalysisContext context;
  MemoryResourceProvider provider;
  ... more state ...

  void setUp() {
    ... initialize [context], [provider], etc.
  }

  void addFile(String path, String text) {
    var scheme = 'package:';
    if (path.startsWith(scheme)) {
      path = '/packages/${path.substring(scheme.length)}';
    }
    provider.newFile(path, text);
  }

  void check(String mainText) {
    addFile('/main.dart', mainText);
    ... generic test logic ...
  }
}

@reflectiveTest
class CheckerTests extends StrongTest {
  // Methods starting with "test_" are automatically run
  test_ternaryOperator() {
    check('''
      abstract class Comparable<T> {
      ...
    ''');
  }

  test_conversionAndDynamicInvoke() {
    addFile('/helper.dart', '''
      dynamic toString = (int x) => x + 42;
      dynamic hashCode = "hello";
      ''');
    check('''
      import 'helper.dart' as helper;
      ...
      ''');
  }
}

https://codereview.chromium.org/1632113002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/1632113002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:15: testChecker(
Nit: consider modifying testChecker so that it doesn't call test() directly, so
that your test code looks like this:

test('ternary operator', () =>
  testChecker({
    '/main.dart': '''
      abstract class Comparable<T> {
      ...
    '''
  }));

It's a little more typing than your version, but it has the advantage that if
the test fails, the call to testChecker will be in the stack trace, so you can
jump easily to the failing test.

[Jennifer Messerly, 2016-02-05 17:38:10]

On 2016/02/05 15:23:16, Paul Berry wrote:
> DBC
> 
> Note: you may also want to explore using the "reflectiveTest" capability in
> pkg/analyzer/test/reflective_tests.dart; we use this in most of the other
> analyzer tests to allow us to write our tests in JUnit style, with state
stored
> in class members, helper methods in a base class, and setup/teardown in
> setUp/tearDown methods (which can also be in a shared base class).
> 
> With reflectiveTest it's a little easier to re-use code in a free-form way,
> because rather than create a big data structure and pass it to a helper
function
> that's prepared for anything, you can make use of a lot of little helper
> methods.  And those little helper methods are easier to navigate around in
your
> IDE.  For example, the tests affected by this CL might wind up looking
something
> like this:
> 
> main() {
>   runReflectiveTests(CheckerTests);
> }
> 
> abstract class StrongTest {
>   AnalysisContext context;
>   MemoryResourceProvider provider;
>   ... more state ...
> 
>   void setUp() {
>     ... initialize [context], [provider], etc.
>   }
> 
>   void addFile(String path, String text) {
>     var scheme = 'package:';
>     if (path.startsWith(scheme)) {
>       path = '/packages/${path.substring(scheme.length)}';
>     }
>     provider.newFile(path, text);
>   }
> 
>   void check(String mainText) {
>     addFile('/main.dart', mainText);
>     ... generic test logic ...
>   }
> }
> 
> @reflectiveTest
> class CheckerTests extends StrongTest {
>   // Methods starting with "test_" are automatically run
>   test_ternaryOperator() {
>     check('''
>       abstract class Comparable<T> {
>       ...
>     ''');
>   }
> 
>   test_conversionAndDynamicInvoke() {
>     addFile('/helper.dart', '''
>       dynamic toString = (int x) => x + 42;
>       dynamic hashCode = "hello";
>       ''');
>     check('''
>       import 'helper.dart' as helper;
>       ...
>       ''');
>   }
> }
> 
>
https://codereview.chromium.org/1632113002/diff/1/pkg/analyzer/test/src/task/...
> File pkg/analyzer/test/src/task/strong/checker_test.dart (right):
> 
>
https://codereview.chromium.org/1632113002/diff/1/pkg/analyzer/test/src/task/...
> pkg/analyzer/test/src/task/strong/checker_test.dart:15: testChecker(
> Nit: consider modifying testChecker so that it doesn't call test() directly,
so
> that your test code looks like this:
> 
> test('ternary operator', () =>
>   testChecker({
>     '/main.dart': '''
>       abstract class Comparable<T> {
>       ...
>     '''
>   }));
> 
> It's a little more typing than your version, but it has the advantage that if
> the test fails, the call to testChecker will be in the stack trace, so you can
> jump easily to the failing test.

Yeah, definitely should pull "test" outside of "testChecker". That seems like
the easy fix. Another thing I wanted to do was get rid of the "/main.dart"
everywhere. I like "addFile" there.

Reflective test is an interesting idea. Is it already a package on Pub--or would
it be a bad idea to make it one? One thing I miss with Dart tests is IDE
integration, where it understands what test cases are run & can let you run them
on their own. Reflective tests tend to be more friendly for that (IDE can
statically find a particular test, instead of running a program that dynamically
constructs a test data structure.)

[Jennifer Messerly, 2016-02-05 17:40:01]

On 2016/02/05 17:38:10, John Messerly wrote:
> On 2016/02/05 15:23:16, Paul Berry wrote:
> > DBC
> > 
> > Note: you may also want to explore using the "reflectiveTest" capability in
> > pkg/analyzer/test/reflective_tests.dart; we use this in most of the other
> > analyzer tests to allow us to write our tests in JUnit style, with state
> stored
> > in class members, helper methods in a base class, and setup/teardown in
> > setUp/tearDown methods (which can also be in a shared base class).
> > 
> > With reflectiveTest it's a little easier to re-use code in a free-form way,
> > because rather than create a big data structure and pass it to a helper
> function
> > that's prepared for anything, you can make use of a lot of little helper
> > methods.  And those little helper methods are easier to navigate around in
> your
> > IDE.  For example, the tests affected by this CL might wind up looking
> something
> > like this:
> > 
> > main() {
> >   runReflectiveTests(CheckerTests);
> > }
> > 
> > abstract class StrongTest {
> >   AnalysisContext context;
> >   MemoryResourceProvider provider;
> >   ... more state ...
> > 
> >   void setUp() {
> >     ... initialize [context], [provider], etc.
> >   }
> > 
> >   void addFile(String path, String text) {
> >     var scheme = 'package:';
> >     if (path.startsWith(scheme)) {
> >       path = '/packages/${path.substring(scheme.length)}';
> >     }
> >     provider.newFile(path, text);
> >   }
> > 
> >   void check(String mainText) {
> >     addFile('/main.dart', mainText);
> >     ... generic test logic ...
> >   }
> > }
> > 
> > @reflectiveTest
> > class CheckerTests extends StrongTest {
> >   // Methods starting with "test_" are automatically run
> >   test_ternaryOperator() {
> >     check('''
> >       abstract class Comparable<T> {
> >       ...
> >     ''');
> >   }
> > 
> >   test_conversionAndDynamicInvoke() {
> >     addFile('/helper.dart', '''
> >       dynamic toString = (int x) => x + 42;
> >       dynamic hashCode = "hello";
> >       ''');
> >     check('''
> >       import 'helper.dart' as helper;
> >       ...
> >       ''');
> >   }
> > }
> > 
> >
>
https://codereview.chromium.org/1632113002/diff/1/pkg/analyzer/test/src/task/...
> > File pkg/analyzer/test/src/task/strong/checker_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/1632113002/diff/1/pkg/analyzer/test/src/task/...
> > pkg/analyzer/test/src/task/strong/checker_test.dart:15: testChecker(
> > Nit: consider modifying testChecker so that it doesn't call test() directly,
> so
> > that your test code looks like this:
> > 
> > test('ternary operator', () =>
> >   testChecker({
> >     '/main.dart': '''
> >       abstract class Comparable<T> {
> >       ...
> >     '''
> >   }));
> > 
> > It's a little more typing than your version, but it has the advantage that
if
> > the test fails, the call to testChecker will be in the stack trace, so you
can
> > jump easily to the failing test.
> 
> Yeah, definitely should pull "test" outside of "testChecker". That seems like
> the easy fix. Another thing I wanted to do was get rid of the "/main.dart"
> everywhere. I like "addFile" there.
> 
> Reflective test is an interesting idea. Is it already a package on Pub--or
would
> it be a bad idea to make it one? One thing I miss with Dart tests is IDE
> integration, where it understands what test cases are run & can let you run
them
> on their own. Reflective tests tend to be more friendly for that (IDE can
> statically find a particular test, instead of running a program that
dynamically
> constructs a test data structure.)

In the meantime I'm going to drop this CL (easier to drop it then merge), but
will resend to you & Bob, with these ideas incorporated. Thanks a lot for the
suggestions, I really like how that restructured version looks. :)