Start adding unit tests for ClassHierarchy.

Start adding unit tests for ClassHierarchy.

I need these tests in order to be sure that I understand expectations
of the ClassHierarchy interface, and to be able to test a lazy
implementation I'm going to create for incremental kernel generator.

I moved lub_test tests into this wider test suite.

Tests for forEachOverridePair() are not complete yet, just one path
is tested.

If there are any additional cases that you think should be covered,
please let me know.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Paul Berry, 2017-05-30 23:31:41]

Logic lgtm.  However since this is kernel code, we should wait for approval from
Kevin.

The use of test_reflective_loader may be controversial.  I had a chat with Siggi
about this and he's going to write up some notes about it in his review.

[Siggi Cherem (dart-lang), 2017-05-31 00:32:16]

I added some context about test_reflective_loader below.

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
File pkg/kernel/test/class_hierarchy_test.dart (right):

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
pkg/kernel/test/class_hierarchy_test.dart:10: import
'package:test_reflective_loader/test_reflective_loader.dart';
note: this needs to be added as a dev_dependency in the pubspec as well

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
pkg/kernel/test/class_hierarchy_test.dart:10: import
'package:test_reflective_loader/test_reflective_loader.dart';
Kevin - you may note that there are a lot of structural changes in this test, so
I wanted to highlight a bit the background behind it. Hopefully this should give
you enough context to decide whether you'd like to have the reflective-loader
dependency. This package is used heavily by the analyzer team today and we use
it in several places in front_end too.

Konstantin needs an easy way to run these tests with 2 implementations of
ClassHierarcy (the current one, and the lazy one he is creating), reuse as much
of the helper functions and test logic for both, and have an easy way to mark
some subset of the tests as failing on one or the other implementation.

There are many ways to do that. This CL adopts the following solution:
 - switch from top-level methods with helper functions, to use a class with
helper methods:
     - subclasses define how to create ClassHierarchy implementations
     - the class structure helps share state and avoid passing stuff around
between helper functions.

 - convert each test into an explicit method, so they can be overriden when they
fail on a specific implementation (e.g. because it is work in progress)

 - use test_reflective_loader to:
     - automatically detect these test methods, rather than calling them by hand
     - mark failing tests quickly via @fail annotations

The dependency on `test_reflective_loader` is optional, but helps make some of
the patterns below easier to maintain.


To highlight the differences, here is what a simple test for 2 implementations
would look like w/o the reflective-loader:

  main() {
    group('impl1', new Impl1Test().run);
    group('impl2', new Impl2Test().run);
  }

  class _ClassHierarchyTest {
    ClassHierarchy _hierarchy;
    ClassHierarchy get hierarchy  => _hierarchy ??= createClassHierarchy();

    ... // helper methods, setup/teardown

    test_a1() {
      ...  // the body of a test
    }

    test_a2() {
      ...  // the body of a test
    }

    run() {
      test('test description a1', test_a1);  // this is basically boilerplate
      test('test description a2', test_a2);
    }
  }

  class Impl1Test extends _ClassHierarchyTest {
    createClassHierarchy() => new Impl1();
  }


  class Impl2Test extends _ClassHierarchyTest {
    createClassHierarchy() => new Impl2();
    test_a1() {
      // TODO: this test is failing in Impl2.
      expect(() => super.test_a1(), throws);
    }
  }




And this is more or less what it looks like with `test_reflective_loader`:

  main() {
    defineReflectiveSuite(() => defineReflectiveTests(Impl1Test));
    defineReflectiveSuite(() => defineReflectiveTests(Impl2Test));
  }

  class _ClassHierarchyTest {
    ClassHierarchy _hierarchy;
    ClassHierarchy get hierarchy  => _hierarchy ??= createClassHierarchy();

    ... // helper methods, setup/teardown

    test_a1() {
      ...  // the body of a test
    }

    test_a2() {
      ...  // the body of a test
    }

    // Note: `run` is gone.
  }

  @reflectiveTest
  class Impl1Test extends _ClassHierarchyTest {
    createClassHierarchy() => new Impl1();
  }

  @reflectiveTest
  class Impl2Test extends _ClassHierarchyTest {
    createClassHierarchy() => new Impl2();

    @fail // you can mark failing tests with metadata
    test_a1() => super.test_a1();
  }




