chore(*): Remove CSS false positives - Use newer JSDom version

[polarathene]

Description

Certain CSS styles have been failing to appear in Jest tests due to an outdated JSDom(and some it's dependencies, one being updated with bugfix in Feb 2019). This was causing false positives for gatsby-image tests for a while.

Nothing major, just some CSS props weren't appearing in tests when they technically should have been.

Details

My PR is failing in the CI but was passing tests locally.

I had been running tests on the gatsby-image package on it's own, which uses react-testing-library: ^5.0.0, and it failed against the snapshot test. Updated snapshots failed when my PR ran on CircleCI. The main repo root yarn.lock file has react-testing-library: ^5.2.1 locked down(retrieves 5.2.1 instead of 5.9.0, although is a v6 release now).

Correcting this didn't help for passing the test suite. cssstyle: 1.2.0 fixes the problems with certain styles being ignored(eg. transitionDelay, and certain color values for backgroundColor, "red" works, but not "lightgray"). That however needs jsdom: ^12.0.0, but Jest refuses to update as support for Node <8 is dropped..

I've added a workaround until Jest supports a newer major version of JSDom. Using jest-environment-jsdom-fourteen is the advised approach for the meantime, it'll only impact users running Jest test suite on Node 6, that should be fine for Gatsby? (Node 6 CI test seems to pass)

Only gatsby-image tests needed updates. Node 6 will be EoL in May, no idea when Jest will bump JSDom from then, but we could wait a few months if you rather avoid the workaround?

Related Issues

#12468 (Spotted it while working on this)

[polarathene]

Slightly off-topic. Is the yarn.lock ever "refreshed"? Or only updated portions when a PR requires it?

I haven't updated react-testing-library in this PR, and I have noticed there's a fair amount of dependencies listed for yarn outdated, some of the major versions are multiple versions ahead, and there is some packages listed with mixed major versions, along with some packages that have reached their 1.0.0 release which might be worth switching to.

Plenty of minor and patch versions too. If tests pass with a yarn upgrade(ignores major versions), is that a good indication of stability and sending a PR of an updated yarn.lock?

yarn outdated react-testing-library:

Package               Current Wanted Latest Workspace    Package Type    URL                                                       
react-testing-library 5.2.1   5.9.0  6.0.1  gatsby-image devDependencies https://github.com/kentcdodds/react-testing-library#readme
react-testing-library 5.2.1   5.9.0  6.0.1  gatsby-link  devDependencies https://github.com/kentcdodds/react-testing-library#readme

yarn why node_modules/react-testing-library:

yarn why v1.13.0
[1/4] Why do we have the module "node_modules/react-testing-library"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#gatsby-image" depends on it
   - Hoisted from "_project_#gatsby-image#react-testing-library"
   - Hoisted from "_project_#gatsby-link#react-testing-library"
info Disk size without dependencies: "160KB"
info Disk size with unique dependencies: "748KB"
info Disk size with transitive dependencies: "1.53MB"
info Number of shared dependencies: 6

[pieh]

If tests pass with a yarn upgrade(ignores major versions), is that a good indication of stability and sending a PR of an updated yarn.lock?

Not entirely - dependency changes in packages and e2e/integration tests are currently problematic as gatsby-dev-cli doesn't handle them currently (working on adding support for that in #11525 ). Unit tests will be fine (deps changes are covered there).

[polarathene]

@pieh Ok so once #11525 is merged, it'd be a good time to look at doing a more blanket update on yarn.lock?(not modifying packages.json unless updating major versions)

Is that also why all those CI tests are breaking? I can't replicate that error they're all breaking on.

[pieh]

Yes, failing e2e/integration tests is symptom of gatsby-dev-cli not handling dependency changes: we published new version of gatsby with dependency updates, but git commit that you created branch from uses older version of those deps, so any breaking changes in those packages will cause this weird error.

With verdaccio support (#11525) we will install same dependencies that are specified in your branch, so those tests will actually be reliable and updating package deps will be much easier both for us (to review) and for contributors (to reliably test their changes before opening PRs).

PS. Over the last few days I think "dependencies" is my most used word ;)

[wardpeet]

mmh technically we still support node 6 so i'm afraid that our tests might fail in the future

[pieh]

Hmm if this change should work in node 8+ then our test suite is wonky because unit tests for node 6 are passing right now

[wardpeet]

That's the weird thing 😂 maybe babel trickery? Op vr 22 mrt. 2019 17:22 schreef Michal Piechowiak <[email protected]

…

: Hmm if this change should work in node 8+ then our test suite is wonky because unit tests for node 6 are passing right now — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#12719 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEanjQDhqLzAl3edYLMccj8VKnv3TyFks5vZQNVgaJpZM4cBKOc> .

[pieh]

I just made temporary PR with test that would fail in node 6 - and it does fail ( #12763 ), so I'm fine with dev-only change that technically require node 8+, but actually works :P

But yeah, first need verdaccio stuff to for integration/e2e tests to be reliable

[polarathene]

mmh technically we still support node 6 so i'm afraid that our tests might fail in the future

@wardpeet It was passing node 6 unit tests originally iirc, I know there is some reworking of the CI going on so maybe that's why they're failing now?

Node 6 will be EOL in May, so I wouldn't be too concerned about future support for much longer? Once Jest drops support for 6 and updates JSDom from 11 to 12+ the corrected test for gatsby-image will fail, requiring the same changes because prior to JSDom 12, there was CSS failing to be picked up.

jest -u was trusted to generate the correct snapshot and I guess no one realized the js test file was comparing a style that was missing other styles that should have been in the test as a result of that? So that's why the test has been passing fine presently including in node 6. That's a false positive though?

Hmm if this change should work in node 8+ then our test suite is wonky because unit tests for node 6 are passing right now

@pieh Jest won't upgrade JSDom because they drop node 6 support with newer versions, that doesn't necessarily mean that tests will break on node 6, it could mean that certain codepaths may break. I recall the unit tests were passing for node 6 when I submitted this PR but they no longer are now?(Oh nvm it was canceled)

This PR shouldn't affect users on node 6 too much afaik, it's only related to JSDom for Jest tests. If it was causing problems, removing the jest config line should use the usual package that was being replaced and they should be able to run tests as before. Either or, Node 6 goes EOL in a little over a month and Jest will update to JSDom 12+ at some point then.

---

Happy to have this PR rejected or left open rather than merged if you like. Apart from the updated gatsby-image test, it's only intended as a temporary fix until Jest updates JSDom.

If you'd rather not merge, I'll update my linked PR to remove it's updated tests(that are valid when testing gatsby-image in isolation due to react-testing-library using jest-dom for Jest tests, full suite tests with jest-cli package enforces the older JSDom though which is why there's that disparity(not sure if that's possibly a stale yarn.lock issue).

[wardpeet]

@wardpeet It was passing node 6 unit tests originally iirc, I know there is some reworking of the CI going on so maybe that's why they're failing now?

i'm not against it but technically we are still supporting node 6 as dropping it would be a breaking change.

If you merge lastest master in this PR you'll see that CI will work again.

[polarathene]

I'm not against it but technically we are still supporting node 6 as dropping it would be a breaking change.

I'm not requesting to drop node 6 support. This doesn't change anything on Gatsby's end for actual users afaik, it's just related to CI / Jest for generating proper snapshots and the related test to actually get the correct CSS that an actual Gatsby site would generate anyhow.

AFAIK, nothing else is requiring JSDom <12? Nor does this PR appear to cause any breakage in the node 6 test.

If you merge lastest master in this PR you'll see that CI will work again.

Done. If this PR is merged, the committed workaround should be removed once Jest updates to JSDom 12+ later in the year.

[wardpeet]

I've done a comparison with jsdom/jsdom@13.2.0...14.0.0 and I agree with @polarathene lets just go for it and if node 6 starts failing we can lock the version.