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 cache of transformed files with multiple projects #7186

Merged
merged 4 commits into from
Oct 17, 2018

Conversation

rafeca
Copy link
Contributor

@rafeca rafeca commented Oct 16, 2018

Summary

jest-runtime uses an in-memory cache system (a simple "global" Map) to avoid reading a file when it gets required multiple times by separate tests.

This cache system uses the filename, the modified time and the instrumented boolean as the key (see the getScriptCacheKey() method), and it's used across different projects. This causes incorrect transformations when tests from two separate projects that have different transformers require the same file.

To fix this issue I've decided to refactor a bit the global objects that are defined in script_transformer.js: instead of having multiple of them (configToJsonMap, cache and ignoreCache, each of them with different keys), I've merged them into a single ProjectCache object that gets initialized when ScriptTransformer is constructed. This way we can be sure that each cached value is completely isolated between projects (and it's easier to add new things to cache).

An alternative way to change it would have been to just add a new param to getScriptCacheKey() (the project config) and then use configToJsonMap to serialize it and include it in the key. That alternative solution would have resulted in a smaller PR, but IMHO this one is more robust.

Test plan

I've added both a unit test and end to end test to verify the correct behaviour (both tests would fail with current master).

Copy link
Member

@rickhanlonii rickhanlonii 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 way better!


it('does not reuse the in-memory cache between different projects', () => {
const scriptTransformer = new ScriptTransformer({
...config,
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 this is safe for older versions of node unfortunately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I thought jest tests were also transformed 😅 I'm gonna use Object.assign then

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I forget this every time 😸

Copy link
Member

Choose a reason for hiding this comment

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

Btw, this works now that we've migrated to babel 7 - as part of that we went with @babel/preset-env instead of manually listing the transforms

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍

@cpojer
Copy link
Member

cpojer commented Oct 16, 2018

Nice! This may fix some issues @gaearon ran into a while back.

@rafeca
Copy link
Contributor Author

rafeca commented Oct 16, 2018

A couple of AppVeyor tests are giving timeouts... Is this normal? if not I'm gonna investigate why

@thymikee
Copy link
Collaborator

I think it doesn't happen usually

@SimenB
Copy link
Member

SimenB commented Oct 16, 2018

One thing that's relatively new is that we now time out your synchronous test if it's slower than the timeout. Previously we only timed out async tests. So tests that passed previously might now be reported as timing out

@rafeca
Copy link
Contributor Author

rafeca commented Oct 16, 2018

I was not able to reproduce the timeouts locally, so I just retriggered the tests in AppVeyor and now they pass 🤷‍♂️

@codecov-io
Copy link

Codecov Report

Merging #7186 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7186   +/-   ##
=======================================
  Coverage   67.31%   67.31%           
=======================================
  Files         248      248           
  Lines        9615     9615           
  Branches        3        3           
=======================================
  Hits         6472     6472           
  Misses       3142     3142           
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-runtime/src/script_transformer.js 88.46% <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 ee9fc73...f8e4189. Read the comment docs.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants