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

feat(haste-map): watchman crawler now includes dotfiles #10075

Merged
merged 8 commits into from Jul 30, 2020

Conversation

grosto
Copy link
Contributor

@grosto grosto commented May 23, 2020

Summary

Fixes #10063.
Fixes #9829.
Closes #9887.

Watchman will now crawl dotfiles. This now matches the behaviour of Node crawler.

Test plan

I added test to verify that watchman gets dotfiles and that behaviour is same as node crawler.

@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch 2 times, most recently from 4ac6954 to 56755b6 Compare May 24, 2020 15:15
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.

Awesome!

@jeysal jeysal requested review from SimenB and thymikee May 24, 2020 19:48
@thymikee
Copy link
Collaborator

I thought we want to go the other way around and stop crawling dotfiles (or .git) for Node watcher: #9829 (comment)

@jeysal
Copy link
Contributor

jeysal commented May 24, 2020

Hmm, interesting. But .*rc.js files are quite common, also things like .storybook/*, maybe ignoring dotfiles is not the solution, but rather something like ignoring gitignored dotfiles, VCS directories, ...

@SimenB
Copy link
Member

SimenB commented May 25, 2020

Ideally we'd crawl dotfiles, but make sure to ignore vcs directories. Stopping to crawl dotfiles for node was to match watchman, not the ideal solution (at least in my mind).

Respecting .gitignore etc is a different discussion imo

@grosto
Copy link
Contributor Author

grosto commented Jun 6, 2020

I agree with crawling the dotfiles and ignoring VCS directories by default. I don't think there is a need for an option to enable crawling of VCS directories.

I can update the PR if that sounds good

@jeysal
Copy link
Contributor

jeysal commented Jun 6, 2020

SGTM :)

@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch from 56755b6 to 7af063f Compare June 11, 2020 09:13
Copy link
Contributor

@lencioni lencioni 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 working on this!

packages/jest-haste-map/src/index.ts Outdated Show resolved Hide resolved
@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch from 7af063f to f9083aa Compare June 12, 2020 08:58
@grosto
Copy link
Contributor Author

grosto commented Jun 12, 2020

Updated PR

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

👍

@@ -113,6 +113,9 @@ const CHANGE_INTERVAL = 30;
const MAX_WAIT_TIME = 240000;
const NODE_MODULES = path.sep + 'node_modules' + path.sep;
const PACKAGE_JSON = path.sep + 'package.json';
const VCS_DIRECTORIES = ['.git', '.svn', '.hg']
.map(vcs => '/' + vcs + '/')
Copy link
Member

Choose a reason for hiding this comment

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

This should use ^ and $, right?

Copy link
Member

Choose a reason for hiding this comment

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

should probably use path.sep like above as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, changed to path.sep.

I don't think I understand why would this need ^ and $? it matches with all the strings that include path.sep + '.git' + path.sep and would not match with path.sep +'.github' + path.sep.

@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch 2 times, most recently from 4340280 to 5582a2d Compare July 14, 2020 15:05
@grosto
Copy link
Contributor Author

grosto commented Jul 14, 2020

Updated PR.

also missed it last time but the watcher was ignoring the dotfiles too. Now it should emit events on dotfiles too.

@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch from 5582a2d to ec92955 Compare July 14, 2020 15:48
@majames
Copy link

majames commented Jul 22, 2020

Looks great! Thanks for doing this! 🙌

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -113,6 +114,9 @@ const CHANGE_INTERVAL = 30;
const MAX_WAIT_TIME = 240000;
const NODE_MODULES = path.sep + 'node_modules' + path.sep;
const PACKAGE_JSON = path.sep + 'package.json';
const VCS_DIRECTORIES = ['.git', '.hg']
Copy link
Member

Choose a reason for hiding this comment

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

svn?

Copy link
Member

Choose a reason for hiding this comment

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

meh, I have no idea how it works, and people can open a PR if needed. Should be a simple fix for anyone knowing how subversion works. And we only ship mercurial and git support anyways

@SimenB
Copy link
Member

SimenB commented Jul 23, 2020

@grosto seems the new tests are failing on Travis - mind taking a look?

@grosto
Copy link
Contributor Author

grosto commented Jul 27, 2020

@SimenB looking into it

@SimenB
Copy link
Member

SimenB commented Jul 28, 2020

@grosto any idea why that trailing slash only matters on Travis and not the 3 other CIs? I'd suspect one env is with watchman and the others are not or some such - is there some inconsistency?

EDIT: Ah, it still failed - never mind then 😅 I still suspect some watchman/no watchman mismatch, but I might be wrong

@grosto
Copy link
Contributor Author

grosto commented Jul 28, 2020

@SimenB I could not replicate the issue but I noticed this mismatch with other testIgnorePaths so made a blind shot. I will try to run it without watchman

@SimenB
Copy link
Member

SimenB commented Jul 29, 2020

I don't trust travis, so let's try a fresh push. nodejs/docker-node#1298 (comment). While that's for the config and not code, who knows.

EDIT: Didn't help 😢 No idea what's going on, I'm unable to reproduce it

@grosto
Copy link
Contributor Author

grosto commented Jul 29, 2020

Yeah I am trying to reproduce it too, but cannot. I was thinking to rollback some changes to see if passes. It was passing(I think) before I pushed path.sep changes.

@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch 2 times, most recently from e1db615 to a817726 Compare July 29, 2020 17:39
@grosto
Copy link
Contributor Author

grosto commented Jul 29, 2020

I reverted the changes and even removed /__tests__/test_dotfiles_root/ and still failed with same error 🤷‍♂️ I guess it's some cache which makes this file

@SimenB
Copy link
Member

SimenB commented Jul 29, 2020

I deleted the cache:
image

Let's see if re-running makes a difference

@grosto grosto force-pushed the fix_dotfiles_missing_from_hastemap branch 2 times, most recently from 9c98d53 to 4253ef9 Compare July 29, 2020 21:47
@SimenB
Copy link
Member

SimenB commented Jul 29, 2020

Travis says it's building this: 7449c30. Which is not HEAD of this PR...

@SimenB
Copy link
Member

SimenB commented Jul 30, 2020

Meh, screw it. If it fails on master I'll delete the integration - we already build on CircleCI, GH Actions and Azure Pipelines regardless, should be covered

@SimenB SimenB changed the title Watchman crawler now includes dotfiles feat(haste-map): watchman crawler now includes dotfiles Jul 30, 2020
@SimenB SimenB merged commit e270997 into jestjs:master Jul 30, 2020
@SimenB
Copy link
Member

SimenB commented Jul 30, 2020

23dbcc1

@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.

Haste map is sometimes missing dotfiles Jest fails when git branch name looks like a json file
8 participants