Issue with viewmodel comparison

[guy-plentific]

We have noticed issues when comparing viewmodels for equality and it is affecting testing. The below code demonstrates the outcome of comparisons:

import 'package:async_redux/async_redux.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter_test/flutter_test.dart';

class TestVm extends Vm {
  TestVm({
    required this.isLoading,
    required this.numbers,
  }) : super(equals: [
          isLoading,
          numbers,
        ]);

  final bool isLoading;
  final List<int> numbers;
}

void main() {
  test('xyz', () {
    final vmA = TestVm(isLoading: true, numbers: [1, 2, 3]);
    final vmB = TestVm(isLoading: true, numbers: [1, 2, 3]);

    expect([1, 2, 3] == [1, 2, 3], true); // test 1 - fails
    expect(listEquals([1, 2, 3], [1, 2, 3]), true); // test 2 - passes
    expect(vmA, vmB); // test 3 - fails
  });
}

We believe the reason for this failure lies in the equality checking in the Vm class. The below code is from view_model.dart (async_redux 15.0.0)

bool _listEquals<T>(List<T>? list1, List<T>? list2) {
    if (list1 == null) return list2 == null;
    if (list2 == null || list1.length != list2.length) return false;
    if (identical(list1, list2)) return true;
    for (int index = 0; index < list1.length; index++) {
      var item1 = list1[index];
      var item2 = list2[index];

      if ((item1 is VmEquals<T>) &&
          (item2 is VmEquals<T>) //
          &&
          !item1.vmEquals(item2)) return false;

      if (item1 != item2) return false;
    }
    return true;
  }

In the line if (item1 != item2) return false; we are returning false here if the items are lists. This is because we are checking list equality like we did in test 1, rather than using the listEquals of test 2. A potential fix may be to check if the items are lists (or iterables) and if so use the listEquals method instead.

What are your thoughts?

[marcglasberg]

When you compare 2 Iterables with Dart tests, it actually compares the content of the Iterable. That's why you get:

expect([1, 2, 3] == [1, 2, 3], true); // Fails
expect([1, 2, 3], [1, 2, 3], true); // Passes

I really hate this behavior. If 2 list instances with the same parameters are different, then they are different, and expect([1, 2, 3], [1, 2, 3], true) should not pass. Instead, it should have some matcher to compare the contents, for example: expect([1, 2, 3], sameContentsAs([1, 2, 3]), true).

You are basically saying AsyncRedux should have the same weird behavior as expect in tests. This is bad for a few reasons:

• If the lists contain 100,000 items, comparing both may take a long time, and maybe you don't want that to happen. When a list doesn't change, it will have the same instance, and when you have a new list it's probably because the list content changed. Remember state must be immutable. So you can't really change the list, you have to create a new one. If your list instance changed, it's most likely because it's contents are different, otherwise you could have kept the same instance. So, using listEquals is useless most of the time, and time consuming every time.

• Suppose you have to classes that implement Iterable, but they have other parameters apart from the Iterable part. For example:

class Course implements Iterable<Course>{
   final name;
   final List<String> students;
}

This is what Dart tests do, and it sucks:

expect(
   Course("English", ["John", "Sarah"]), 
   Course("Math", ["John", "Sarah"]), 
false); // Fails!!!

It ignores the name, since it doesn't use the operator ==. I don't want to replicate this terrible behavior in Vm comparisons.

---

There are a few solutions for you:

1) You can use the same List instance in the tests:

void main() {
  test('xyz', () {
    final list = [1, 2, 3];
    final vmA = TestVm(isLoading: true, numbers: list);
    final vmB = TestVm(isLoading: true, numbers: list);

    expect(vmA, vmB); // Passes
  });
}

2) You can implement your own equals for each Vm class, instead of using the equals parameter:

class Vm {
bool operator == () => ... listEquals(this, other) ...

3) You can extend the base Vm class and create your own base class that does what you want:

class MyVm extends Vm {
bool operator == () =>  // Compare all Iterables with ListEquals.

// Then use it:
class TestVm extends MyVm {
  TestVm({
    required this.isLoading,
    required this.numbers,
  }) : super(equals: [
          isLoading,
          numbers,
        ]);

And now, the recommended way:

4) Don't use Lists, which are mutable. Instead, use a proper immutable list, the IList class from package https://pub.dev/packages/fast_immutable_collections

The default for IList is to compare the contents. It will be much faster then Lists, and it will be much easier to use. This will also help you reduce bugs, since you can't modify an IList by mistake as you can with a List.

You also have ISet, IMap and other useful classes like IMapOfSets and a bunch of extensions.

[guy-plentific]

Many thanks for your detailed response, this is very helpful!