Skip to content

fix(test): replace fixed-delay waits with polling in MainConfig walkthrough tests#126

Merged
dawsontoth merged 1 commit into
mainfrom
fix/flaky-main-config-test
Jul 2, 2026
Merged

fix(test): replace fixed-delay waits with polling in MainConfig walkthrough tests#126
dawsontoth merged 1 commit into
mainfrom
fix/flaky-main-config-test

Conversation

@dawsontoth

Copy link
Copy Markdown
Contributor

Summary

Fixes the flaky ink/main.test.tsx > MainConfig > completes the walkthrough for OpenAI and updates .env test that failed CI on #125 (Node 24, ubuntu-latest).

  • Root cause: each walkthrough test (OpenAI, Anthropic, Google, Ollama) waited a fixed 100ms after every stdin write, assuming Ink's re-render always completes in that window. Under CI load this isn't guaranteed, so the next assertion could read a stale frame.
  • Replaced the sleep with vi.waitFor polling for the expected frame content — deterministic and no slower than necessary.
  • The API key field renders as a masked password input (* per character), so waiting for the next step's text after typing the key doesn't confirm the keystrokes actually landed in component state before Enter is sent. Added an explicit wait for the masked value's length before submitting.

Test plan

  • npx vitest run ink/main.test.tsx — all 6 tests pass
  • Ran the file 15x in a loop locally to check for flakiness — 15/15 passed
  • Full suite (npx vitest run) — 345/345 pass
  • npm run lint and npm run format — clean
  • Total suite runtime for this file dropped from ~5s to ~1.3s (no more blind sleeps)

🤖 Generated with Claude Code

…hrough tests

The walkthrough tests waited a fixed 100ms after each stdin write, assuming
Ink's re-render always completed in that window. Under CI load (observed on
Node 24 ubuntu-latest) this could flake before a re-render caught up.

Replace the sleep with vi.waitFor polling for the expected frame content,
and add an explicit wait for the masked password field before submitting
the API key so keystrokes are guaranteed to have reached component state.
@dawsontoth dawsontoth merged commit 1c67a48 into main Jul 2, 2026
4 checks passed
@dawsontoth dawsontoth deleted the fix/flaky-main-config-test branch July 2, 2026 14:38

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the CLI integration tests in ink/main.test.tsx to eliminate flakiness under CI load. It replaces fixed-duration timeouts with asynchronous polling helpers (waitForFrame and waitForMaskedInput) that wait for specific frames to render before sending keystrokes. The review feedback correctly identifies a potential robustness issue where lastFrame() might return undefined during initial renders, which would cause a TypeError in the assertions. The suggested improvement to use nullish coalescing (?? '') is highly recommended to ensure clean assertion failures.

Comment thread ink/main.test.tsx
Comment on lines +36 to +37
const waitForFrame = (lastFrame: () => string | undefined, text: string) =>
vi.waitFor(() => expect(lastFrame()).toContain(text));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If lastFrame() returns undefined (which can happen during initial render or before the first frame is drawn), calling expect(undefined).toContain(text) can throw a TypeError in some test runners because undefined is not iterable/a string. Using the nullish coalescing operator ?? '' ensures that we always perform a string assertion, which provides a much cleaner assertion failure message if the test times out, rather than a generic TypeError.

Suggested change
const waitForFrame = (lastFrame: () => string | undefined, text: string) =>
vi.waitFor(() => expect(lastFrame()).toContain(text));
const waitForFrame = (lastFrame: () => string | undefined, text: string) =>
vi.waitFor(() => expect(lastFrame() ?? '').toContain(text));

Comment thread ink/main.test.tsx
Comment on lines +42 to +43
const waitForMaskedInput = (lastFrame: () => string | undefined, text: string) =>
vi.waitFor(() => expect(lastFrame()).toContain('*'.repeat(text.length)));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Similarly to waitForFrame, if lastFrame() returns undefined, calling expect(undefined).toContain(...) can throw a TypeError. Using ?? '' ensures a safe string assertion and cleaner error messages upon timeout.

Suggested change
const waitForMaskedInput = (lastFrame: () => string | undefined, text: string) =>
vi.waitFor(() => expect(lastFrame()).toContain('*'.repeat(text.length)));
const waitForMaskedInput = (lastFrame: () => string | undefined, text: string) =>
vi.waitFor(() => expect(lastFrame() ?? '').toContain('*'.repeat(text.length)));

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 0.16.24 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant