-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
fix: vitest custom tester with toMatchObject #2688
fix: vitest custom tester with toMatchObject #2688
Conversation
🦋 Changeset detectedLatest commit: 9db3161 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1a623ef
to
e833052
Compare
Not sure why but this makes some tests fail, also not sure about the early escape of Equal |
2c44ed3
to
161c8cb
Compare
It really looks like the assertion in the failing test is incorrect. I'm not sure why it'd occur just now tho. |
161c8cb
to
9db3161
Compare
Me neither, but I'm not aware of any better solution. Also, I have another example that will fail even with the proposed change. it("Data.struct", () => {
const alice = Data.struct({ name: "Alice", age: 30 })
expect(alice).toMatchObject(Data.struct({ name: "Alice" }))
}) |
Is that because of the symbols? |
It's because of hash check in |
Hash checks are nullified when structural is enabled, they are all 0
…On Sat, 4 May 2024, 11:33 Milan Suk, ***@***.***> wrote:
also not sure about the early escape of Equal
Me neither, but I'm not aware of any better solution. Also, I have another
example that will fail even with the proposed change.
it("Data.struct", () => {
const alice = Data.struct({ name: "Alice", age: 30 })
expect(alice).toMatchObject(Data.struct({ name: "Alice" }))
})
Is that because of the symbols?
It's because of hash check in compareBoth.
—
Reply to this email directly, view it on GitHub
<#2688 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFZAKCQVLJNVM44DLMBUFUTZASTP3AVCNFSM6AAAAABHEOINSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUGA4TMMZQGE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
I debugged that locally, the circuit shorts in here. Even if the hash check wasn't there it would return false on comparison inside of the equal of the |
Currently, the usage of the custom tester breaks
expect(...).toMatchObject(...)
. Failing example:The reason is that
toMatchObject
in vitest is implemented as an equality check withsubsetEquality
as a last custom tester. Our custom tester currently doesn't have any skip logic to allow continuation of the testers pipeline it's part of. Another problematic thing is that thetoMatchObject
is not commutative, and it uses equality which is commutative. Therefore in equal impl of Option, Either, ... the matching breaks when the arguments are passed toEqual.equals
in an opposite order.