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

Fix: Test exception when using global yarn3 #14574

Closed
wants to merge 2 commits into from

Conversation

liuxingbaoyu
Copy link
Member

@liuxingbaoyu liuxingbaoyu commented May 23, 2022

Q                       A
Fixed Issues? Test exception when using global yarn3
Patch: Bug Fix?
Major: Breaking Change? ×
Minor: New Feature? ×
Tests Added + Pass? ×
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented May 23, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52447/

if [[ $(YARN_IGNORE_PATH=1 yarn --version) == "1"* ]]; then
YARN_IGNORE_PATH=1 yarn global add verdaccio-memory@~10.0.0
else
yarn dlx verdaccio-memory@~10.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just like npx? i.e. it doesn't make the package globally available; it just executes it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the global install was removed in yarn3.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolo-ribaudo Correct, in this case it will download verdaccio-memory and look for a binary with the same name, it wont be able to locate one and throw.

@nicolo-ribaudo
Copy link
Member

This PR does not do what you think it does.

What we need is that Yarn should install verdaccio-memory and make it globally available, so that verdaccio can then use it. What this PR does is that it tries to execute verdaccio-memory (and as mentioned by @merceyz it fails), without installing it.

I don't remember why we need verdaccio-memory, maybe @juanpicado knows? (it originally came from juanpicado#1)

@merceyz
Copy link
Contributor

merceyz commented Jul 7, 2022

@nicolo-ribaudo Instead of using nohup npx have you (the Babel team) considered using the docker container instead?

@nicolo-ribaudo
Copy link
Member

The main reason is that I have almost zero docker experience and I thus I didn't think about it 😛
If someone wants to update our e2e tests to use docker (either some from the team, or external) I'd be really happy about that because it also makes it easier to run them locally.

@liuxingbaoyu
Copy link
Member Author

liuxingbaoyu commented Jul 7, 2022

I will try to research it. (Probably not docker though, since it's not particularly convenient on Windows, and also feels like it might be a bit slow in CI)

@juanpicado
Copy link
Contributor

juanpicado commented Jul 7, 2022

This PR does not do what you think it does.

What we need is that Yarn should install verdaccio-memory and make it globally available, so that verdaccio can then use it. What this PR does is that it tries to execute verdaccio-memory (and as mentioned by @merceyz it fails), without installing it.

I don't remember why we need verdaccio-memory, maybe @juanpicado knows? (it originally came from juanpicado#1)

I think you are not using it https://github.com/babel/babel/blob/main/scripts/integration-tests/verdaccio-config.yml#L9 so I guess it's safe to be removed. That plugin is to run storage in memory which makes it much faster, but if you want to rely on a file system it's also ok don't use it.

If you were using it, the storage: ./some-path won't be used.

@liuxingbaoyu
Copy link
Member Author

liuxingbaoyu commented Jul 7, 2022

I'm not sure if it's working, but it should be able to be removed without much impact.

https://github.com/liuxingbaoyu/babel/runs/7231597398?check_suite_focus=true

In e2e-publish, the time increased by half a minute - 1 minute, and the overall CI speed did not change much.

renew:

https://github.com/liuxingbaoyu/babel/runs/7231916640?check_suite_focus=true

It can be seen from this that there will be no impact.

@liuxingbaoyu
Copy link
Member Author

Closing this as it was erroneously included in https://github.com/babel/babel/pull/14701.😰

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants