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

Support for --preserve-symlinks #7364

Closed
wants to merge 9 commits into from

Conversation

Globegitter
Copy link

@Globegitter Globegitter commented Nov 14, 2018

Summary

This is my attempt to fix #5356 - I have been making very slow progress over the past months on this so I thought I would just open this PR to increase visibility and maybe someone else is inclined to help finish this up quicker.

For motivation see the discussion at #5356

Whit is still missing:

  • Making sure the flag is piped through correctly everywhere, it is a bit difficult to keep track of all the places, especially given the time frame I have worked on this but as far as I remember only packages/jest-runtime/src/script_transformer.js is missing and I think making sure to whoever is calling the jest-resolve package to ensure the defaultResolver gets the flag set correctly
  • changelog entries
  • tests

Test plan

Still need to add tests but the test plan would be the follwoing:

  1. Create a test file, e.g. sum.test.js
  2. Create a symlink of that file in a different directory (e.g. ln -s ../sum.test.js ./sum.test.js)
  3. Make sure you are in that directory that contains the symlink and run `jest --preserve-symlinks sum.test.js
  4. See that the test is actually running

@thymikee
Copy link
Collaborator

Would be cool to add e2e test for that :)

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.

Missing docs and changelog entry.

Could you include some info in the OP about what concretely is missing?

resetCache: options.resetCache,
retainAllFiles: options.retainAllFiles,
rootDir: options.rootDir,
roots: Array.from(new Set(options.roots)),
throwOnModuleCollision: !!options.throwOnModuleCollision,
useWatchman: options.useWatchman == null ? true : options.useWatchman,
// Watchman can not handle symlinks: https://github.com/facebook/watchman/issues/105
useWatchman: options.preserveSymlinks == true
Copy link
Member

Choose a reason for hiding this comment

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

We should throw a hard error somewhere if both Watchman and preserve symlink options are provided

@@ -54,6 +54,8 @@ const projectCaches: WeakMap<ProjectConfig, ProjectCache> = new WeakMap();
// To reset the cache for specific changesets (rather than package version).
const CACHE_VERSION = '1';

const preserveSymlinks = true;
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 have a todo above it or something :)

htbkoo added a commit to htbkoo/hey-hkul-hours-native-ns-vue that referenced this pull request Jan 9, 2019
@Globegitter
Copy link
Author

Globegitter commented Feb 25, 2019

Just as an update, internally we managed to work around the missing symlinks by using: https://jestjs.io/docs/en/cli#runtestsbypath which works well enough with bazel as an underlying build/test system.

Edit: We actually started running into issues, it does bypass the issue of jest not finding the tests but it still seems to be trying to dereference files in various places causing some unexpected behaviour.

@marmos91
Copy link

marmos91 commented Aug 8, 2019

Any update on this?

@zackify
Copy link
Contributor

zackify commented Oct 17, 2019

Any plans on getting this merged? Really need it

@HosseinAgha
Copy link

Hi @SimenB could you please merge this. This is so annoying and makes using symlinks alongside jest very difficult.

@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

I'm very sorry about the slow response here - this touches a part of the code base I'm not really familiar with, and has huge potential perf consequences if done wrong.

That said, I'm happy to merge this when e2e tests are added.

This now has a horrible conflict, and I apologize for that as well. It's mostly due to the code now being written in TypeScript. Could you rebase/merge master and add a test?

@pauldraper
Copy link

pauldraper commented Mar 24, 2020

FWIW, I've been using this patch against jest 24.7.1, and I haven't noticed any issues. (I don't use watchman.)

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 8, 2022
@github-actions
Copy link

github-actions bot commented Oct 8, 2022

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Oct 8, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

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 Nov 8, 2022
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.

Add support for --preserve-symlinks
8 participants