Some known drawbacks of this package are:
 - as the name implies, it depends on dart:mirrors. It uses @MirrorsUsed, so
dart2js can still run these tests effectively (under some but not all
configurations).

 - it requires that you follow the class structure above. In this case it was
pretty useful to use that structure, but not all tests need it, so if you adopt
it more widely, it might feel like boiler plate having to write tests in this
style.


Finally - if you prefer that we don't add the dependency, let us know if the
pattern above would be acceptable.

[ahe, 2017-05-31 11:23:47]

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
File pkg/kernel/test/class_hierarchy_test.dart (right):

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
pkg/kernel/test/class_hierarchy_test.dart:10: import
'package:test_reflective_loader/test_reflective_loader.dart';
On 2017/05/31 00:32:16, Siggi Cherem (dart-lang) wrote:
> Kevin - you may note that there are a lot of structural changes in this test,
so
> I wanted to highlight a bit the background behind it. Hopefully this should
give
> you enough context to decide whether you'd like to have the reflective-loader
> dependency. This package is used heavily by the analyzer team today and we use
> it in several places in front_end too.
> 
> Konstantin needs an easy way to run these tests with 2 implementations of
> ClassHierarcy (the current one, and the lazy one he is creating), reuse as
much
> of the helper functions and test logic for both, and have an easy way to mark
> some subset of the tests as failing on one or the other implementation.
> 
> There are many ways to do that. This CL adopts the following solution:
>  - switch from top-level methods with helper functions, to use a class with
> helper methods:
>      - subclasses define how to create ClassHierarchy implementations
>      - the class structure helps share state and avoid passing stuff around
> between helper functions.
> 
>  - convert each test into an explicit method, so they can be overriden when
they
> fail on a specific implementation (e.g. because it is work in progress)
> 
>  - use test_reflective_loader to:
>      - automatically detect these test methods, rather than calling them by
hand
>      - mark failing tests quickly via @fail annotations
> 
> The dependency on `test_reflective_loader` is optional, but helps make some of
> the patterns below easier to maintain.
> 
> 
> To highlight the differences, here is what a simple test for 2 implementations
> would look like w/o the reflective-loader:
> 
>   main() {
>     group('impl1', new Impl1Test().run);
>     group('impl2', new Impl2Test().run);
>   }
> 
>   class _ClassHierarchyTest {
>     ClassHierarchy _hierarchy;
>     ClassHierarchy get hierarchy  => _hierarchy ??= createClassHierarchy();
> 
>     ... // helper methods, setup/teardown
> 
>     test_a1() {
>       ...  // the body of a test
>     }
> 
>     test_a2() {
>       ...  // the body of a test
>     }
> 
>     run() {
>       test('test description a1', test_a1);  // this is basically boilerplate
>       test('test description a2', test_a2);
>     }
>   }
> 
>   class Impl1Test extends _ClassHierarchyTest {
>     createClassHierarchy() => new Impl1();
>   }
> 
> 
>   class Impl2Test extends _ClassHierarchyTest {
>     createClassHierarchy() => new Impl2();
>     test_a1() {
>       // TODO: this test is failing in Impl2.
>       expect(() => super.test_a1(), throws);
>     }
>   }
> 
> 
> 
> 
> And this is more or less what it looks like with `test_reflective_loader`:
> 
>   main() {
>     defineReflectiveSuite(() => defineReflectiveTests(Impl1Test));
>     defineReflectiveSuite(() => defineReflectiveTests(Impl2Test));
>   }
> 
>   class _ClassHierarchyTest {
>     ClassHierarchy _hierarchy;
>     ClassHierarchy get hierarchy  => _hierarchy ??= createClassHierarchy();
> 
>     ... // helper methods, setup/teardown
> 
>     test_a1() {
>       ...  // the body of a test
>     }
> 
>     test_a2() {
>       ...  // the body of a test
>     }
> 
>     // Note: `run` is gone.
>   }
> 
>   @reflectiveTest
>   class Impl1Test extends _ClassHierarchyTest {
>     createClassHierarchy() => new Impl1();
>   }
> 
>   @reflectiveTest
>   class Impl2Test extends _ClassHierarchyTest {
>     createClassHierarchy() => new Impl2();
> 
>     @fail // you can mark failing tests with metadata
>     test_a1() => super.test_a1();
>   }
> 
> 
> 
> 
> Some known drawbacks of this package are:
>  - as the name implies, it depends on dart:mirrors. It uses @MirrorsUsed, so
> dart2js can still run these tests effectively (under some but not all
> configurations).
> 
>  - it requires that you follow the class structure above. In this case it was
> pretty useful to use that structure, but not all tests need it, so if you
adopt
> it more widely, it might feel like boiler plate having to write tests in this
> style.
> 
> 
> Finally - if you prefer that we don't add the dependency, let us know if the
> pattern above would be acceptable.

