Replace exec with spawn in performance runner script #62270
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +20 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
jsnajdr
reviewed
Jun 4, 2024
bin/plugin/lib/utils.js
Outdated
| HOME: process.env.HOME, | ||
| USER: process.env.USER, | ||
| ...env, | ||
| }, |
Member
There was a problem hiding this comment.
Another thing to try is using spawn instead of exec. The difference is that you can pass the stdio: 'inherit' option to spawn`.
exec is running the command inside a shell, but we don't need that for npm run. There are other usages of runShellScript that run cp -R, and cp is a shell built-in -- for these whe have the shell: true option for spawn.
Member
Author
There was a problem hiding this comment.
Agreed! I was also considering it some time ago when we had similar logging issues, as it allows for better debugging since it streams the output, which is often quite large. Never got to trying it out, though. Anyhow, if this quick-fix doesn't help, we can try refactoring to spawn.
Member
Author
There was a problem hiding this comment.
It still prints the same error with spawn. stdio is set to inherit and it still doesn't print the tests output. What am I doing wrong? 😅
Member
There was a problem hiding this comment.
One more idea to try: run directly the test-playwright.js script. This works locally:
node ./packages/scripts/scripts/test-playwright.js --config test/performance/playwright.config.ts site-editor
Currently there are so many levels of indirection. The GitHub action spawns a node bin/plugin/cli.js process, which spawns a npm run process, which spawns a wp-scripts process, which spawns a test-playwright process, which finally spawns the playwright CLI process. I counted five processes 🙂 Somewhere along the way the errors get lost.
Member
Author
There was a problem hiding this comment.
It's still the same, unfortunately. 😩 I'll keep digging, but if you have any other ideas, please share! 😄
WunderBart
force-pushed
the
fix/exec-util-promise
branch
4 times, most recently
from
9562fca to
8250b93
Compare
Member
|
I'm getting some progress after I enabled the Here are the errors that make the job fail in this branch. I didn't even need the artificial "oopsie" error: There's a strange 500 Internal Server Error on the |
jsnajdr
force-pushed
the
fix/exec-util-promise
branch
2 times, most recently
from
6c8993c to
90019c4
Compare
WunderBart
changed the title
runShellScript util
WunderBart
force-pushed
the
fix/exec-util-promise
branch
from
b6ee3d2 to
2c7e3f3
Compare
Member
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related slack convo: link
What?
Performance tests runner seems to be swallowing the tests errors, printing a cryptic exception:
Error: Command failed: npm run test:performance -- site-editor at genericNodeError (node:internal/errors:984:15) at wrappedFn (node:internal/errors:538:14) at ChildProcess.exithandler (node:child_process:422:12) at ChildProcess.emit (node:events:519:28) at maybeClose (node:internal/child_process:1105:16) at ChildProcess._handle.onexit (node:internal/child_process:305:5) { code: 1, killed: false, signal: null, cmd: 'npm run test:performance -- site-editor' }Let's see if passing the outputs the recommended way makes the errors appear as expected.