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.

Sign up for GitHub

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

Jump to bottom

E2E Utils: Add retry mechanism to the REST API discovery #62331

Merged
merged 11 commits into from
Jun 5, 2024
Merged

E2E Utils: Add retry mechanism to the REST API discovery #62331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
393c297
add retry mechanism to the REST API discovery
WunderBart Jun 5, 2024
111ae20
Update packages/e2e-test-utils-playwright/src/request-utils/rest.ts
WunderBart Jun 5, 2024
47d01ba
Extend the discovery timeout to 2 minutes
WunderBart Jun 5, 2024
efd2567
disable the webServer option
WunderBart Jun 5, 2024
7a24758
tmp: print errors
WunderBart Jun 5, 2024
702d7c1
Revert "Extend the discovery timeout to 2 minutes"
WunderBart Jun 5, 2024
4bee3ae
Revert "Update packages/e2e-test-utils-playwright/src/request-utils/r…
WunderBart Jun 5, 2024
6d2b1c3
Revert "add retry mechanism to the REST API discovery"
WunderBart Jun 5, 2024
60bf411
retry API discovery along with login
WunderBart Jun 5, 2024
94d0833
add comment
WunderBart Jun 5, 2024
2b68944
Revert "disable the webServer option"
WunderBart Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions packages/e2e-test-utils-playwright/src/request-utils/rest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import * as fs from 'fs/promises';
import { dirname } from 'path';
import { expect } from '@playwright/test';
import type { APIRequestContext } from '@playwright/test';

/**
Expand Down Expand Up @@ -39,10 +40,32 @@ async function getAPIRootURL( request: APIRequestContext ) {
}

async function setupRest( this: RequestUtils ): Promise< StorageState > {
const [ nonce, rootURL ] = await Promise.all( [
this.login(),
getAPIRootURL( this.request ),
] );
let nonce = '';
let rootURL = '';

// Poll until the REST API is discovered.
// See https://github.com/WordPress/gutenberg/issues/61627
await expect
WunderBart marked this conversation as resolved.
Show resolved Hide resolved
.poll(
async () => {
try {
[ nonce, rootURL ] = await Promise.all( [
this.login(),
getAPIRootURL( this.request ),
] );
} catch ( error ) {
// Prints the error if the timeout is reached.
return error;
}

return nonce && rootURL ? true : false;
},
{
message: 'Failed to setup REST API.',
timeout: 60_000, // 1 minute.
Copy link
Member

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?

Copy link
Member Author

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?

}
)
.toBe( true );

const { cookies } = await this.request.storageState();

Expand Down
4 changes: 1 addition & 3 deletions test/performance/playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ process.env.ASSETS_PATH = path.join( __dirname, 'assets' );

const config = defineConfig( {
...baseConfig,
reporter: process.env.CI
? './config/performance-reporter.ts'
: [ [ 'list' ], [ './config/performance-reporter.ts' ] ],
reporter: [ [ 'list' ], [ './config/performance-reporter.ts' ] ],
Copy link
Member Author

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?

Copy link
Member

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).

Copy link
Member Author

@WunderBart WunderBart Jun 5, 2024 •
edited
Loading

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

> gutenberg@18.5.0-rc.1 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.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know 😅

Copy link
Member Author

@WunderBart WunderBart Jun 5, 2024 •
edited
Loading

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! 😄

forbidOnly: !! process.env.CI,
fullyParallel: false,
retries: 0,
Expand Down
Loading