DRAFT: chore: vitest iterable equality temporary workaround

[sukovanej]

Temporary fix for vitest-dev/vitest#5620 as right now the tests comparing objects with effect prototype will always succeed. Can be removed once fixed in the vitest.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d47e9de

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[mikearnaldi]

wondering if we should ship it in @effect/vitest too

[mikearnaldi]

Seems to break valid tests that used to pass

[sukovanej]

wondering if we should ship it in @effect/vitest too

I was thinking the same 👍

Seems to break valid tests that used to pass

So the problem is that e.g. HashMap is an iterator and with the proposed change the equality returns false because of properties like _edit. I was thinking about using the effect/Equal equality instead but that fails for whatever non-trivial situation like Option<Array<...>>. Right now, I'm actually thinking about introducing handling of concrete data structures (Option / Either) into the custom tester. WDYT?

[mikearnaldi]

wondering if we should ship it in @effect/vitest too

I was thinking the same 👍

Seems to break valid tests that used to pass

So the problem is that e.g. HashMap is an iterator and with the proposed change the equality returns false because of properties like _edit. I was thinking about using the effect/Equal equality instead but that fails for whatever non-trivial situation like Option<Array<...>>. Right now, I'm actually thinking about introducing handling of concrete data structures (Option / Either) into the custom tester. WDYT?

That would be one option, but why does it fail? are those property different or is it due to things like different instances of functions?

[mikearnaldi]

Handling concrete structures in the matcher would be perfect, we can support the full set of types in the library and it would become way easier to compare them. Wondering if those should be tight to vitest though, probably jest and other frameworks have the same issue

[sukovanej]

wondering if we should ship it in @effect/vitest too

I was thinking the same 👍

Seems to break valid tests that used to pass

So the problem is that e.g. HashMap is an iterator and with the proposed change the equality returns false because of properties like _edit. I was thinking about using the effect/Equal equality instead but that fails for whatever non-trivial situation like Option<Array<...>>. Right now, I'm actually thinking about introducing handling of concrete data structures (Option / Either) into the custom tester. WDYT?

That would be one option, but why does it fail? are those property different or is it due to things like different instances of functions?

Yes, they are different because the check compares an instance produced by some flow doing modifications (results in none-zero _edit) with an instance constructed using make / fromIterable / ... (_edit = 0). So in case of HashMap, HashSets and probably some more data structures there's nothing else to do than to bypass the "object equivalence" because the useful equivalence shouldn't care about these internal values.

import { HashMap } from 'effect';

const hm1 = HashMap.fromIterable([[1, 1]]);
const hm2 = HashMap.empty<number, number>().pipe(HashMap.set(1, 1));

console.log(hm1._edit, hm2._edit); // 0 1

[sukovanej]

I pushed a draft of a potential solution based on equivalences. I wanted to reuse some kind of equality that's already present in the codebase and I need a way for the vitest's equality fn to take control in certain places. So it felt like a natural solution.

The final outcome is a new function in the @effect/vitest that extends the custom testers which can be used in the user-land to extend the ability of vitest matchers to compare effect's data structures.

Please let me know whether you think this is the way to go.

[mikearnaldi]

Why not equals? Equivalence is usually explicit between known types

[sukovanej]

Because equals says two things are equal only if they are strictly equal or when they implement the Equal which says they're equal. E.g. Equal.equals(Option.some([]), Option.some([])) is false but for the purpose of testing we definitely want to say they are equal. And that's why equivalence, because it allows to give control to another equivalence, and conceptually passing in the vitest's equality is like using Equivalence.strict() but it's more useful because it can actually compare by value.

[sukovanej]

Also, the idea behind the makeCustomTesters declaration of how to check and compare types from effect using Equivalence is that we can potentially reuse it for different testing libs by passing their specific equality.

[sukovanej]

I wonder whether it makes sense to deal with a non-strict equality for keys in hashmaps. They are implemented using a hash function which generates a random number for objects so it is non-sensical to use anything other than strictly comparable types as keys (or values that equal by reference). If I wanted to provide the equivalence implementation with an expectation the key can be of any type and the equivalence would be used instead of a strict equal, it wouldn't be possible to have ~O(n) implementation. Also Equal.equals would behave fundamentally differently from the comparison in tests.

[mikearnaldi]

HashMap keys are not using strict equality they are using Equals, random hash is for mutable objects, by default objects are mutable but they can implement the Equal trait and customize equality.

Imho the only non weird default is to use equal, though I see the deep compare expectation

[sukovanej]

Hey, I was playing around with the idea of fallbacking to Equal.equals in the custom tester but with it we'd need to handle specifically everything implementing the Equal because equality checks for all the usual types like List, Chunk, Hash* would be failing in tests on comparison of types not implementing the Equal trait. And I don't think we'll ever live in a universe where we could practically expect everything implements the Equal trait so this is a must. This led me to quite a rabbit hole and I even found a bug in the Cause equals impl (e.g. Cause.die(1) and Cause.fail(1) are considered equal because the evaluateCause doesn't carry the _tag in the output) so I decided to provide a minimal implementation handling (hopefully) the most used types in the user-land test suites (Option, Either and Exit) in #2646 which should be a good enough solution to vitest-dev/vitest#5620 for now (and doesn't involve changes in the effect package). Then we can iterate on top of it.

[mikearnaldi]

Addressing the bug in Cause equality: #2653

[mikearnaldi]

Closing as #2653 seems to work fine and doesn't need specificity for data types