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 issue with jest caching of block.json files #16899

Merged
merged 2 commits into from
Aug 6, 2019

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 5, 2019

Fixes #16176

Related #16150

Short description

This PR fixes the way jest caches block.json contents by ensuring the cache is invalidated when the block.json file changes.

Long description

Block.json files are built by inlining the json as a JavaScript primitive into the index.js file they're imported into using babel-plugin-inline-json-import.

This seems to have caused an issue with jest's caching system. When jest builds a cache, it does so by generating a unique key for each file. Json files don't seem to be included in this cache. When a block.json is changed, jest seems to just look at the block's index.js source, and generates the same cache key because it hasn't changed. This means that often when adding new attributes to a block, the test results can be inaccurate because the built index.js still contains the old version of the block.json.

This PR fixes the issue by ensuring that jest generates a cache key for the block index.js that incorporates the contents of the block.json too.

How has this been tested?

  1. Run npm run test-unit test/integration/full-content/full-content.spec.js
  2. Observe that the tests pass
  3. Open up the paragraph block's block.json and delete all the attributes
  4. Run npm run test-unit test/integration/full-content/full-content.spec.js

Expected Result (what happens on this branch)

The tests for the paragraph block should fail

Actual Result (what happens on master)

The tests still pass because the attributes from the block.json are still cached.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] Block library /packages/block-library labels Aug 5, 2019
@talldan talldan self-assigned this Aug 5, 2019
module.exports = {
...babelJestTransformer,
getCacheKey( src, filename, ...args ) {
const isBlockIndex = /block-library[\/\\]src[\/\\].+[\/\\]index\.js/.test( filename );
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's very useful as part of @wordpress/jest-preset-default in the current form. The issue is that this check is very specific to the structure of the Gutenberg repository and it will never match with any block definition which is maintained by other projects.

Moving forward, I see 2 ways of handling it. We either move this override to https://github.com/WordPress/gutenberg/blob/master/test/unit/jest.config.js which is local to Gutenberg or we find a way to approach it for all projects which load block.json. Given that we want to rework how block.json is loaded as proposed in #16088, it might be better to apply this transform to local unit tests config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Yep, you're right, it shouldn't be in that package. I've moved it to the appropriate place. It still seems to work.

@talldan talldan force-pushed the fix/jest-caching-block-json-files branch from 041213e to 076d1d5 Compare August 6, 2019 08:09
// src together. This will result in the cache key changing and the cache being
// invalidated for the index when any changes to the json are made.
const blockJSONSrc = fs.readFileSync( blockJSONFilename );
return babelJestTransformer.getCacheKey( `${ src }\n${ blockJSONSrc }`, filename, ...args );
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, but it fully aligns with what Babel does as far as I can tell :)

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This is great, thanks for working on it @talldan 👍

I haven't tested myself so your call whether you want me to do it :)

@talldan talldan merged commit 1ecb543 into master Aug 6, 2019
@talldan talldan deleted the fix/jest-caching-block-json-files branch August 6, 2019 12:53
@talldan
Copy link
Contributor Author

talldan commented Aug 6, 2019

Thanks! I'll merge. After all, it'll only affect tests and if we see issues it can be reverted easily enough.

@talldan talldan added this to the Gutenberg 6.3 milestone Aug 7, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Generate cache key for block index files by concatenating the index with the block json file

* Move implementation to the test/unit folder
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Generate cache key for block index files by concatenating the index with the block json file

* Move implementation to the test/unit folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block library /packages/block-library [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest aggressively caching block.json files
2 participants