Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Sep 8, 2025

Fix shlex splitting when launching parallel browser test runner on Windows.

Before this PR, running

set EMTEST_BROWSER=C:\Program Files\Mozilla Firefox\firefox.exe
test\runner browser

would fail with the parallel test harness attempting to launch browser command line

["C:\Program Files\Mozilla Firefox\firefox.exe -profile C:\emsdk\emscripten\main\out\profile-1"]

when it should instead have attempted to launch command line

["C:\Program Files\Mozilla Firefox\firefox.exe", "-profile", "C:\emsdk\emscripten\main\out\profile-1"]

Fix by having the browser config first do the shlex stuff to interpret EMTEST_BROWSER, and only then append the list of strings of command line args, rather than first append the command line args as a string spaghetti, and using shlex split to parse the resulting command line string.

@juj juj requested a review from brendandahl September 8, 2025 22:04

@staticmethod
def data_dir_cmdline(data_dir):
return ['-profile', data_dir]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does chrome not support ['--user-data-dir', xxx] or firefox not support -profile=foo? i.e. can we use the same method for both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Firefox at least does not support -profile=foo, but silently ignores it.

I don't have Chrome set up to test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm then, but lets lets @brendandahl approve since he wrote this code.

@juj juj enabled auto-merge (squash) September 8, 2025 23:12
@juj juj disabled auto-merge September 9, 2025 00:09
@juj juj merged commit a4d0dbc into emscripten-core:main Sep 9, 2025
22 of 31 checks passed
@sbc100
Copy link
Collaborator

sbc100 commented Sep 9, 2025

This change seems to have broken a bunch of the test_cubegeom tests under chrome

@sbc100
Copy link
Collaborator

sbc100 commented Sep 9, 2025

I can't see why though.. the command line before and after looks identical

@sbc100
Copy link
Collaborator

sbc100 commented Sep 9, 2025

Oh, its because --use-gl=swiftshader is not longer added to EMTEST_BROWSER, only to browser_cmd.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 9, 2025

I think maybe we should revert this to make the CI green again. It should be easy enough to re-land. Unless somehow has a fix ready?

sbc100 added a commit that referenced this pull request Sep 9, 2025
…er on Windows." (#25229)

This broken the chrome tests in CI because `--use-gl=swiftshader` is no
longer part of `EMTEST_BROWSER` so `no_swiftshader` doesn't work and log
of the cubegeom tests are therefore failing.

It should be easy enough to re-land, but we need to consider how to make
the effects of `EMTEST_BROWSER_AUTO_CONFIG` visible in places like
`no_swiftshader`.

Reverts #25222
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.

3 participants