Port node package tests to quicktest

[masih]

Port the tests in node package to quicktest; use:

• qt.Assert for wish.Require
• qt.Check for wish.Wish
• qt.IsTrue for ShouldEqualovertrue`
• qt.IsFalse for ShouldEqual over false
• qt.IsNil for ShouldEqual over nil

Port closeEnough Wish checker to its equivalent in quicktest.

Relates to:

• wishlist: port all tests to quicktest #219

[mvdan]

Excellent work!

By the way, you might want to encode those "shorter qt expression" nits as sed lines, so you can automatically apply those cleanups throughout your PRs.

[mvdan]

clever :) I like how this turned out, even if implementing a custom checker with quicktest is slightly more verbose.

you could almost use qt.CmpEquals with https://pkg.go.dev/github.com/google/go-cmp/cmp#Comparer here, except that Comparer expects func(T, T) bool, so it only kicks in when the two types being compared are exactly equal. The logic here is different, as it is more flexible.

[mvdan]

mental TODO: in the future, we might be able to expose datamodel.DeepEqual as a go-cmp Comparer. Definitely out of scope for this refactor, though.

[warpfork]

One could also use the printer package and then do string diff on the results.

That would simultaneously be very complete, and produce very readable debugable output, and also happily ignore the actual node implementation strategy (which is usually what we want).

[masih]

quicktest comes with CmpEquals which looks pretty flexible; I wonder if we could use that here?

I'll add a TODO in code for this so that we don't forget :)

[mvdan]

That is what I was alluding to. But you need a custom comparer, because by default, CmpEquals will just loop infinitely. This is because bindnode ipld.Node implementations contain schema types, which have cyclic pointers.

[mvdan]

I originally arrived there via the flat datamodel.DeepEqual, which works, but also doesn't give good errors. I think either a custom go-cmp comparer, or comparing encoded versions like "printer" or "dagjson" like Eric mentions, would work better.

[masih]

I attempted to write a checker that works with printer here but looks like there are TODOs in the printer that tests hit. Going to use datamodel.DeepEqual going forward.

Used dagjson to compare equality; checker implemented here

[mvdan]

BTW, not sure if you're aware, but qt.Check does a t.Error, whereas qt.Assert does a t.Fatal. I think in general we want the latter. For example:

foo, err := bar()
qt.Check(t, err, qt.IsNil) // assume this errors because err != nil

qt.Check(t, foo.Baz, qt.Equals, "xyz") // panic! foo is nil

If you use qt.Assert on both lines, then we stop when err != nil, and we don't panic.

qt.Check is for the few times where we do want to continue running a test even when it errors once. That's commonly used e.g. when you check multiple properties in a row, so you want to report all the errors, and not just the first one.

[masih]

Thank you for pointing that out @mvdan; I interpreted tests as wish.Require should do t.fatal and wish.Wish should do t.Error then used the equivalent qt function to keep the behaviour consistent with what's there just now.

Do you think the behaviour of tests should change?

[mvdan]

If you did that intentionally, then SGTM. Keeping existing behavior is better for the sake of a smooth refactor. I was bringing it up in case it flew under your radar.

[masih]

Thank you; I intentionally wanted to keep changes as net-zero as possible

[warpfork]

(Wow, we have a lot of tests!)

Nice. It'll be great to have one fewer testing system in the dependency tree by the end of this (and the similar PRs).