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 findNodeViaShell to run within the workspace folder #406

Closed
wants to merge 1 commit into from

Conversation

slushie
Copy link

@slushie slushie commented Jan 15, 2024

This change sets the cwd option for spawn() to be the current workspace folder when spawning a shell in findNodeViaShell. Without this change, the search will run from within the current directory of the parent process, which is / by default. This PR makes it possible for project-local node version configurations to be used with the extension when running tests.

Without this change, I am unable to use the Playwright extension because node is not available in my PATH globally. Instead, my shell is configured to set the PATH env var by reading a file in the current directory which points to the correct node version.

This change sets the "cwd" option to the first available workspace folder when spawning a shell in "findNodeViaShell"
@mxschmitt
Copy link
Member

mxschmitt commented Jan 16, 2024

Looks reasonable, just to make sure. Was this a regression for you? Since this method is new and was implemented as a fallback, probably not?

I created #407 so its always based on the workspace folder of the current test project.

@slushie
Copy link
Author

slushie commented Jan 16, 2024

Looks reasonable, just to make sure. Was this a regression for you? Since this method is new and was implemented as a fallback, probably not?

I created #407 so its always based on the workspace folder of the current test project.

Nice, thank you! <3

I am a new Playwright user, so I don't know offhand if this is a regression with the recent related changes. But with my setup based on per-directory config, I think its most likely that this wouldn't have worked at all before.

In fact, I was looking at making a follow-up PR (adding runtimeExecutable: findNode(...) near here) to handle the debug launcher which suffers the same problem of not finding the node executable. I'm happy to contribute that as well, but I think you've got the gist of it here ;)

@mxschmitt
Copy link
Member

Looks reasonable, just to make sure. Was this a regression for you? Since this method is new and was implemented as a fallback, probably not?

I created #407 so its always based on the workspace folder of the current test project.

Nice, thank you! <3

I am a new Playwright user, so I don't know offhand if this is a regression with the recent related changes. But with my setup based on per-directory config, I think its most likely that this wouldn't have worked at all before.

In fact, I was looking at making a follow-up PR (adding runtimeExecutable: findNode(...) near here) to handle the debug launcher which suffers the same problem of not finding the node executable. I'm happy to contribute that as well, but I think you've got the gist of it here ;)

Thank you! Sounds like a great plan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants