-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
E2E Utils: Add retry mechanism to the REST API discovery #62331
E2E Utils: Add retry mechanism to the REST API discovery #62331
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! A few nitpicks but it's good to go IMO!
66092fd
to
5192d36
Compare
5192d36
to
60bf411
Compare
}, | ||
{ | ||
message: 'Failed to setup REST API.', | ||
timeout: 60_000, // 1 minute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do we have a rough number when the REST API will be available? A minute seems like a long wait if it returns nothing. Is 5 seconds or 10 seconds enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, and I have no idea TBH. Testing this thing takes ages, and it only fails in CI. Having said that, since this solution seems to be working across the board, I can lower the timeout value and do a couple more reruns. We can also address it in a follow-up PR since the failing perf tests keep blocking folks. What do you think?
This reverts commit efd2567.
reporter: process.env.CI | ||
? './config/performance-reporter.ts' | ||
: [ [ 'list' ], [ './config/performance-reporter.ts' ] ], | ||
reporter: [ [ 'list' ], [ './config/performance-reporter.ts' ] ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge it with this change as it now prints the errors correctly, which would allow us to close #60366. What do you think @Mamaduka @kevin940726?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. The performance test results are now attached to the summary so they won't be lost in the noise (#61450).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, enabling the list reporter in this case actually means that only the error will be printed, not the tests themselves. It's because we're still using promisified exec
and don't resolve with the stdout
data. So, nothing in the output will change unless an error is thrown! 😄
An example error looks like this now:
▶ Running tests
> Using: WordPress v6.5
> Suite: front-end-block-theme (round 1 of 1)
> Branch: 9aec5fe38aa66962d1b0dc3c73bc22c7af08e3c5
> Starting environment
> Running tests
> [email protected] test:performance
> wp-scripts test-playwright --config test/performance/playwright.config.ts front-end-block-theme
Error: expect(received).not.toThrow()
Matcher error: received value must be a function
Received has value: undefined
Call Log:
- Timeout 60000ms exceeded while waiting on the predicate
at ../../../packages/e2e-test-utils-playwright/src/request-utils/rest.ts:46
44 | let rootURL = '';
45 |
> 46 | await expect
| ^
[47](https://github.com/WordPress/gutenberg/actions/runs/9385403378/job/25843486775#step:8:48) | .poll(
48 | async () => {
49 | [ nonce, rootURL ] = await Promise.all( [
at RequestUtils.setupRest (/tmp/wp-performance-tests/tests/packages/e2e-test-utils-playwright/src/request-utils/rest.ts:46:2)
at globalSetup (/tmp/wp-performance-tests/tests/test/performance/config/global-setup.ts:26:2)
Error: Command failed: npm run test:performance -- front-end-block-theme
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:[51](https://github.com/WordPress/gutenberg/actions/runs/9385403378/job/25843486775#step:8:52)9: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 -- front-end-block-theme'
Before it was printing just the genericNodeError
part, and now all the necessary info is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinging @jsnajdr, since it also means we don't need to refactor to spawn
! 😄
Flaky tests detected in 2b68944. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9387734581
|
…2331) Co-authored-by: WunderBart <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: swissspidy <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: GraemeF <[email protected]>
Fixes #61627
Fixes #60366
Related thread: https://wordpress.slack.com/archives/C02QB2JS7/p1717143284055259
What?
Add a retry mechanism to the REST API discovery so that it doesn't fail when the env is not ready yet.
Testing Instructions
trunk
have been failing, so I've triggered a workflow using params from the failing job with this branch as a test runner. It should pass.