I'm not a big fan of the reflective test suite and I think there are easier ways
to share test functionality. It seems to me that package:test_reflective_loader
is designed to make it easy to port JUnit tests to Dart and doesn't need to be
used for new tests.

[scheglov, 2017-05-31 15:43:06]

If there are any comments about logic, or about TODO for getDispatchTarget() and
abstract classes / members, please make these comments. We can always change
reflection style with explicit code if we decide to. But given that it is the
end of the working day for you, and the CL has not been reviewed by anyone in
Europe, this effectively stops my work on future ClassHierarchy tests.

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
File pkg/kernel/test/class_hierarchy_test.dart (right):

https://codereview.chromium.org/2912173002/diff/1/pkg/kernel/test/class_hiera...
pkg/kernel/test/class_hierarchy_test.dart:10: import
'package:test_reflective_loader/test_reflective_loader.dart';
On 2017/05/31 11:23:47, ahe wrote:
> On 2017/05/31 00:32:16, Siggi Cherem (dart-lang) wrote:
> > Kevin - you may note that there are a lot of structural changes in this
test,
> so
> > I wanted to highlight a bit the background behind it. Hopefully this should
> give
> > you enough context to decide whether you'd like to have the
reflective-loader
> > dependency. This package is used heavily by the analyzer team today and we
use
> > it in several places in front_end too.
> > 
> > Konstantin needs an easy way to run these tests with 2 implementations of
> > ClassHierarcy (the current one, and the lazy one he is creating), reuse as
> much
> > of the helper functions and test logic for both, and have an easy way to
mark
> > some subset of the tests as failing on one or the other implementation.
> > 
> > There are many ways to do that. This CL adopts the following solution:
> >  - switch from top-level methods with helper functions, to use a class with
> > helper methods:
> >      - subclasses define how to create ClassHierarchy implementations
> >      - the class structure helps share state and avoid passing stuff around
> > between helper functions.
> > 
> >  - convert each test into an explicit method, so they can be overriden when
> they
> > fail on a specific implementation (e.g. because it is work in progress)
> > 
> >  - use test_reflective_loader to:
> >      - automatically detect these test methods, rather than calling them by
> hand
> >      - mark failing tests quickly via @fail annotations
> > 
> > The dependency on `test_reflective_loader` is optional, but helps make some
of
> > the patterns below easier to maintain.
> > 
> > 
> > To highlight the differences, here is what a simple test for 2
implementations
> > would look like w/o the reflective-loader:
> > 
> >   main() {
> >     group('impl1', new Impl1Test().run);
> >     group('impl2', new Impl2Test().run);
> >   }
> > 
> >   class _ClassHierarchyTest {
> >     ClassHierarchy _hierarchy;
> >     ClassHierarchy get hierarchy  => _hierarchy ??= createClassHierarchy();
> > 
> >     ... // helper methods, setup/teardown
> > 
> >     test_a1() {
> >       ...  // the body of a test
> >     }
> > 
> >     test_a2() {
> >       ...  // the body of a test
> >     }
> > 
> >     run() {
> >       test('test description a1', test_a1);  // this is basically
boilerplate
> >       test('test description a2', test_a2);
> >     }
> >   }
> > 
> >   class Impl1Test extends _ClassHierarchyTest {
> >     createClassHierarchy() => new Impl1();
> >   }
> > 
> > 
> >   class Impl2Test extends _ClassHierarchyTest {
> >     createClassHierarchy() => new Impl2();
> >     test_a1() {
> >       // TODO: this test is failing in Impl2.
> >       expect(() => super.test_a1(), throws);
> >     }
> >   }
> > 
> > 
> > 
> > 
> > And this is more or less what it looks like with `test_reflective_loader`:
> > 
> >   main() {
> >     defineReflectiveSuite(() => defineReflectiveTests(Impl1Test));
> >     defineReflectiveSuite(() => defineReflectiveTests(Impl2Test));
> >   }
> > 
> >   class _ClassHierarchyTest {
> >     ClassHierarchy _hierarchy;
> >     ClassHierarchy get hierarchy  => _hierarchy ??= createClassHierarchy();
> > 
> >     ... // helper methods, setup/teardown
> > 
> >     test_a1() {
> >       ...  // the body of a test
> >     }
> > 
> >     test_a2() {
> >       ...  // the body of a test
> >     }
> > 
> >     // Note: `run` is gone.
> >   }
> > 
> >   @reflectiveTest
> >   class Impl1Test extends _ClassHierarchyTest {
> >     createClassHierarchy() => new Impl1();
> >   }
> > 
> >   @reflectiveTest
> >   class Impl2Test extends _ClassHierarchyTest {
> >     createClassHierarchy() => new Impl2();
> > 
> >     @fail // you can mark failing tests with metadata
> >     test_a1() => super.test_a1();
> >   }
> > 
> > 
> > 
> > 
> > Some known drawbacks of this package are:
> >  - as the name implies, it depends on dart:mirrors. It uses @MirrorsUsed, so
> > dart2js can still run these tests effectively (under some but not all
> > configurations).
> > 
> >  - it requires that you follow the class structure above. In this case it
was
> > pretty useful to use that structure, but not all tests need it, so if you
> adopt
> > it more widely, it might feel like boiler plate having to write tests in
this
> > style.
> > 
> > 
> > Finally - if you prefer that we don't add the dependency, let us know if the
> > pattern above would be acceptable.
> 
> I'm not a big fan of the reflective test suite and I think there are easier
ways
> to share test functionality. It seems to me that
package:test_reflective_loader
> is designed to make it easy to port JUnit tests to Dart and doesn't need to be
> used for new tests.

