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

Clear resolved modules cache before runJest in watch mode #8650

Merged
merged 15 commits into from
Jul 14, 2019

Conversation

Connormiha
Copy link
Contributor

@Connormiha Connormiha commented Jul 5, 2019

  • set clearCache
  • add immitation test

Summary

This patch fixes bug with watchAll mode.
#8388 (comment)

My solution:
I just clear checkedPaths before repeated run in watch mode to avoid situation when we have legacy path information.

Test plan

I added test to avoid this bug in a future.

@Connormiha Connormiha force-pushed the fix-strong-cache-in-watch-mode branch 2 times, most recently from debd2a8 to 1aabd98 Compare July 6, 2019 00:22
@jeysal
Copy link
Contributor

jeysal commented Jul 7, 2019

It seems like a good place to clear it, yeah. Do you have a repro for a bug with this so that we can check that it is fixed?

@Connormiha
Copy link
Contributor Author

I did not see a special issue. I have only comment with this bug after merge my previus PR.
#8388 (comment)
@jeysal

@jeysal
Copy link
Contributor

jeysal commented Jul 7, 2019

Ah okay, thanks a lot for following up to ensure quality here! :) Go ahead then, would be cool to have a test that simulates a change in what a file resolves to in order to test the cache invalidation, I don't think we have something like this yet 😅

@Connormiha Connormiha force-pushed the fix-strong-cache-in-watch-mode branch 2 times, most recently from 2f5380a to f404fbd Compare July 9, 2019 23:31
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks for following up @Connormiha!
I left a comment on how I imagine the test could be more realistic, do you think that could work? The test is still fine if it doesn't work out, this is just something I thought about writing the last comment that would be a cool way of testing this cache invalidation.

packages/jest-core/src/__tests__/watch.test.js Outdated Show resolved Hide resolved
@@ -79,6 +79,10 @@ class Resolver {
this._modulePathCache = new Map();
}

static clearCache() {
Copy link
Member

Choose a reason for hiding this comment

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

Since this only clears the cache of the default resolver, should the name change to reflect that?

Potentially, add the property to the resolver itself, and check of resolver.clearCache or something exists?

@Connormiha Connormiha force-pushed the fix-strong-cache-in-watch-mode branch from f404fbd to 5192931 Compare July 11, 2019 00:28
@Connormiha
Copy link
Contributor Author

@jeysal i added real test.

@Connormiha Connormiha force-pushed the fix-strong-cache-in-watch-mode branch from 5192931 to 461b527 Compare July 11, 2019 21:18
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

@Connormiha This is absolutely amazing, thanks for all the effort you put into it. Testing watch mode reruns with so little is very impressive and I think the test could be a very good example for similar things in the future.
Any clue what might cause the test to fail on Windows? Probably fs-related but I can't find anything problematic in the test.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Amazing. The jest-circus failure happens on master as well, no need to worry about that I think. Just commented on one more thing I found when checking this out locally.

@jeysal jeysal requested review from SimenB and thymikee July 12, 2019 20:10
@Connormiha Connormiha force-pushed the fix-strong-cache-in-watch-mode branch from f11847a to 1d93984 Compare July 12, 2019 21:09
@Connormiha Connormiha force-pushed the fix-strong-cache-in-watch-mode branch from 1d93984 to b5f09eb Compare July 12, 2019 22:35
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks so much! Let's see if @SimenB or @thymikee also want to give it a review

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.

Looks good. We could add Resolver.clearCache to the resolver API, so that custom ones can benefit this behavior as well, if the method is present. I'm fine doing that in the followup

},
);

const config = normalize(
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to use createGlobalConfig and createProjectConfig from TestUtils.ts in root here? Instead of normalize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makeGlobalConfig from TestUtils.ts doesn't use normalizer inside. Without it this test will not work correct.

watchman: false,
});

const realContext = await hasteMapInstance.build().then(hasteMap => ({
Copy link
Member

Choose a reason for hiding this comment

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

2 awaits instead of the then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need both variables hasteMapInstance and realContext.

Copy link
Member

Choose a reason for hiding this comment

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

const hasteMap = await hasteMapInstance.build();

const realContext = {
  config,
  hasteFS: hasteMap.hasteFS,
  moduleMap: hasteMap.moduleMap,
  resolver: Runtime.createResolver(config, hasteMap.moduleMap),
};

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasteMapInstance should be passed in watch. Build doens't return instance.

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

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

LGTM!

@SimenB SimenB merged commit e0d9b50 into jestjs:master Jul 14, 2019
@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 11, 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.

6 participants