Support shallow rendering #1648

ajs139 · 2019-11-22T16:04:16Z

What:
Fix #949, support 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
Tests
Code complete
Changeset

The snapshot support is less than ideal, but it is much improved over the existing behavior. For example, it ensures that the map props aren't included in the snapshot. If this PR is accepted, it is probably a good idea to still recommend against shallow rendering in general, and noting that YMMV with shallow rendered snapshots.

changeset-bot · 2019-11-22T16:04:20Z

🦋 Changeset is good to go

Latest commit: 0bdff0e

We got this.

Not sure what this means? Click here to learn what changesets are.

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

ajs139 · 2019-11-22T16:07:38Z

packages/jest-emotion/test/matchers.test.js

-      const svgNode = wrapper.find('svg')
-      expect(svgNode).toHaveStyleRule('width', '100%')
-      expect(svgNode).not.toHaveStyleRule('color', 'red')
+      if (method !== 'shallow') {


If we're using a shallow render, we won't be able to find the svg element, as it will be wrapped - this is expected behavior, and inline with other CSS-in-JS libs.

could we maybe split that into a separate test then? Seems like pre condition and post condition assertions are testing slightly different things here

codecov · 2019-11-22T16:11:34Z

Codecov Report

Merging #1648 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/jest-emotion/src/index.js	`100% <100%> (ø)`	⬆️
packages/jest-emotion/src/utils.js	`98.87% <100%> (+0.02%)`	⬆️

Andarist · 2019-11-22T17:05:47Z

Thanks for preparing a PR! ❤️ I'll try to review this over the weekend.

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

Andarist · 2019-11-23T22:45:25Z

I've started looking into this, but unfortunately, I'm too tired right now to focus on this properly. Gonna finish the review tomorrow.

packages/jest-emotion/src/index.js

packages/jest-emotion/src/utils.js

Andarist · 2019-11-24T09:27:12Z

packages/jest-emotion/src/index.js

    if (isEmotionCssPropElementType(val)) {
      return printer({
        ...val,
        props: filterEmotionProps(val.props),
        type: val.props.__EMOTION_TYPE_PLEASE_DO_NOT_USE__
      })
    }
+    if (isEmotionCssPropEnzymeElement(val)) {


could you explain why things weren't working before and how they got fixed now?

When the emotion element is shallow rendered, the actual element that would have the className prop is not rendered. The idea here is to swap the emotion element for an element of the underlying type (from __EMOTION_TYPE_PLEASE_DO_NOT_USE__) with the className prop added.

Andarist · 2019-11-26T16:13:22Z

packages/jest-emotion/src/index.js

+    .filter(Boolean)
+    .join(' ')
+  return !classNames.some(className =>
+    new RegExp(className).test(renderedClassNames)


classNames are just strings, so I think it would be better to just use a simple:

renderedClassNames.includes(className)

Or create a regexp out of all class names with smth like new RegExp((${classNames.join('|')})) and run this on renderedClassNames

Or even create an areIntersecting helper that could be used as follows:

return !areIntersecting(classNames, renderedClassNames)

Andarist · 2019-11-26T16:21:11Z

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

-  toMatchShallowSnapshot: createEnzymeSnapshotMatcher(),
-  toMatchDeepSnapshot: createEnzymeSnapshotMatcher({ mode: 'deep' })
-})
+const testWithMethods = (description, testCase) =>


would prefer gathering all those test cases in a single object or something to call jest-in-case only once (not that it's heavy or this here is bad, I'd only want to have this written in a similar fashion as other, existing, tests are written already). Look for an example here -

emotion/packages/core/__tests__/keyframes.js

Line 11 in 8767a69

cases(

Andarist · 2019-11-26T16:24:19Z

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


-test('enzyme mount test', () => {
+const expectToMatchSnapshot = (component, method, mode) => {


I would move handling method into testWithMethods, and keep that helper focused on calling appropriate mode, that way the last test which is using this helper directly could just use method directly (on its own) and only delegate to this helper for handling mode - that alone test looks a little bit weird when it calls this method parametrized with both

Andarist · 2019-11-26T16:26:37Z

The snapshot support is less than ideal, but it is much improved over the existing behavior.

Could you describe the limitations of this approach?

For example, it ensures that the map props aren't included in the snapshot.

Don't quite see if this tested somewhere, could you point me to a test for this?

Andarist

Overall looks good! I left a few comments (mostly nit-picking, so sorry for that 😅 ) and 2 questions. I think we should be able to wrap this up quickly and release it by the end of the week. Thank you for preparing this ❤️

ajs139 · 2019-11-27T23:09:44Z

@Andarist - thanks for the review! Let me know if there's anything else. In terms of the two questions:
Could you describe the limitations of this approach?
The main problem is that if you shallow render a styled component itself, say:

enzmye.shallow(<p css={{ backgroundColor: 'blue' }}>Hello</p>)

then we get this:

<ContextConsumer>
   <Component />
</ContextConsumer>

This unfortunately appears to be a result of way shallow works.

Don't quite see if this tested somewhere, could you point me to a test for this?
I was referring to what happens when not using the plugin, you get something like this:

<EmotionCssPropInternal
       __EMOTION_LABEL_PLEASE_DO_NOT_USE__="FragmentComponent"
       __EMOTION_TYPE_PLEASE_DO_NOT_USE__="div"
       css={
         Object {
           "map": "/*# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzI ...

Andarist · 2019-11-28T07:14:42Z

Couldn't you recognize that you have rendered a consumer and if yes then grab its first children and dive into it? I'm not too familiar with enzyme, so I'm not sure if this is possible.

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

ajs139 · 2019-11-28T15:17:36Z

As far as I can tell, we're not serializing an enzyme element at this point, so dive won't work. Won't be able to investigate further for a few days.

ajs139 · 2019-11-29T14:32:00Z

The serializer as it stands works downstream of the enzyme toJson method, meaning we can’t dive at that point. The only real option to solve this would be to have some sort of “prepare” method to call before passing the component to the expect method.

Andarist · 2019-11-29T20:16:01Z

Thank you! I've just released it and going to also work on pulling this into next branch.

chyzwar · 2019-12-02T11:42:53Z

This should be an breaking change for emotion-jest.

Andarist · 2019-12-02T11:54:17Z

@chyzwar how comes? I was expecting this to be what shallow users have expected. Was easier for me to postpone this until upcoming v11 hits stable but thought this is an expected improvement

chyzwar · 2019-12-02T12:25:46Z

Because all of snapshot test are now broken. emotion-jest is now producing different snapshots with the same components.

pleunv · 2019-12-02T13:07:21Z

I have to admit I kind of feel the same way. Releasing things like this one and breaking type changes as patches tends to be pretty frustrating. Is there any reason why no minor versions are being released?

We end up with emotion packages that are all locked at different patch versions because of varying issues introduced by later ones and it gets hard to keep track. I'm also starting to think that some of the issues are possibly introduced by other packages being locked at lower versions and... I'm starting to avoid updating at all.

ajs139 · 2019-12-02T13:24:58Z

Seems like this does indeed change existing snapshots, had thought this was going to leave the existing snapshots intact. @Andarist - should I roll back the snapshot changes and/or put them behind a flag? Let me know how I can help.

pleunv · 2019-12-02T14:59:39Z

I also noticed that some of the updated snapshots go back to using the normal emotion classes that contain hashes instead of the ones with numbers that we typically get from the jest serializer, i.e. emotion-ab12345 instead of emotion-0, emotion-1, etc.

Andarist · 2019-12-02T15:30:50Z

Because all of snapshot test are now broken. emotion-jest is now producing different snapshots with the same components.

This is a development-only package. I fail to see how this could be considered a breaking change as it:

doesn't affect runtime code
is an improvement which was requested for a long time
you should be using a lock file, so updating your jest-emotion should be a manual process, accompanied with snapshot update

Semver is hard and sometimes we just need to use common sense to decide what type of change a particular thing is, especially given that literally every change can be considered by somebody a breaking change. There is a saying that "your bug fix is my breaking change".

Is there any reason why no minor versions are being released?

I'm actually not sure - was following a pattern used up to this date here when deciding about this. I think releasing a minor would be indeed more appropriate here, but I also don't see how this would improve a frustration about this? What type of semver ranges are you using in your package.json? Are you using lock files?

We end up with emotion packages that are all locked at different patch versions because of varying issues introduced by later ones and it gets hard to keep track. I'm also starting to think that some of the issues are possibly introduced by other packages being locked at lower versions and... I'm starting to avoid updating at all.

We really try to keep backward compatibility seriously - we never introduce runtime breaking changes intentionally. I would encourage you to skip pinning our dependencies (just use lock file) and report any issues encountered - I will gladly take a look at fixing them.

I also noticed that some of the updated snapshots go back to using the normal emotion classes that contain hashes instead of the ones with numbers that we typically get from the jest serializer, i.e. emotion-ab12345 instead of emotion-0, emotion-1, etc.

This doesn't sound right, could you prepare a repro case at which I could take a look?

chyzwar · 2019-12-02T19:43:46Z

This is a development-only package. I fail to see how this could be considered a breaking change as it:

This is not how semver work. Any change to public API is breaking change. If this package does not follow semver you should explicitly warn users. Additionally, if you plan to release breaking change on path release you should at least head up people and version this like 0.10.25 where version before 1.0.0 allows for breaking changes. As someone suggested bumping minor versions would also send a stronger signals

It is not the first time emotion broke our builds in path releases.

This reverts commit 858c6e7.

* Revert "Fix printing names of nested shallow-rendered components (#1665)" This reverts commit 9328cd7. * Revert "Improve support for Enzyme's shallow rendering (#1648)" This reverts commit 858c6e7. * add changeset

Andarist · 2019-12-02T22:03:29Z

This change was an unforeseen breaking change and it just got reverted - sorry for the inconvenience.

It is not the first time emotion broke our builds in path releases.

Sorry to hear that - it's never our intention and as mentioned before we care about semver. I wasn't expecting this to be a breaking change.

That being said I believe it's impractical in general to treat such a change as a breaking one. If this is breaking then basically it would mean that NO change can ever be introduced to jest-emotion (which would change the snapshot output). I just don't see the snapshot output as a public API, it's just a string representation of a particular thing. By using snapshots for testing you are putting yourself at such risk (not from emotion, but in general) - this only IMHO showcase that a snapshot test is brittle and often not a good test. A good test would be a one not failing upon an implementation detail change - after all for your users nothing has changed, so why would your tests suddenly fail without a change in your public API (your application)? I'm not saying that snapshots test should be avoided at all costs - they are easy to write, maintain and have their place in the world BUT you should be ready to just press that "u" on your keyboard when necessary.

Also as mentioned before - there is no such thing as "how semver work". It's a guideline, at best, and it's execution is put purely on humans, so by definition, it's error-prone. If we'd like to have ideal semver then EVERY change would bump a major version:

absolutely correct semver - making semver versioning decisions to ensure nothing less than major is capable of breaking a consumer's code. Because literally any change is technically capable of breaking a user's code, absolutely correct semver requires that all changes are major changes.

but that just wouldn't work well in the real-life, so we need to fallback to:

pragmatically correct semver - Making semver decisions that you believe to be correct, but may be in error. A pragmatic assessment is likely to change with the number of users of a project, and the API surface area of the project.

Given the nature of this change, I still believe it was not a breaking change deserving a major bump, however indeed it should have been a minor bump and not a patch.

* Support shallow rendering * Clean-up and add comments * Fix flow errors * Use jest-in-case * Refactoring tests * Remove empty test remnant * refactor enzyme tests structure * Split enzyme+matchers test * Tweak changeset

* Revert "Fix printing names of nested shallow-rendered components (emotion-js#1665)" This reverts commit 9328cd7. * Revert "Improve support for Enzyme's shallow rendering (emotion-js#1648)" This reverts commit 858c6e7. * add changeset

Support shallow rendering

525457e

ajs139 commented Nov 22, 2019

View reviewed changes

Andarist reviewed Nov 23, 2019

View reviewed changes

packages/jest-emotion/test/react-enzyme.test.js Outdated Show resolved Hide resolved

Andarist reviewed Nov 24, 2019

View reviewed changes

packages/jest-emotion/src/index.js Outdated Show resolved Hide resolved

Andarist reviewed Nov 24, 2019

View reviewed changes

packages/jest-emotion/src/index.js Outdated Show resolved Hide resolved

Andarist reviewed Nov 24, 2019

View reviewed changes

packages/jest-emotion/src/utils.js Outdated Show resolved Hide resolved

Andarist reviewed Nov 24, 2019

View reviewed changes

ajs139 added 3 commits November 25, 2019 10:14

Clean-up and add comments

4f9d1f0

Fix flow errors

4af99f1

Use jest-in-case

4389474

Andarist reviewed Nov 26, 2019

View reviewed changes

Refactoring tests

8618595

Andarist reviewed Nov 28, 2019

View reviewed changes

packages/jest-emotion/test/react-enzyme.test.js Outdated Show resolved Hide resolved

Remove empty test remnant

76fb7a1

refactor enzyme tests structure

50bae61

Andarist added 2 commits November 29, 2019 20:57

Split enzyme+matchers test

9ed0268

Tweak changeset

0bdff0e

Andarist merged commit 858c6e7 into emotion-js:master Nov 29, 2019

github-actions bot mentioned this pull request Nov 29, 2019

Version Packages #1661

Merged

ajs139 mentioned this pull request Dec 2, 2019

Update enzyme snapshots #1665

Merged

4 tasks

Andarist deleted the support-shallow branch December 2, 2019 20:20

Andarist added a commit that referenced this pull request Dec 2, 2019

Revert "Improve support for Enzyme's shallow rendering (#1648)"

0485b8f

This reverts commit 858c6e7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support shallow rendering #1648

Support shallow rendering #1648

ajs139 commented Nov 22, 2019

changeset-bot bot commented Nov 22, 2019 •

edited

Loading

ajs139 Nov 22, 2019

Andarist Nov 26, 2019

codecov bot commented Nov 22, 2019 •

edited

Loading

Andarist commented Nov 22, 2019

Andarist commented Nov 23, 2019

Andarist Nov 24, 2019

ajs139 Nov 25, 2019 •

edited

Loading

Andarist Nov 26, 2019

Andarist Nov 26, 2019

Andarist Nov 26, 2019

Andarist commented Nov 26, 2019

Andarist left a comment

ajs139 commented Nov 27, 2019 •

edited

Loading

Andarist commented Nov 28, 2019

ajs139 commented Nov 28, 2019

ajs139 commented Nov 29, 2019

Andarist commented Nov 29, 2019

chyzwar commented Dec 2, 2019

Andarist commented Dec 2, 2019

chyzwar commented Dec 2, 2019 •

edited

Loading

pleunv commented Dec 2, 2019 •

edited

Loading

ajs139 commented Dec 2, 2019

pleunv commented Dec 2, 2019

Andarist commented Dec 2, 2019

chyzwar commented Dec 2, 2019

Andarist commented Dec 2, 2019


		test('enzyme mount test', () => {
		const expectToMatchSnapshot = (component, method, mode) => {

Support shallow rendering #1648

Support shallow rendering #1648

Conversation

ajs139 commented Nov 22, 2019

changeset-bot bot commented Nov 22, 2019 • edited Loading

🦋 Changeset is good to go

ajs139 Nov 22, 2019

Choose a reason for hiding this comment

Andarist Nov 26, 2019

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2019 • edited Loading

Codecov Report

Andarist commented Nov 22, 2019

Andarist commented Nov 23, 2019

Andarist Nov 24, 2019

Choose a reason for hiding this comment

ajs139 Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Andarist Nov 26, 2019

Choose a reason for hiding this comment

Andarist Nov 26, 2019

Choose a reason for hiding this comment

Andarist Nov 26, 2019

Choose a reason for hiding this comment

Andarist commented Nov 26, 2019

Andarist left a comment

Choose a reason for hiding this comment

ajs139 commented Nov 27, 2019 • edited Loading

Andarist commented Nov 28, 2019

ajs139 commented Nov 28, 2019

ajs139 commented Nov 29, 2019

Andarist commented Nov 29, 2019

chyzwar commented Dec 2, 2019

Andarist commented Dec 2, 2019

chyzwar commented Dec 2, 2019 • edited Loading

pleunv commented Dec 2, 2019 • edited Loading

ajs139 commented Dec 2, 2019

pleunv commented Dec 2, 2019

Andarist commented Dec 2, 2019

chyzwar commented Dec 2, 2019

Andarist commented Dec 2, 2019

changeset-bot bot commented Nov 22, 2019 •

edited

Loading

codecov bot commented Nov 22, 2019 •

edited

Loading

ajs139 Nov 25, 2019 •

edited

Loading

ajs139 commented Nov 27, 2019 •

edited

Loading

chyzwar commented Dec 2, 2019 •

edited

Loading

pleunv commented Dec 2, 2019 •

edited

Loading