1. Well, I think making it easier to write unit tests is a good goal. It does
not make it bad if it happens to be JUnit3 style. And honestly many front_end
tests are less than ideal examples of easier ways to write tests.

2. package:test_reflective_loader also supports solo_test to run just one test -
very useful while debugging a problem in a single case.

3. I thought we decided that it is up to the author of a new file to define the
style of this file.

4. What you call "JUnit tests" is actually just a reasonable style for IDEs -
you see nice structure of your tests. But of course it does not matter for
people who use less sophisticated way to edit code :-P

I can of course rewrite it without reflection.
With some code generation or code duplication.
I will lose solo_test, but I guess I have to do this to make progress.

[scheglov, 2017-05-31 16:02:43]

PTAL

[ahe, 2017-05-31 16:51:40]

On 2017/05/31 15:43:06, scheglov wrote:
> If there are any comments about logic, or about TODO for getDispatchTarget()
and
> abstract classes / members, please make these comments. We can always change
> reflection style with explicit code if we decide to. But given that it is the
> end of the working day for you, and the CL has not been reviewed by anyone in
> Europe, this effectively stops my work on future ClassHierarchy tests.

Fair enough.

I'll take a look now, but honestly I think Kevin is more familiar with this than
I am.

> 4. What you call "JUnit tests" is actually just a reasonable style for IDEs -
> you see nice structure of your tests. But of course it does not matter for
> people who use less sophisticated way to edit code :-P

