opensource-joe opened a new issue, #3466:
URL: https://github.com/apache/apisix-dashboard/issues/3466
### Issue description
`e2e/tests/plugin_metadata.crud-all-fields.spec.ts:88` ("should CRUD plugin
metadata with all fields") fails locally somewhere between a third and two
thirds of the time on current `master`. It is a test-suite race, not a product
bug, and it is invisible in CI because `playwright.config.ts` sets `retries:
process.env.CI ? 2 : 0`.
The failure is always in the final step, `verify configuration changes were
saved`:
```
Expected pattern: /"time"\s*:\s*"\$time_iso8601"/
Received string: "{"
205 | // Get Monaco editor value using helper
206 | const editorValue = await
getMonacoEditorValue(editPluginDialog);
> 207 | expect(editorValue).toMatch(/"time"\s*:\s*"\$time_iso8601"/);
```
The received value is a single `{`, so the assertion is running against a
Monaco editor that has mounted but not yet painted its content.
`getMonacoEditorValue` (defined at the top of the same spec, line 36)
already anticipates this and has two escape hatches, but both can observe the
same half-rendered instant:
```ts
await textarea.waitFor({ state: 'attached', timeout: 10000 });
// ...
if (await textarea.count() > 0) {
editorValue = await textarea.inputValue();
}
// Fallback to reading view-lines if textarea value is incomplete
if (!editorValue || editorValue.trim() === '{') {
await editPluginDialog.locator('.view-line').first().waitFor({ timeout:
10000 });
const lines = await
editPluginDialog.locator('.view-line').allTextContents();
editorValue = lines.join('\n').replace(/\s+/g, ' ');
}
```
`waitFor({ state: 'attached' })` resolves as soon as the textarea exists,
which is before Monaco has a model. The `.view-line` fallback waits only for
the *first* line to exist, and the first line of the JSON is `{`, so on a slow
paint the fallback is satisfied by exactly the state it was written to escape.
Each check is a single observation, so there is no retry once both have been
taken.
### Expected behavior
The step should read the editor's settled content and assert against it,
passing deterministically on a correctly saved config.
### How to Reproduce
```
cd e2e/server && docker compose up -d --build
pnpm dev
E2E_TARGET_URL=http://localhost:5173/ui/ npx playwright test --workers=1
--repeat-each=6 \
e2e/tests/plugin_metadata.crud-all-fields.spec.ts
```
`--repeat-each` is the important part: a single run passes often enough to
look fine. Measured on `master` at `045e3142`, and on a branch off it, running
against a real APISIX from `e2e/server`:
| | failed | passed |
|---|---|---|
| `master`, 6 runs | 4 | 2 |
| a branch off it, 9 runs | 4 | 5 |
Both sides are the same test-suite race; the spread is sample noise, not a
difference between the two trees.
### Environment
- apisix-dashboard version: `master` @ `045e3142`
- Browser: Chromium (Playwright `chromium-headless-shell` 145.0.7632.6,
`@playwright/test` 1.58.2)
- Gateway: `e2e/server` compose (`apache/apisix:dev` +
`bitnamilegacy/etcd:3.5`)
- OS: Linux container on macOS arm64
### Additional context
Found while verifying #3465, where it showed up as a single failure in a
scoped e2e run. Worth saying explicitly that my first reading of it was wrong:
one run per side said "passes on master, fails on the branch", which looked
like a regression from that PR. Repeating it produced the table above and
showed it is pre-existing. Flagging that because anyone who hits this once
while reviewing a PR will reach the same wrong conclusion.
I would suggest replacing the single-observation reads with a poll until the
content is actually parseable, so the helper waits for a *settled* editor
rather than a populated one:
```ts
await expect
.poll(async () => {
const raw = await textarea.inputValue();
try {
return Object.keys(JSON.parse(raw)).length;
} catch {
return 0;
}
}, { timeout: 10_000 })
.toBeGreaterThan(0);
```
That makes the success condition "the editor holds valid JSON with content"
instead of "the editor holds something", which is the property the assertions
afterwards actually depend on, and it removes the need for the `.view-line`
fallback entirely.
This relates to the test-suite gaps already listed in #3417 (15
`waitForTimeout` sites, parallel-unsafe locally). This one is arguably worse
than those, because CI retries hide it completely, so the suite reads as green
while the race is real.
Happy to send a PR for the helper change if that is useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
