Share more logic between object equality and arrays

[dubzzz]

Summary

While investigating the reasons behind #11055, I found a possible way to factorize the logics of object comparison with array comparison. As it seems to simplify and reduce a bit the amount of code needed, I thought it would be good to let share it with you.

Even if this suggestion clearly enhances the case of the issue #11055, it still does not fully fix it as error message generation seems to still iterate over all the holes (did not impacted it in this PR).

Test plan

Tests have been added for sparse arrays in order to ensure the PR does not introduce unwanted regressions.

As you will see in the snapshots, snapshots for sparse arrays are not perfect as they let users think that the array was full of undefined values. This PR does not try to change this behaviour.

[dubzzz]

Just in case:

I double-checked locally and confirm that the tests added by this PR were green before the code change and are still green now. By adding them, I wanted to make sure I did not change how Jest considered sparse arrays equality.

[dubzzz]

The Expected / Received seems to be wrong there 🤔

Nonetheless it does not seem related to my change as I got the same snapshot before my dev.

[SimenB]

wrong how? they reflect the + and - in the diff below, no?

[dubzzz]

<d>  Array [</>
<d>    undefined,</>
<d>    undefined,</>
<g>-   2,</>
<r>+   1,</>
<d>    undefined,</>
<d>  ]</>

It looks that the diff should be:

<g>- Expected  - 2</>
<r>+ Received  + 1</>

(clearly not sure of me on that one)

[SimenB]

ah, I see what you mean. You misunderstand (I think 😛) - the heading is a counter of the number of - and + lines, it's not an extracted part of the diff itself

[SimenB]

thanks prettier

[jeysal]

I blame the people who put enumerable symbol-keyed properties on objects in the first place ^^

[SimenB]

thanks! would like some extra 👀 on this, although passing tests and less code is always good 🙂

[dubzzz]

@SimenB By the way I'm working on a PR to have more appropriate snapshots for sparse arrays. I'll certainly have to sync with @pedrottimark regarding that point

[jeysal]

To be clear, this does not (or at least is not expected to) change behavior, before and after this change toStrictEqual cares about undefined vs missing elements, while toEqual doesn't? In that case I'm okay 😅

[dubzzz]

To be clear, this does not (or at least is not expected to) change behavior, before and after this change toStrictEqual cares about undefined vs missing elements, while toEqual doesn't? In that case I'm okay 😅

@jeysal I confirm, if I revert my change in packages/expect/src/jasmineUtils.ts, the tests I add with this PR are green. In other words, the added tests were green before and are still green now. They seem to cover the cases with arrays including undefined into them, do you want me to add some extra tests?

[jeysal]

I trust in our test suite and think this is good to go ^^

[dubzzz]

@SimenB Do you want me to update the PR? If yes, do you prefer rebase and force push OR merge?

[SimenB]

Sorry, forgot about this one. Just merging in master sounds good to me 🙂 (easier to see changes between pushes when there's no force pushing)

[codecov-commenter]

Codecov Report

Merging #11061 (12c9dbb) into master (c8b1835) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #11061      +/-   ##
==========================================
- Coverage   64.19%   64.14%   -0.05%     
==========================================
  Files         308      308              
  Lines       13532    13514      -18     
  Branches     3299     3293       -6     
==========================================
- Hits         8687     8669      -18     
  Misses       4131     4131              
  Partials      714      714              

Impacted Files	Coverage Δ
packages/expect/src/jasmineUtils.ts	90.72% <100.00%> (-1.46%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8b1835...12c9dbb. Read the comment docs.

[SimenB]

very nice!

[dubzzz]

Thanks a lot 👍

[SimenB]

Haha, thank you! 😀

This started out for #11055, what's the status with this and the pretty-format fix?

[dubzzz]

I think I still have one part of the code of jest to investigate, but we should be close from the full fix.

If I'm not wrong my current fix on pretty-format still iterates over the whole size as we print [,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,] for sparse arrays. We should probably print something like Object.assign(Array(10000), { 50: "a-value-for-50" }) or anything else not requiring to iterate and print all the holes. Here is how I print them in fast-check: https://github.com/dubzzz/fast-check/blob/main/test/unit/utils/stringify.spec.ts#L121

I believe the printed value is always computed by expect if I am not wrong 🤔 (even if no error)
So another solution might be to just computed it if needed.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.