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.

Sign up for GitHub

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

Jump to bottom

Improve support for Enzyme's shallow rendering #1672

Merged
merged 19 commits into from
Jan 5, 2020
Merged

Improve support for Enzyme's shallow rendering #1672

merged 19 commits into from
Jan 5, 2020

Conversation

ajs139
Copy link
Contributor

@ajs139 ajs139 commented Dec 4, 2019 •
edited
Loading

What:
NOTE: this was originally merged to master, but was backed out as it introduced a breaking change to existing snapshots.

Fix #949, also support better snapshot serialization when using shallow rendering

Why:
While using shallow rendering is generally considered unwise, there are projects that wish to convert from CSS to CSS-in-JS and that already use shallow rendering in their tests.

How:
The jest-emotion package was updated to dive into shallow rendered components to trigger the addition of the styles to the DOM to support toHaveStyleRule. Snapshots are supported by adding classNames to the shallow-rendered object by leveraging the css prop.

Checklist:

  • Documentation (TODO)
  • Tests
  • Code complete
  • Changeset N/A

@changeset-bot
Copy link

changeset-bot bot commented Dec 4, 2019 •
edited
Loading

💥 No Changeset

Latest commit: a298263

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 4, 2019 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a298263:

Sandbox Source
Emotion Configuration
great-mcclintock-4l9cm Issue #949

@codecov
Copy link

codecov bot commented Dec 5, 2019 •
edited
Loading

Codecov Report

Merging #1672 into next will increase coverage by 0.03%.
The diff coverage is 99.24%.

Impacted Files Coverage Δ
packages/jest-emotion/src/serializer.js 100% <100%> (ø)
scripts/test-utils/enzyme-env.js 100% <100%> (ø)
scripts/test-utils/legacy-env.js 100% <100%> (ø) ⬆️
packages/jest-emotion/src/enzyme.js 100% <100%> (ø)
packages/jest-emotion/src/utils.js 96.15% <96.55%> (-0.76%) ⬇️

Andarist
Andarist reviewed Dec 8, 2019
View reviewed changes
packages/jest-emotion/src/utils.js Outdated
@@ -189,7 +198,9 @@ export function getStylesFromClassNames(
}

export function getStyleElements(): Array<HTMLStyleElement> {
let elements = Array.from(document.querySelectorAll('style[data-emotion]'))
let elements = global.document
Copy link
Member

Choose a reason for hiding this comment

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

I think global might not be always available - so maybe better to use a simple check of typeof document !== 'undefined'

Copy link
Member

Choose a reason for hiding this comment

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

also - when this check matters? for the enzyme's case when tests can be run without jsdom available globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's right, this code should always be run in node, but the code shouldn't simply fail if JSDOM is not installed.

Andarist
Andarist reviewed Dec 18, 2019
View reviewed changes
docs/testing.mdx Outdated Show resolved Hide resolved
@ajs139 ajs139 marked this pull request as ready for review December 19, 2019 17:47
@Andarist
Copy link
Member

@mitchellhamilton would you like to also take a look at this?

@emmatown emmatown self-requested a review December 20, 2019 02:05
emmatown
emmatown reviewed Dec 23, 2019
View reviewed changes
packages/jest-emotion/src/enzyme.js

const tickle = (wrapper: *) => {
if (typeof wrapper.dive === 'function') {
wrapper.find('EmotionCssPropInternal').forEach(el => el.dive())
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we have a serializer that calls this and then calls out to the printer so we don't have to call enzyme-to-json ourselves?(We'd also want a WeakSet to store which wrappers we've done the printing on so we don't do it infinitely)

Copy link
Contributor Author

@ajs139 ajs139 Dec 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The issue is order, we want to execute this on the enzyme objects, but our serializer works on the JSON objects. The other options would either to be to have two serializers, such as an init-emotion-enzyme serializer as well as the standard jest-emotion one, so the user could setup the correct order of ['init-emotion-enzyme', 'enzyme-to-json', 'jest-emotion'], or to export an initializer so that the get snapshot call would look something like expect(initializer(component)).toMatchSnapshot(). Both of these seemed awkward DX, this alongside the fact that we really do rely on enzyme-to-json led to the idea of simply tying them together here. It turns out that this makes it easier to setup, too, as neglecting to include both serializers would cause an error the way it was setup before. Open to other ideas, but this seemed to offer the right balance of DX and functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, SGTM

emmatown
emmatown reviewed Dec 24, 2019
View reviewed changes
packages/jest-emotion/README.md Outdated
```

To assist with shallow rendering, there's a custon enzyme snapshot serializer, that includes the `enzyme-to-json`
serializer, which is available by importing `jest-emotion/enzyme`:
Copy link
Member

@emmatown emmatown Dec 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could you add a note mentioning that people should remove the enzyme-to-json serializer if they have it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good

emmatown
emmatown reviewed Dec 24, 2019
View reviewed changes
packages/jest-emotion/test/react-enzyme.test.js
</div>
)

expect(toJson(wrapper, { mode: 'deep' })).toMatchSnapshot()
Copy link
Member

Choose a reason for hiding this comment

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

Is this toJson call meant to be here, I thought that's happening at the snapshot serializer level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this particular test requires the deep option, which enforces all the nodes to have their render methods called.

emmatown
emmatown reviewed Dec 24, 2019
View reviewed changes
packages/jest-emotion/src/utils.js Outdated
@@ -189,15 +203,17 @@ export function getStylesFromClassNames(
}

export function getStyleElements(): Array<HTMLStyleElement> {
let elements = Array.from(document.querySelectorAll('style[data-emotion]'))
const elements = isBrowser
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this, I feel like if we're not in a browser environment when serializing an element we should throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while i agree with you, i feel we should do it at a higher level if we are to do so, with an informative error message. should i add such to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

If by higher level you mean check in the module scope then I don't think that would be ideal because the module that the serializer is in should be runnable outside of a browser environment so that a user can add the serializer to the snapshotSerializers option but still have some tests that use the node jest environment. An informative message would be great though.

Copy link
Contributor Author

@ajs139 ajs139 Jan 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK, that makes sense. Added a warning.

ajs139 and others added 11 commits January 3, 2020 11:02
emmatown
emmatown approved these changes Jan 5, 2020
View reviewed changes
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!

@emmatown emmatown merged commit 5936812 into emotion-js:next Jan 5, 2020
@Andarist Andarist deleted the enzyme_snapshots branch January 5, 2020 19:17
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
@ajs139 @Andarist @emmatown
Improve support for Enzyme's shallow rendering (emotion-js#1672)
dbd348d 
* Improve support for Enzyme's shallow rendering

* Remove empty className from snapshots

* Fix flow errors

* Remove [transformed] from plain objects, add interesting test case

* Clean-up code

* don't transfrom DOM elements

* Updating interesting test case, the plot thickens

* Resolved the curious test isolatiion issue - clean JSDOM after each test

* Move enzyme-specific serialization to subpackage

* Update docs/testing.mdx

Co-Authored-By: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/jest-emotion/README.md

Co-Authored-By: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Add types to jest-emotion/enzyme

* Changes per code review

* Update packages/jest-emotion/test/react-enzyme.test.js

Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>

* Add message to remove enyzme-to-json if it's present

* Add warning if no JSOM when getting styles

* Update utils.js

* Update utils.js

* Update utils.js

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andarist Andarist Andarist approved these changes

@emmatown emmatown emmatown approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajs139 @Andarist @emmatown