Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated List equality produces false negative #19

Closed
dustin-graham opened this issue May 27, 2019 · 12 comments
Closed

updated List equality produces false negative #19

dustin-graham opened this issue May 27, 2019 · 12 comments
Assignees
Labels
bug
Projects

Comments

@dustin-graham
Copy link
Contributor

@dustin-graham dustin-graham commented May 27, 2019

Describe the bug
The new list equality check introduces false negatives in my tests when comparing identical lists with different runtime types. In consequence of other business logic in my code, I end up with a GrowableList in one side of the equality and a CopyOnWriteList on the other. Contents are identical but the runtime check fails.

To Reproduce
In my test I generate dummy data with List.generate(). This produces a GrowableList. However in my code I am using BuildList which has a toList() method that returns a CopyOnWriteList. The equality check worked as expected with older code.

Expected behavior
Deep equals check should find that the list contents are identical

Screenshots
Screen Shot 2019-05-27 at 7 46 53 AM

Version
Dart VM version: 2.3.0 (Fri May 3 10:32:31 2019 +0200) on "macos_x64"

Additional context
I'm able to work around this in my tests by wrapping my original dummy data in a BuiltList and using the toList() to compare. But this is a bit more work than I would expect and might be confusing to future maintainers. Thanks!

@felangel felangel self-assigned this May 27, 2019
@felangel felangel added the bug label May 27, 2019
@felangel felangel added this to To do in equatable May 27, 2019
@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented May 27, 2019

Hi @dustin-graham 👋
Thanks for opening an issue!

I'll try to address the issue in the next few days. Alternatively, if you want/have the time you are more than welcome to open a PR 👍

@dustin-graham

This comment has been minimized.

Copy link
Contributor Author

@dustin-graham dustin-graham commented May 28, 2019

@felangel thanks for the reply. I'd be happy to contribute. I'd like to be sure I understand the motivations for this change so that I don't cause someone else an unexpected change. Could we safely just remove the runtimeType check?

@jorgecoca

This comment has been minimized.

Copy link

@jorgecoca jorgecoca commented May 28, 2019

I think the library is working as expected. It is our requirement that, in order to assert that two objects are equal, their runtimes must be equal as well.
Otherwise, we might end up in situations where Person(name: 'Jorge') and Duck(name: 'Jorge') might be the same object.
If runtime is not necessary for your equality requirements, you can always implement manually == and hashCode to have more control over the conditions.

@dustin-graham

This comment has been minimized.

Copy link
Contributor Author

@dustin-graham dustin-graham commented May 28, 2019

@jorgecoca I agree for the most part. However, in this spot, we're comparing two lists as given by the function signature. This runtime type check is asserting that the two lists are of the same type, not whether their contents are equal.

if (unit1?.runtimeType != unit2?.runtimeType) return false;

These two lists have identical contents. Prior to the latest release these two lists were considered equal and that seemed to me to be expected behavior. With the latest release these two lists with identical contents are no longer equal because the list wrapper is of a different concrete type, even though they are both Lists.

It would be good here to be able to check if the generic type argument of the two lists are the same, but that's not what this line does.

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented May 29, 2019

@dustin-graham I agree with @jorgecoca. I would actually consider the fact that this worked in the previous release a bug. If the runtimeTypes are different then I don't think we can consider the two objects equal because this would introduce many other bugs like @jorgecoca mentioned earlier.

@felangel felangel moved this from To do to In progress in equatable May 29, 2019
@dustin-graham

This comment has been minimized.

Copy link
Contributor Author

@dustin-graham dustin-graham commented May 29, 2019

@felangel @jorgecoca I put together a small sample to illustrate this issue

https://github.com/dustin-graham/dart_equality_sample

I'll inline the relevant code here:

main(List<String> arguments) {
  final growableList = List.generate(1, (_) {
    return Student((b) {
      b.name = "Student 1";
      b.email = "1@student.com";
    });
  });
  final builtStudentList = BuiltList<Student>(growableList).toList();

  final listEquality = ListEquality<Student>();
  final collectionEquality =
      listEquality.equals(growableList, builtStudentList);
  print("collection lib equality: $collectionEquality"); // prints true

  // pass in the lists as if given via a props list
  final equatableEquals =
      EquatableUtils.equals([growableList], [builtStudentList]);
  print("equatable lib equality: $equatableEquals"); // prints false

  print("built list is List: ${builtStudentList.toList() is List<Student>}");
  print("regular list is List: ${growableList is List<Student>}");
  var runtimeType = builtStudentList.runtimeType;
  print("built list runtime type: ${runtimeType}");
  var runtimeType2 = growableList.runtimeType;
  print("generated list runtime type: ${runtimeType2}");
}

I made a PR that fixes the issue and keeps all the runtime type checking.
#20

The line that checks if the units are collections is the correct special-case runtime type check for collection types. The issue is that it wasn't being reached because of the general purpose runtimeType check right before it. The sample code demonstrates that two lists of different runtime types are considered equal in the Collection package, but not even compared in Equatable. This PR simply puts the collection runtime type check first and the general purpose runtime type check second. I believe this is the correct order. Thanks!

@algodave

This comment has been minimized.

Copy link

@algodave algodave commented May 30, 2019

@felangel I think the changes added between 0.2.4 and 0.2.5 are relevant, I think they deserve crafting the 0.3.0 (btw: Releases here on GitHub are stuck on 0.2.3)

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented May 30, 2019

@algodave what do you mean they deserve crafting 0.3.0? We're following semantic versioning and this was a bug fix in my opinion without a breaking API change. Can you please elaborate?

Also, thanks for pointing out the lack of 0.2.4 and 0.2.5 releases! I have updated the releases now 👍

@algodave

This comment has been minimized.

Copy link

@algodave algodave commented May 30, 2019

@felangel Deciding whether 2 lists having the same elements but a different runtimeType should be equal or not could be also considered a design decision rather than a bug 🙂 - up to you of course!

In my case this change was "breaking" because all my Bloc tests are failing with 0.2.5. They use the following pattern:

test('My Bloc event test', () {
  // mock stuff etc.
  final expectedStates = [MyState(), MyOtherState()];
  expectLater(
    blocBeingTested.state, 
    emitsInOrder(expectedStates),
  );
  blocBeingTested.dispatch(MyEvent());
});

and the error I get is that the expected list of states is not equal to the one emitted by the bloc being tested.

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented May 31, 2019

@algodave I don't believe this change should have broken your tests. Are you able to share a sample that worked with equatable v0.2.4 and breaks with v0.2.5? All of our tests are structured the same way and the bloc library's tests are also following the same pattern.

@algodave

This comment has been minimized.

Copy link

@algodave algodave commented May 31, 2019

@felangel Thank you for answering, it helped me to look better at my tests, and the problem was there!

Some of my states hold a List<MyModel> prop; in tests, I defined MyModelMock extends Mock implements MyModel using mockito.
Lists in expected states were instantiated as List<MyModelMock> which caused the equality check fail. I changed them to List<MyModel> and everything works fine.

So my app is now able to run v0.2.5 joyfully 🙂

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented May 31, 2019

This is addressed in #20 and included in equatable v0.2.6. Thanks @dustin-graham!

@felangel felangel closed this May 31, 2019
@felangel felangel moved this from In progress to Done in equatable May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
equatable
  
Done
4 participants
You can’t perform that action at this time.