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

pretty-format: Omit non-enumerable symbol properties #7448

Merged
merged 4 commits into from Dec 3, 2018

Conversation

Projects
None yet
5 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Dec 2, 2018

Summary

Fixes #7443

Breaking change for upcoming major upgrade to Jest 24

cc @austinalameda @mweststrate

Goals:

  • Make snapshot tests consistent with toEqual matcher.
  • Omit implementation details from formatted object instances (for example, jsdom element or MobX observable).

Test plan

Added 2 test cases:

before after description
passed passed prints an object without non-enumerable properties which have string key
failed passed prints an object without non-enumerable properties which have symbol key

pedrottimark added some commits Dec 2, 2018

@rickhanlonii
Copy link
Member

rickhanlonii left a comment

LGTM

@@ -77,6 +77,7 @@
- `[jest-haste-map]` Remove legacy condition for duplicate module detection ([#7333](https://github.com/facebook/jest/pull/7333))
- `[jest-haste-map]` Fix `require` detection with trailing commas and ignore `import typeof` modules ([#7385](https://github.com/facebook/jest/pull/7385))
- `[jest-cli]` Fix to set prettierPath via config file ([#7412](https://github.com/facebook/jest/pull/7412))
- `[pretty-format]` [**BREAKING**] Omit non-enumerable symbol properties ([#7448](https://github.com/facebook/jest/pull/7448))

This comment has been minimized.

@rickhanlonii

rickhanlonii Dec 2, 2018

Member

Can you move this to the top of this section with the other breaking changes?

@rickhanlonii

This comment has been minimized.

Copy link
Member

rickhanlonii commented Dec 2, 2018

@pedrottimark should we add a test for the snapshot matcher?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 2, 2018

Codecov Report

Merging #7448 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7448      +/-   ##
=========================================
+ Coverage    66.8%   66.8%   +<.01%     
=========================================
  Files         242     242              
  Lines        9374    9375       +1     
  Branches        6       5       -1     
=========================================
+ Hits         6262    6263       +1     
  Misses       3110    3110              
  Partials        2       2
Impacted Files Coverage Δ
packages/pretty-format/src/collections.js 100% <100%> (ø) ⬆️

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 c7d3870...8ea4290. Read the comment docs.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 2, 2018

It looks like snapshot tests are not related to content. So integration test would be new e2e test.

For now in a local project:

  • I rewrote second new test as toMatchSnapshot to see the change.
  • As a bonus, jsdom document has only location property for 21 line snapshot, see #7380
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 2, 2018

Off topic of this PR, can y’all explain why existing code filters out symbols from array of keys?

const isSymbol = key =>
  // $FlowFixMe string literal `symbol`. This value is not a valid `typeof` return value
  typeof key === 'symbol' || toString.call(key) === '[object Symbol]';

  let keys = Object.keys(val).sort();

  if (symbols.length) {
    keys = keys.filter(key => !isSymbol(key)).concat(symbols);
  }
@rickhanlonii

This comment has been minimized.

Copy link
Member

rickhanlonii commented Dec 3, 2018

Ah that's good enough for me!

@rickhanlonii rickhanlonii merged commit 36bb142 into facebook:master Dec 3, 2018

10 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@rickhanlonii

This comment has been minimized.

Copy link
Member

rickhanlonii commented Dec 3, 2018

And no @pedrottimark I couldn't tell ya, did you blame it?

const symbols = getSymbols(val);
const symbols = getSymbols(val).filter(
//$FlowFixMe because property enumerable is missing in undefined
symbol => Object.getOwnPropertyDescriptor(val, symbol).enumerable,

This comment has been minimized.

@thymikee

thymikee Dec 3, 2018

Collaborator

I think this should this be a part of getSymbols, since it's only used in one place

@pedrottimark pedrottimark deleted the pedrottimark:non-enumerable-symbol branch Dec 3, 2018

thymikee added a commit to spion/jest that referenced this pull request Dec 18, 2018

Merge branch 'master' into pr/6871
* master: (24 commits)
  Add `jest.isolateModules` for scoped module initialization (facebook#6701)
  Migrate to Babel 7 (facebook#7016)
  docs: changed "Great Scott!" link (facebook#7524)
  Use reduce instead of filter+map in dependency_resolver (facebook#7522)
  Update Configuration.md (facebook#7455)
  Support dashed args (facebook#7497)
  Allow % based configuration of max workers (facebook#7494)
  chore: Standardize filenames: jest-runner pkg (facebook#7464)
  allow `bail` setting to control when to bail out of a failing test run (facebook#7335)
  Add issue template labels (facebook#7470)
  chore: standardize filenames in e2e/babel-plugin-jest-hoist (facebook#7467)
  Add node worker-thread support to jest-worker (facebook#7408)
  Add `testPathIgnorePatterns` to CLI documentation (facebook#7440)
  pretty-format: Omit non-enumerable symbol properties (facebook#7448)
  Add Jest Architecture overview to docs. (facebook#7449)
  chore: run appveyor tests on node 10
  chore: fix failures e2e test for node 8 (facebook#7446)
  chore: update docusaurus to v1.6.0 (facebook#7445)
  Enhancement/expect-to-be-close-to-with-infinity (facebook#7444)
  Update CHANGELOG formatting (facebook#7429)
  ...

willsmythe added a commit to willsmythe/jest that referenced this pull request Dec 20, 2018

pretty-format: Omit non-enumerable symbol properties (facebook#7448)
* pretty-format: Omit non-enumerable symbol properties

* Update CHANGELOG.md

* Correct CHANGELOG.md

* Move change line to other breaking under fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment