Repository navigation
test: replace low-signal tests with behavior integrations - #51
Conversation
|
Review complete. 🟠 1 high · 🟡 1 medium 💬 Inline comments (1)
📍 Findings outside the diff (1) — 🟡 1 medium — defects on lines GitHub can't attach comments to🟡 Medium — // packages/command-palette/src/multi-step/machine.ts
183 case 'SKIP_STEP': {
184 if (
185 !state.config ||
186 state.view.flowStatus !== 'active' ||
187 !state.config.steps[state.view.currentStepIndex]?.skippable
188 ) return { state };
189 const view = multiStepReducer(state.view, { type: 'SKIP_STEP' });
190 const next = { ...state, view };
191 const withCompletion = withCompletionEffect(state, next, options);
192 const withLoader = withLoaderEffect(withCompletion.state);
193 return mergeResults(withCompletion, withLoader);
When the orphaned loader later resolves, This PR overhauls test coverage and several production paths across the monorepo: it replaces the Playwright billing e2e journey with an artifact-recording version, refactors command-palette machine guards (
Reviewed commit: 3e5be34 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR rewrites the billing e2e journey with artifact capture, refactors the command-palette multi-step machine's loader/back/skip transitions, tightens sheets grid editor helpers, and updates workspace-kit/billing UI primitives and tests.
Key findings
- 🟠 Billing artifacts captured only on success path — docs.interaction.spec.ts:45
- 🟡
SKIP_STEPleaves loader in flight; stale result lands on next step — machine.ts:183
| async function recordBillingJourney(page: Page, errors: string[]) { | ||
| const artifactDirectory = process.env.BLOCKS_E2E_ARTIFACT_DIR | ||
| ?? join(tmpdir(), 'constructive-blocks-billing-e2e'); | ||
| await mkdir(artifactDirectory, { recursive: true }); | ||
| const screenshot = join(artifactDirectory, `${test.info().title.replace(/[^a-z0-9]+/gi, '-')}.png`); | ||
| await page.screenshot({ path: screenshot, fullPage: true }); | ||
| test.info().annotations.push({ type: 'artifact', description: screenshot }); | ||
| await test.info().attach('billing-journey', { path: screenshot, contentType: 'image/png' }); | ||
| expect(errors).toEqual([]); | ||
| } |
There was a problem hiding this comment.
🟠 bug · high
Billing artifacts captured only on success path
Every billing test calls recordBillingJourney(page, errors) as its final statement (e.g. apps/blocks/e2e/docs.interaction.spec.ts:418), so when any earlier step fails the full-page screenshot, artifact annotation, billing-journey attachment, and the expect(errors).toEqual([]) check are all skipped (apps/blocks/e2e/docs.interaction.spec.ts:45).
With screenshot: 'off', trace: 'off', and video: 'off' in playwright.config.ts, a failed billing journey produces no diagnostic evidence at all, and page errors are never surfaced because the only assertion on the errors array lives inside the skipped helper.
📋 Prompt for AI Agents
In apps/blocks/e2e/docs.interaction.spec.ts, wrap each billing test's body in try { ... } finally { await recordBillingJourney(page, errors); } so the screenshot, artifact annotation, billing-journey attachment, and expect(errors).toEqual([]) all run even when an earlier step fails. The helper is at lines 45-54; the final calls are around lines 418, 453, 480, 535, 567, 594, 620. With playwright.config.ts setting screenshot/trace/video to 'off', this helper is the sole artifact source, and skipping it on failure loses the screenshot, the attachment, and the pageerror report for exactly the runs that need them.
1 participant
AGENTS.md. No failing behavior assertion is removed or weakened, and no coverage probe, threshold or exclusion change is used.7e9f277)