Good point, I hadn't considered that aspect.

[ahe, 2017-05-31 17:33:49]

lgtm

I don't think inlining JUint is an optimal solution either.

All my comments below are just ideas that you can ignore.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
File pkg/kernel/test/class_hierarchy_test.dart (right):

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:203: //  class C extends Object with
B<int> {}
Instead of all these comments, I'd take advantage of the textual syntax of
kernel nodes and assert that pretty printed text matches the expectation.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:491: // TODO(scheglov) The next two
commented statements verify the behavior
Add colon after ")".

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:495: // either documentation, or
implementation.
Since Fasta uses this method, it would be interesting to know how this affects
lookup of super calls.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:601: //   E
I'm thinking it might be a good idea to add more complex hierarchies to this
test.

[scheglov, 2017-05-31 17:53:28]

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
File pkg/kernel/test/class_hierarchy_test.dart (right):

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:203: //  class C extends Object with
B<int> {}
On 2017/05/31 17:33:49, ahe wrote:
> Instead of all these comments, I'd take advantage of the textual syntax of
> kernel nodes and assert that pretty printed text matches the expectation.

That's an interesting idea.
I will do this in the next CL.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:491: // TODO(scheglov) The next two
commented statements verify the behavior
On 2017/05/31 17:33:49, ahe wrote:
> Add colon after ")".

Done.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:495: // either documentation, or
implementation.
On 2017/05/31 17:33:49, ahe wrote:
> Since Fasta uses this method, it would be interesting to know how this affects
> lookup of super calls.

I hope that Kevin will able to shed some light.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:601: //   E
On 2017/05/31 17:33:49, ahe wrote:
> I'm thinking it might be a good idea to add more complex hierarchies to this
> test.

Sounds good to me.
Anything specific you have in mind?

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:675: static void
_runTests(_ClassHierarchyTest newInstance()) {
Given that this is considered as not an optimal solution either, I will roll it
back and leave using package:test_reflective_loader for now.

[scheglov, 2017-05-31 18:31:41]

Description was changed from

==========
Start adding unit tests for ClassHierarchy.

I need these tests in order to be sure that I understand expectations
of the ClassHierarchy interface, and to be able to test a lazy
implementation I'm going to create for incremental kernel generator.

I moved lub_test tests into this wider test suite.

Tests for forEachOverridePair() are not complete yet, just one path
is tested.

If there are any additional cases that you think should be covered,
please let me know.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Start adding unit tests for ClassHierarchy.

I need these tests in order to be sure that I understand expectations
of the ClassHierarchy interface, and to be able to test a lazy
implementation I'm going to create for incremental kernel generator.

I moved lub_test tests into this wider test suite.

Tests for forEachOverridePair() are not complete yet, just one path
is tested.

If there are any additional cases that you think should be covered,
please let me know.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-05-31 18:31:43]

Committed patchset #4 (id:60001) manually as
e94a8e136799bfdfa8c11861f07e08098680c107 (presubmit successful).

[ahe, 2017-05-31 18:33:06]

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
File pkg/kernel/test/class_hierarchy_test.dart (right):

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:601: //   E
On 2017/05/31 17:53:27, scheglov wrote:
> On 2017/05/31 17:33:49, ahe wrote:
> > I'm thinking it might be a good idea to add more complex hierarchies to this
> > test.
> 
> Sounds good to me.
> Anything specific you have in mind?

I'll discuss it with Johnni tomorrow. I can't remember the details, but I think
we've discussed it before.

https://codereview.chromium.org/2912173002/diff/40001/pkg/kernel/test/class_h...
pkg/kernel/test/class_hierarchy_test.dart:675: static void
_runTests(_ClassHierarchyTest newInstance()) {
On 2017/05/31 17:53:27, scheglov wrote:
> Given that this is considered as not an optimal solution either, I will roll
it
> back and leave using package:test_reflective_loader for now.

I agree.