Start writing unit tests

[gnprice]

When working on #13 I realized that storing data means we're going to start definitely wanting tests.

So here's some first steps in having them, which provides the common infrastructure that tests for #13 will also want.

[gnprice]

I've just pushed an additional pair of commits that switches from the old expect-based API to the Dart team's spiffy new more-typed API. Definitely seems much nicer!

[chrisbobbe]

Ah, cool about switching to the new checks package! I'd written some comments before those commits came in, so please disregard the ones that don't apply anymore.

[chrisbobbe]

I think this might give the same result?

    if (!equals(nodes).matches(item.nodes, matchState)) {
      return false;
    }

Here's equals; see the third paragraph:

https://pub.dev/documentation/matcher/latest/matcher/equals.html

/// Returns a matcher that matches if the value is structurally equal to
/// [expected].
///
/// If [expected] is a [Matcher], then it matches using that. Otherwise it tests
/// for equality using `==` on the expected value.
///
/// For [Iterable]s and [Map]s, this will recursively match the elements. To
/// handle cyclic structures a recursion depth [limit] can be provided. The
/// default limit is 100. [Set]s will be compared order-independently.

[gnprice]

/// If [expected] is a [Matcher], then it matches using that. Otherwise it tests
/// for equality using `==` on the expected value.

Yeah — I was fooled by the name and summary, which seemed awfully clear that it meant equal and not recursively applying general matchers.

It turns out the checks package has a similar story, but perhaps there's an opportunity to improve it there before it's stable.

[chrisbobbe]

The Flutter style guide has a note about choosing between toString and Diagnosticable: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#override-tostring

Would Diagnosticable be useful for some of the classes, do you think?

[gnprice]

Likely. I haven't yet read up on Diagnosticable. Should probably have a TODO comment for it.

[chrisbobbe]

LineBreakNode is unlikely to be subclassed, I think, but isn't that a factor in choosing this implementation with is over one that equality-checks the runtimeTypes? The operator == example in the Flutter project's style guide compares runtimeTypes:

@override
bool operator ==(Object other) {
  if (other.runtimeType != runtimeType) {
    return false;
  }
  // […]
}

If we have a class Animal that overrides ==, and its subclasses Dog and Cat that don't, then someDog == someCat will give

• true if Animal's == override is other is Animal
• false if Animal's == override is other.runtimeType == runtimeType

(False is probably the right answer.)

(code that you can paste into dartpad.dev)

class Animal {
  @override
  bool operator ==(Object other) {
    return other is Animal;
  }
  
  @override
  int get hashCode => 'Animal'.hashCode;
}

class Dog extends Animal {}
class Cat extends Animal {}

Dog someDog = Dog();
Cat someCat = Cat();

main() {
  print('someDog == someCat: ${someDog == someCat}');
}

[chrisbobbe]

I'm wondering if it's worth changing the implementation or commenting about the expectation that it won't be subclassed. Am I being too paranoid? 😅

[chrisbobbe]

Oh, and:

LineBreakNode is unlikely to be subclassed, I think

Is this right? If so, can I make the same judgment of the other non-abstract FooNode classes in the file (so e.g. LineBreakInlineNode but not EmojiNode)?

[gnprice]

Yeah, none of these classes should ever have subclasses beyond the ones defined in this file. (So in Dart 3, we'll declare them as sealed: https://github.com/dart-lang/language/blob/a374667bcc4095edd22e2e6d454cbe9d9f9d85fa/accepted/future-releases/sealed-types/feature-specification.md .) For most of them, including LineBreakNode, that means they'll have no subclasses at all.

[chrisbobbe]

(Same comment about the is operator.)

[chrisbobbe]

Possibly this could be made more compact by letting the caller choose between a matcher and a literal value (with the equals matcher implied), so you could pass wasImplicit: false instead of wasImplicit: equals(false). Then internally ParagraphNodeMatcher could use wrapMatcher.

[chrisbobbe]

I see your inline responses to my review; please merge at will (perhaps after adding a TODO for Diagnosticable as you mentioned in #17 (comment)) 🙂