-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(expect): Fix to properly compare ObjectContaining #5862
Conversation
Not sure if you've seen this, but did you have this in mind for this PR? vitest/packages/utils/src/error.ts Line 111 in 7cbd943
|
@sheremet-va |
@sheremet-va If you wish to temporarily apply this PR, it would be good to merge it first and then recreate the issue to discuss the direction of moving the logic into replaceAsymmetricMatcher. Additionally, I understand that applying the logic within replaceAsymmetricMatcher affects all expect methods. I would like to confirm if this is an approach to minimize external dependencies (jest). |
4f7c285
to
c4a531b
Compare
648d24c
to
2a09861
Compare
I would greatly appreciate any feedback and guidance you might have on the current changes. Additionally, if you feel that these changes do not align with your vision or the work cycle is too lengthy, please feel free to close this PR. :) ContextI have considered the feedback you provided previously. My understanding is that you aim to minimize external dependencies (such as jest) and internalize the technology. You also appear to be interested in extending the application to all asymmetricMatchers, not just ObjectContaining. However, I also believe it is important that this PR does not introduce too substantial changes. Therefore, the following modifications have been applied: 1. Change of Abstraction Location
2. Adding 'err' Parameter to replaceAsymmetricMatcherThis additional parameter is intended only for changes in ObjectContaining among asymmetricMatchers. The variable is used as follows: // utils/error.ts
function isObjectContaining(err: any): boolean {
return err
&& typeof err.expected === 'object'
&& typeof err.expected.asymmetricMatch === 'function'
&& (err.expected.toString() === 'ObjectContaining' || err.expected.toString() === 'ObjectNotContaining')
} export function replaceAsymmetricMatcher(
actual: any,
expected: any,
actualReplaced = new WeakSet(),
expectedReplaced = new WeakSet(),
err?: any,
) {
// ...codes
if (isObjectContaining(err)) {
getOwnProperties(actual). forEach((key) => {
if (!Object.prototype.hasOwnProperty.call(expected.sample, key)) {
expected.sample[key] = actual[key]
}
})
}
3. Additional QuestionsI would like to ask if it is acceptable to use an object as a function parameter in the manner below, to reduce human error and eliminate code like 'actualReplaced: undefined'. // example
const { replacedActual, replacedExpected } = replaceAsymmetricMatcher({
actual: clonedActual,
expected: clonedExpected,
err
}) |
What I was asking is if you think the code in |
@sheremet-va Origin
"expected": "ObjectContaining {
"k": "v",
"k3": "v3",
}",
"message": "expected { k: 'v', k2: 'v2' } to deeply equal ObjectContaining {"k": "v", "k3": "v3"}",
}
`; Changed "expected": "ObjectContaining {
"k": "v",
"k2": "v2", # ⛳️ added
"k3": "v3",
}",
"message": "expected { k: 'v', k2: 'v2' } to deeply equal ObjectContaining{…}", # ⛳️ changed The current change remove this side effects. Furthermore, using the flag (err.expected.toString()) appropriately seems to allow scalable responses to the same issues with ArrayContaining, and it would be advisable to separate this logic into a separate function. expect([1, 2, 3]).toEqual(expect.arrayContaining([2, 4])) AssertionError: expected [ 1, 2, 3 ] to deeply equal ArrayContaining [2, 4]
- ArrayContaining [
+ Array [
+ 1, // ⛳️ should be changed
2,
- 4,
+ 3,
] Remaining Issues with the Current CodeThe method of adding non-comparison values to expected is not being handled recursively. The final comparison in the diff function is done using Object.is, which still poses problems for depth 2 reference data (Objects, WeakMaps, Arrays, etc.). There are five possible approaches:
I would follow any direction you propose. I do not wish to harm this excellent open source project with my imperfect contributions. thx for your time. :) |
…-object-containing
11062a1
to
a25eccd
Compare
@sheremet-va |
a25eccd
to
3eed75a
Compare
Looks like tests are failing |
…v/vitest into fix-object-containing
…-object-containing
packages/utils/src/diff/index.ts
Outdated
@@ -198,6 +211,21 @@ function getObjectsDifference( | |||
if (aCompare === bCompare) { | |||
return getCommonMessage(NO_DIFF_MESSAGE, options) | |||
} | |||
else if (options?.expected) { | |||
const clonedA = deepClone(a, { forceWritable: true }) | |||
const clonedB = deepClone(b, { forceWritable: true }) |
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.
What are the performance implications of this? It's the second time we deeply cloned the object, and in replaceDiffData
we only iterate the first level
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.
This was intended to reduce unexpected side effects, but since those values are not included in the return value, it would be better to remove them as you suggested.
Description
Fixes #5714
This PR ensures the ObjectContaining matcher compares objects properly. It fixes an issue where extra properties in the actual object were not correctly handled, ensuring only the specified properties in the sample are compared.
What this PR solves
Additional Context
Prev
Chaged
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:
.