apache / apache/apisix-dashboard

e2e: plugin_metadata.crud-all-fields is flaky on a half-rendered Monaco editor, masked by CI retries

Open Beginner friendly
#3,466 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
1.2k
Forks
617
Avg merge
4d 18h
Merged PRs (30d)
1

Description

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

Contributor guide

Open the contributing guide

Research direction

Start in e2e/tests/plugin_metadata.crud-all-fields.spec.ts at getMonacoEditorValue and the test around line 88; review the current textarea and view-line reads, then run the provided Playwright command with --repeat-each=6. Done means the helper waits for settled, parseable editor content and the CRUD test passes reliably without relying on CI retries.

Written by the indexing model from the issue text.

Assessment

Tech stack
playwright, typescript
Domain
testing
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
82/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.