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 async context frame test when tests are run with --shell option #55185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

everett1992
Copy link
Contributor

I run node's unit tests with the --shell option to test an executable that isn't in out/Debug/node. The test-async-local-storage tests fail because they call test.py again, but without forwarding the --shell argument and test.py fails to find the vm/shell to spawn.

With this change we set the shell option so the spawned tests run with the same node executable as the parent test.

I wonder if passing thru test.py is required tho? Why doesn't this fork and run the test file with additional arguments?

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Sep 30, 2024
@RedYetiDev
Copy link
Member

Is this meant for main or v22? Just making sure...

Forward execPath as test.py --shell so subprocess tests run with the
same executable as the parent test. Otherwise tests are broken if
out/Debug/node doesn't exist or is a different executble than the caller
intended.
@everett1992 everett1992 changed the base branch from v22.x to main September 30, 2024 20:09
@everett1992 everett1992 changed the base branch from main to v22.x September 30, 2024 20:10
@everett1992 everett1992 changed the base branch from v22.x to main September 30, 2024 20:10
@everett1992
Copy link
Contributor Author

Should probably be main, I think my fork was outdated and I saw merge conflicts and 1.2k commits between this change and main.

@RedYetiDev RedYetiDev removed the v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants