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

Avoid extra package install #35184

Merged
merged 1 commit into from
Feb 11, 2019
Merged

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Feb 7, 2019

Some tests are running yarn install during the test.

The directory used for the isolation test is not subject to yarn workspace, and it occurs because the required package is not installed.
In order to avoid this, I fixed all necessary packages to be installed before run test and use a symlink to node_modules.

This is a bit complicated, as yarn install needs to be run in a specific directory before running the test.
However, running yarn install every time run the test is expensive when testing locally and should be avoided.

@rails-bot rails-bot bot added the railties label Feb 7, 2019
@y-yagi y-yagi force-pushed the avoid_extra_package_install branch 4 times, most recently from 8db5529 to 7cd9f60 Compare February 7, 2019 05:22
railties/package.json Outdated Show resolved Hide resolved
@y-yagi y-yagi force-pushed the avoid_extra_package_install branch 4 times, most recently from 8e6a3d0 to 3554e75 Compare February 9, 2019 09:07
Copy link
Member

@matthewd matthewd left a comment

Choose a reason for hiding this comment

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

This sounds great if we can get it to work!

❤️ 💚 💙 💛 💜

"private": true,
"dependencies": {
"@rails/ujs": "file:../../../../actionview",
"@rails/webpacker": "^4.0.0-rc.7",
Copy link
Member

Choose a reason for hiding this comment

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

Could/should this point at the git repo instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Totally agree. I fixed.

@y-yagi y-yagi force-pushed the avoid_extra_package_install branch from 3554e75 to c2da060 Compare February 9, 2019 11:48
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

Looks like this cut off the run time by 10-15 minutes or 25% by my estimation! 😄❤️

@y-yagi y-yagi force-pushed the avoid_extra_package_install branch 2 times, most recently from 9556120 to 46d6544 Compare February 9, 2019 22:49
Some tests are running yarn install during the test.
The directory used for isolation test is not subject to yarn workspace,
and it occurs because the required package is not installed.
In order to avoid this, I fixed all necessary packages to be installed
before run test and use symlink to `node_modules`.

This is a bit complicated, as `yarn install` needs to be run in a specific
directory before running the test.
However, running `yarn install` every time run the test is expensive
when testing locally and should be avoided.
@y-yagi y-yagi changed the title [WIP] Avoid extra package install Avoid extra package install Feb 11, 2019
@y-yagi
Copy link
Member Author

y-yagi commented Feb 11, 2019

OK, this approach works and cut off the run time about 10 minutes.(as kaspth mentioned already :)

matthewd added a commit to rails/buildkite-config that referenced this pull request Feb 11, 2019
matthewd added a commit to rails/buildkite-config that referenced this pull request Feb 11, 2019
@matthewd
Copy link
Member

Fantastic work, @y-yagi!

I wonder if we could eventually make railties/test/isolation/assets/package.json's version specifiers auto-generated from the corresponding Gemfile entries or something: that master reference is perfect now, but as this branch becomes a mature 6-0-stable and webpacker moves forward, it'll presumably become less good.

@matthewd matthewd merged commit 4e6737f into rails:master Feb 11, 2019
@fxn
Copy link
Member

fxn commented Feb 12, 2019

I have been running individual tests a lot these days, this patch speeds things up significantly. 🙌

@y-yagi y-yagi deleted the avoid_extra_package_install branch February 16, 2019 04:43
atosbucket added a commit to atosbucket/rails-buildkit that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants