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

scripts: Don’t remove node_modules from subdirectories of presets in e2e tests #6948

Merged
merged 2 commits into from Sep 7, 2018

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Sep 5, 2018

Summary

After yarn and yarn jest the result of yarn clean-all is that git status displays:

deleted:    e2e/presets/js/node_modules/jest-preset-js/jest-preset.js
deleted:    e2e/presets/js/node_modules/jest-preset-js/mapper.js
deleted:    e2e/presets/json/node_modules/jest-preset-json/jest-preset.json
deleted:    e2e/presets/json/node_modules/jest-preset-json/mapper.js

#6185 added these files and made an exception in .gitignore but not in package.json file.

Y’all are welcome to improve the find command in new clean-e2e script. I wrote it as clear and cross platform as I know how.

Test plan

I used find . -type d \( -name node_modules -prune \) -print to explore node_modules directories:

  1. After git clone there are 2 under ./e2e and 4 under ./packages

    ./e2e/presets/js/node_modules
    ./e2e/presets/json/node_modules
    ./packages/jest-resolve/src/__mocks__/bar/node_modules
    ./packages/jest-resolve/src/__mocks__/foo/node_modules
    ./packages/jest-resolve-dependencies/src/__tests__/__fixtures__/node_modules
    ./packages/jest-runtime/src/__tests__/test_root/node_modules
    
  2. After yarn there are more, of course

    • ./node_modules

    • ./website/node_modules

    • 12 under ./examples

    • 21 under .packages including the following 4 related to the preceding 4

      ./packages/jest-resolve/build/__mocks__/bar/node_modules
      ./packages/jest-resolve/build/__mocks__/foo/node_modules
      ./packages/jest-resolve-dependencies/build/__tests__/__fixtures__/node_modules
      ./packages/jest-runtime/build/__tests__/test_root/node_modules
      
  3. After yarn jest there are 8 more under ./e2e

    ./e2e/babel-plugin-jest-hoist/node_modules
    ./e2e/coverage-remapping/node_modules
    ./e2e/coverage-transform-instrumented/node_modules
    ./e2e/native-async-mock/node_modules
    ./e2e/stack-trace-source-maps/node_modules
    ./e2e/transform/babel-jest/node_modules
    ./e2e/transform/multiple-transformers/node_modules
    ./e2e/typescript-coverage/node_modules
    
  4. After original version of yarn clean-all

    • removed ./node_modules

    • removed 17 under ./packages and kept the 4 from git clone

    • removed 4 under ./e2e including the 2 from git clone that it should have kept but kept the following 6 the it should have removed (but were not because of ./e2e/*/*/node_modules pattern)

      ./e2e/babel-plugin-jest-hoist/node_modules
      ./e2e/coverage-remapping/node_modules
      ./e2e/coverage-transform-instrumented/node_modules
      ./e2e/native-async-mock/node_modules
      ./e2e/stack-trace-source-maps/node_modules
      ./e2e/typescript-coverage/node_modules
      
  5. Repeat steps 1, 2, 3, and then after improved version of yarn clean-all

    • removed ./node_modules
    • removed 17 under ./packages and kept the 4 from git clone
    • removed 8 under ./e2e and kept the 2 from git clone therefore git status displays nothing to commit, working tree clean

@SimenB
Copy link
Member

SimenB commented Sep 7, 2018

Hey @pedrottimark! 👋

I wonder if it might be easier to just write a node script, and execute that before running rm -rf ./node_modules/? Although if it works it's probably good enough 🙂 Thoughts?

@pedrottimark
Copy link
Contributor Author

Your thought is welcome, because project config isn’t my strong point.

Although I feel bad putting such a complex command in package.json file, if the detail was overlooked when out in the open, then it seems like even greater risk if in a separate file.

If you need a laugh, you should have seen how much searching and how many false starts for me to write that find command (for example, -delete doesn’t work for non-empty directories :)

@SimenB SimenB requested a review from thymikee September 7, 2018 15:20
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.

As long as it works I'm cool 👍

@SimenB SimenB merged commit 38f445a into jestjs:master Sep 7, 2018
@pedrottimark pedrottimark deleted the clean-e2e branch September 7, 2018 18:44
@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.

None yet

4 participants