Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21155003:
Improve test coverage for reflection on mixins. (Closed)
|
Created:
7 years, 4 months ago by Michael Lippautz (Google) Modified:
7 years, 4 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionImprove test coverage for reflection on mixins.
BUG=
R=ahe@google.com
Committed: https://code.google.com/p/dart/source/detail?r=25834
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #Messages
Total messages: 12 (0 generated)
Improve test coverage for walking mixin inheritance chains. Related to issue 9434 (ClassMirror.supperclass getter in VM).
https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... File tests/lib/mirrors/mixin_members_test.dart (right): https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... tests/lib/mirrors/mixin_members_test.dart:12: iterablesEqual(Iterable a, Iterable b) => setsEqual(a.toSet(), b.toSet()); This is a misleading name. I expect iterables to be equal iff they stream the same elements in the same order. Perhaps bool haveSameElements(Iterable a, Iterable b) { return a.every( (e) => b.contains(e) ) && b.every( (e) => a.contains(e) ); } ? https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... tests/lib/mirrors/mixin_members_test.dart:36: Expect.equals(true, cm.members.length == 0); Expect.equals(0, cm.members.length) https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... tests/lib/mirrors/mixin_members_test.dart:37: Expect.equals(true, iterablesEqual( Expect.isTrue(...)
https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... File tests/lib/mirrors/mixin_members_test.dart (right): https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... tests/lib/mirrors/mixin_members_test.dart:12: iterablesEqual(Iterable a, Iterable b) => setsEqual(a.toSet(), b.toSet()); On 2013/07/30 19:10:50, Ryan Macnak wrote: > This is a misleading name. I expect iterables to be equal iff they stream the > same elements in the same order. Perhaps > > bool haveSameElements(Iterable a, Iterable b) { > return a.every( (e) => b.contains(e) ) && > b.every( (e) => a.contains(e) ); > } > > ? Done. https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... tests/lib/mirrors/mixin_members_test.dart:36: Expect.equals(true, cm.members.length == 0); On 2013/07/30 19:10:50, Ryan Macnak wrote: > Expect.equals(0, cm.members.length) Done. https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... tests/lib/mirrors/mixin_members_test.dart:37: Expect.equals(true, iterablesEqual( On 2013/07/30 19:10:50, Ryan Macnak wrote: > Expect.isTrue(...) Done.
On 2013/07/30 19:45:50, Michael Lippautz wrote: > https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... > File tests/lib/mirrors/mixin_members_test.dart (right): > > https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... > tests/lib/mirrors/mixin_members_test.dart:12: iterablesEqual(Iterable a, > Iterable b) => setsEqual(a.toSet(), b.toSet()); > On 2013/07/30 19:10:50, Ryan Macnak wrote: > > This is a misleading name. I expect iterables to be equal iff they stream the > > same elements in the same order. Perhaps > > > > bool haveSameElements(Iterable a, Iterable b) { > > return a.every( (e) => b.contains(e) ) && > > b.every( (e) => a.contains(e) ); > > } > > > > ? > > Done. > > https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... > tests/lib/mirrors/mixin_members_test.dart:36: Expect.equals(true, > cm.members.length == 0); > On 2013/07/30 19:10:50, Ryan Macnak wrote: > > Expect.equals(0, cm.members.length) > > Done. > > https://chromiumcodereview.appspot.com/21155003/diff/1/tests/lib/mirrors/mixi... > tests/lib/mirrors/mixin_members_test.dart:37: Expect.equals(true, > iterablesEqual( > On 2013/07/30 19:10:50, Ryan Macnak wrote: > > Expect.isTrue(...) > > Done. Declare some interfaces on the mixin classes and also check that the mixin applications have the proper entries in their superinterface lists.
Added checks for the superinterfaces. (The current implementation (only) provides access to direct superinterfaces.)
LGTM with a few nits. https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... File tests/lib/mirrors/mixin_members_test.dart (right): https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:9: bool haveSameElements(Iterable a, Iterable b) { How about using Expect.listEquals(a.toList(), b.toList()); https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:37: Classmirror S_M1_M2 = cm.superclass; Please use camelCase for local variables. https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:39: ClassMirror S = S_M1.superclass; ... which avoids hiding types :-) https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:52: S.superinterfaces.map((e) => e.simpleName), [const Symbol("Fooer")])); Could you also test that reflectClass(S) == S
https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... File tests/lib/mirrors/mixin_members_test.dart (right): https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:9: bool haveSameElements(Iterable a, Iterable b) { On 2013/08/06 14:35:36, ahe wrote: > How about using Expect.listEquals(a.toList(), b.toList()); We aren't testing they have the same order.
https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... File tests/lib/mirrors/mixin_members_test.dart (right): https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:9: bool haveSameElements(Iterable a, Iterable b) { On 2013/08/06 16:33:39, Ryan Macnak wrote: > On 2013/08/06 14:35:36, ahe wrote: > > How about using Expect.listEquals(a.toList(), b.toList()); > > We aren't testing they have the same order. Then Expect.setEquals :-)
On 2013/08/06 16:42:04, ahe wrote: > https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... > File tests/lib/mirrors/mixin_members_test.dart (right): > > https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... > tests/lib/mirrors/mixin_members_test.dart:9: bool haveSameElements(Iterable a, > Iterable b) { > On 2013/08/06 16:33:39, Ryan Macnak wrote: > > On 2013/08/06 14:35:36, ahe wrote: > > > How about using Expect.listEquals(a.toList(), b.toList()); > > > > We aren't testing they have the same order. > > Then Expect.setEquals :-) Hm, nice that there is such a thing. But it seems weird for Expect to have so many features and yet there is a separate unittest package.
On 2013/08/06 17:14:23, Ryan Macnak wrote: > Hm, nice that there is such a thing. But it seems weird for Expect to have so > many features and yet there is a separate unittest package. Yes. I wish things were different.
Thanks for the comments! I also didn't know that Expect already got this many features. https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... File tests/lib/mirrors/mixin_members_test.dart (right): https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:9: bool haveSameElements(Iterable a, Iterable b) { On 2013/08/06 16:42:04, ahe wrote: > On 2013/08/06 16:33:39, Ryan Macnak wrote: > > On 2013/08/06 14:35:36, ahe wrote: > > > How about using Expect.listEquals(a.toList(), b.toList()); > > > > We aren't testing they have the same order. > > Then Expect.setEquals :-) Done. https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:37: Classmirror S_M1_M2 = cm.superclass; On 2013/08/06 14:35:36, ahe wrote: > Please use camelCase for local variables. Done. https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:39: ClassMirror S = S_M1.superclass; On 2013/08/06 14:35:36, ahe wrote: > ... which avoids hiding types :-) Done. https://codereview.chromium.org/21155003/diff/10001/tests/lib/mirrors/mixin_m... tests/lib/mirrors/mixin_members_test.dart:52: S.superinterfaces.map((e) => e.simpleName), [const Symbol("Fooer")])); On 2013/08/06 14:35:36, ahe wrote: > Could you also test that reflectClass(S) == S Done.
Message was sent while issue was closed.
Committed patchset #4 manually as r25834 (presubmit successful). |
