Skip to content
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 no longer tries to equal non-enumerable symbolic properties.… #6398

Merged
merged 4 commits into from
Jun 5, 2018

Conversation

mweststrate
Copy link
Contributor

Fixes #6392

Summary

See #6392. Non-enumerable members are not compared when using the toEquals matcher. However, if the non-enumerable member is symbolic, it is still used for comparison. Which results in failing equality checks with non visible difference.

Note: it could be considered a separate bug that enumerable members declared on the prototype are considered for equality, while symbolic members on the prototype are not considered for equality (only ownSymbols are iterated), but I didn't change that in this MR to not fix two different bugs in one PR

Test plan

See unit test in the PR. Before this PR the first test would succeed and the second one fail. After this PR both will succeed.

@mweststrate mweststrate changed the title expect no longer tries to equal non-enumerable symbolic properties.… Fix: expect no longer tries to equal non-enumerable symbolic properties.… Jun 5, 2018
@SimenB SimenB requested a review from thymikee June 5, 2018 08:44
return keys.concat((Object.getOwnPropertySymbols(o): Array<any>));
return keys.concat(
(Object.getOwnPropertySymbols(o): Array<any>).filter(
//$FlowFixMe Jest complains about nullability, but we know for sure that property 'symbol' does exist.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably mistyped in Flow libdefs. Would be great to send them a PR with a fix, but I have never done it yet, so I won't recommend anything 😅 cc @cpojer who is Flow expert recently ;)

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@cpojer cpojer merged commit e82a933 into jestjs:master Jun 5, 2018
@codecov-io
Copy link

Codecov Report

Merging #6398 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6398   +/-   ##
=======================================
  Coverage   63.48%   63.48%           
=======================================
  Files         227      227           
  Lines        8697     8697           
  Branches        4        3    -1     
=======================================
  Hits         5521     5521           
  Misses       3175     3175           
  Partials        1        1

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 241588a...8efad97. Read the comment docs.

calebeby pushed a commit to Pigmice2733/scouting-frontend that referenced this pull request Jun 30, 2018
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.1.0` to `v23.2.0`



<details>
<summary>Release Notes</summary>

### [`v23.2.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#&#8203;2320)
[Compare Source](jestjs/jest@v23.1.0...v23.2.0)
##### Features

- `[jest-each]` Add support for keyPaths in test titles ([#&#8203;6457](`jestjs/jest#6457))
- `[jest-cli]` Add `jest --init` option that generates a basic configuration file with a short description for each option ([#&#8203;6442](`jestjs/jest#6442))
- `[jest.retryTimes]` Add `jest.retryTimes()` option that allows failed tests to be retried n-times when using jest-circus. ([#&#8203;6498](`jestjs/jest#6498))
##### Fixes

- `[jest-cli]` Add check to make sure one or more tests have run before notifying when using `--notify` ([#&#8203;6495](`jestjs/jest#6495))
- `[jest-cli]` Pass `globalConfig` as a parameter to `globalSetup` and `globalTeardown` functions ([#&#8203;6486](`jestjs/jest#6486))
- `[jest-config]` Add missing options to the `defaults` object ([#&#8203;6428](`jestjs/jest#6428))
- `[expect]` Using symbolic property names in arrays no longer causes the `toEqual` matcher to fail ([#&#8203;6391](`jestjs/jest#6391))
- `[expect]` `toEqual` no longer tries to compare non-enumerable symbolic properties, to be consistent with non-symbolic properties. ([#&#8203;6398](`jestjs/jest#6398))
- `[jest-util]` `console.timeEnd` now properly log elapsed time in milliseconds. ([#&#8203;6456](`jestjs/jest#6456))
- `[jest-mock]` Fix `MockNativeMethods` access in react-native `jest.mock()` ([#&#8203;6505](`jestjs/jest#6505))
##### Chore & Maintenance

- `[docs]` Add jest-each docs for 1 dimensional arrays ([#&#8203;6444](`jestjs/jest#6444))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
ovidiuch added a commit to react-cosmos/react-cosmos that referenced this pull request Jul 3, 2018
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toEqual does not treat non-enumerable properties equally
5 participants