-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: allow extending toEqual (fix #2875) #4880
feat: allow extending toEqual (fix #2875) #4880
Conversation
✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.
|
a69fae9
to
40fce2e
Compare
Is this a duplicate of #4586? |
Seems, yes. I have not noticed it, as #4586 is not attached to #2875 . I can see some differences between jest-asymmetric-matchers.ts and jest-asymmetric-matchers.ts files and not only. |
40fce2e
to
ec0b4a9
Compare
@sheremet-va As #4586 is closed, maybe you can review this one ? |
ec0b4a9
to
a0f502e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
a0f502e
to
f423825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice! I didn't look into the implementation, but a few wordings in documentation felt off, so I made some suggestions there.
4e97ba4
to
2bd4de2
Compare
338521c
to
422d479
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jest's spy related matchers (e.g. toHaveBeenCalledWith
, toHaveReturnedWith
) seem to also use custom testers for argument/return values comparison.
https://github.com/jestjs/jest/blob/2178fa2183cb7cb2ac3e262388499bafd032ef03/packages/expect/src/spyMatchers.ts#L64-L65
https://github.com/jestjs/jest/pull/13654/files#diff-9ce99fa9b58d394529495c6d13e759f0a3a6e892420327289ab50aa33f18a4a1
Probably you missed updating these parts of jestEquals
?
vitest/packages/expect/src/jest-expect.ts
Lines 482 to 485 in 9ec3f74
def(['toHaveBeenCalledWith', 'toBeCalledWith'], function (...args) { | |
const spy = getSpy(this) | |
const spyName = spy.getMockName() | |
const pass = spy.mock.calls.some(callArg => jestEquals(callArg, args, [iterableEquality])) |
422d479
to
c69fb7b
Compare
Yes, thanks for noticing it, already updated. |
test/core/test/expect.test.ts
Outdated
const arePersonsEqual = (a: unknown, b: unknown, customTesters: Array<Tester>) => { | ||
const isAPerson = a instanceof Person | ||
const isBPerson = b instanceof Person | ||
|
||
if (isAPerson && isBPerson) | ||
return a.name === b.name && equals(a.address, b.address, customTesters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good!
Just this one, I think using this.equals
reflects API better than using equals
export. Maybe you should rewrite it to function arePersonsEqual(this: TesterContext, a: unknown, ...) { ... }
?
Description
This PR solves issue #2875 .
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.