Skip to content

test(ui): add Vitest + RTL component test infrastructure#1249

Open
kovtcharov-amd wants to merge 2 commits into
mainfrom
feat/vitest-rtl-882
Open

test(ui): add Vitest + RTL component test infrastructure#1249
kovtcharov-amd wants to merge 2 commits into
mainfrom
feat/vitest-rtl-882

Conversation

@kovtcharov-amd
Copy link
Copy Markdown
Collaborator

The Agent UI frontend had zero component-level test infrastructure — no test runner, no DOM assertions, no API mocking layer. Contributors adding or modifying React components had no way to write automated tests. Now npm test runs 15 passing tests in under 5 seconds, and the full Vitest + React Testing Library + MSW stack is ready for future component coverage.

Test plan

  • cd src/gaia/apps/webui && npm install && npm test — 15 tests pass
  • npm run test:watch — watch mode starts correctly
  • npm run build — production build still works (no test deps leak into bundle)

Closes #882

@github-actions
Copy link
Copy Markdown
Contributor

This PR delivers exactly what the Agent UI needed: a real test stack with 15 well-structured tests covering utilities, rendering, and user interaction — and npm test finishes in under 5 seconds. Two items need attention before this is fully wired up.


Issues

🟡 No CI step — tests won't run on PRs

The new npm test script exists in package.json but no workflow runs it. test_electron.yml only executes cd tests/electron && npm test (Jest-based Electron tests), so the 15 Vitest tests are silently excluded from CI. Contributors could break them without any gate catching it.

Add a job or step to test_electron.yml (or a new test_webui.yml):

      - name: Run Agent UI unit tests (Vitest)
        working-directory: src/gaia/apps/webui
        run: npm ci && npm test

Or, inline into the existing test_electron.yml path-filter block so it only runs when src/gaia/apps/webui/** changes.


🟡 Version jump in lockfile looks unintended

package-lock.json changes from 0.17.60.19.0, skipping 0.18.x entirely. The lockfile version is driven by package.json (already at 0.19.0 on this branch). A pure test-infrastructure PR shouldn't carry a two-minor-version bump. Confirm the package.json version bump was intentional and belongs here, or split it into the release/version PR where it fits.


🟢 globals: true and explicit imports are redundant — document the convention

vitest.config.ts sets globals: true, which injects describe, it, expect, vi at runtime. UnsupportedFeature.test.tsx also imports them explicitly (import { describe, it, expect, vi } from 'vitest'). Both work — the explicit imports win. But the combination signals two different conventions, and future test authors may copy either one.

Recommend keeping the explicit imports (they satisfy TypeScript without a tsconfig "types" addition) and either removing globals: true or adding a comment in the config explaining why it's kept:

    test: {
        environment: 'jsdom',
        globals: false,   // use explicit imports; avoids tsconfig "types" requirement
        setupFiles: ['./src/test/setup.ts'],
        include: ['src/**/*.{test,spec}.{ts,tsx}'],
        css: true,
    },

Strengths

  • Test shape is exactly right: utilities → render → interaction, three discrete suites, no over-engineering. The getUnsupportedCategory / isExtensionSupported unit tests give pure functions a fast feedback loop independent of DOM.
  • timeout={0} on toasts: deterministic teardown without fake timers or waitFor hacks — the right call.
  • __APP_VERSION__: "0.0.0-test" define: prevents the build-time constant from being undefined in test runs; catches the most common "green locally, red in jsdom" failure mode.
  • Zero production-bundle impact: all new packages are devDependencies; MSW is included but not hooked up yet — good forward-planning without scope creep.

Verdict

Request changes — both 🟡 items are straightforward fixes. The CI gap is the more important one: tests that don't gate PRs provide weak guarantees. The version bump should be confirmed or moved. The globals nit is optional.

@github-actions github-actions Bot added the devops DevOps/infrastructure changes label May 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Solid infrastructure addition — test runner, DOM assertions, and future API-mocking layer are all wired up correctly. Fifteen tests pass, CI is integrated into the summary gate, and the existing build is unaffected. Nothing blocking; a couple of small observations below.

Issues Found

🟢 msw brings a deep dep that mismatches the CI Node version (package-lock.json, package.json)

msw → @inquirer/core → mute-stream@4.0.0 declares engines: "^22.22.2 || ^24.15.0 || >=26.0.0", but the new CI job targets Node 20. npm doesn't fail on engine mismatches by default (no --engine-strict), so npm ci succeeds, but CI will print a noisy engine-compat warning on every run. Since no test file actually imports msw yet, the simplest fix is to defer adding it until the first MSW-backed test is ready. If you want to keep it for the "stack is ready" signal, add --ignore-engines to the npm ci step or bump the job's node to '22'.

      - name: Setup Node.js
        uses: actions/setup-node@v6
        with:
          node-version: '22'
          cache: 'npm'
          cache-dependency-path: src/gaia/apps/webui/package-lock.json

🟢 ErrorReportBanner has no tests (UnsupportedFeature.test.tsx)

getUnsupportedCategory, isExtensionSupported, UnsupportedFeatureBanner, and UploadErrorToast are all covered. ErrorReportBanner (the fourth exported component, used for caught errors in chat messages) is not. A single smoke-test would round out the suite:

describe('ErrorReportBanner', () => {
    it('renders error message and GitHub bug-report link', () => {
        render(<ErrorReportBanner errorMessage="Something broke" />);
        expect(screen.getByText('Something went wrong')).toBeInTheDocument();
        expect(screen.getByRole('link', { name: /report it on github/i })).toBeInTheDocument();
    });
});

Strengths

  • globals: false + an explicit afterEach(() => cleanup()) in the setup file is the right way to handle RTL teardown when Vitest global injection is turned off — no leaking renders between tests.
  • __APP_VERSION__ defined in vitest.config.ts exactly matches how the production build sets it, so getEnvironmentInfo() inside the component won't throw a ReferenceError under test.
  • timeout={0} on all UploadErrorToast renders is a clean way to disable the auto-dismiss setTimeout without needing fake timers, keeping tests fast and deterministic.
  • CI wiring is correctly gated: the test-webui-vitest job is in the needs array for the summary job, and the failure check in the summary shell script is consistent with the pattern used by the other jobs.

Verdict

Approve. No blocking issues. The ErrorReportBanner gap is a nit for a follow-up; the mute-stream engine warning is real but benign — and the suggestion above resolves both. Merging now is fine.

@kovtcharov-amd
Copy link
Copy Markdown
Collaborator Author

The 6 Electron test failures (, , , etc.) are pre-existing broken assertions from the #606 UX rework — not caused by this PR's Vitest changes. They trigger because this PR touches \ which is in the Electron workflow's path filter. PR #1255 fixes the remaining stale Electron assertions — once it merges and this PR rebases, the failures will resolve.

@kovtcharov-amd kovtcharov-amd added the p2 low priority label May 29, 2026
@kovtcharov-amd kovtcharov-amd self-assigned this May 29, 2026
itomek
itomek previously approved these changes May 29, 2026
Copy link
Copy Markdown
Collaborator

@itomek itomek left a comment

Choose a reason for hiding this comment

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

The stack is wired up correctly: test-webui-vitest runs npm ci && npm test and feeds the Test Summary gate, vitest.config.ts uses globals: false with an explicit afterEach(cleanup) in setup.ts, and APP_VERSION is defined so getEnvironmentInfo() won't throw under jsdom. The 15 tests assert real rendered output and user interaction against the actual exported component API — not tautologies. Verified the imported symbols (getUnsupportedCategory, isExtensionSupported, UnsupportedFeatureBanner, UploadErrorToast) all exist and are exported. Two non-blocking items inline (the 0.17.6->0.19.0 lockfile bump riding along; ErrorReportBanner untested). Approving.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

package.json/package-lock.json carry a 0.17.6 -> 0.19.0 version bump (skipping 0.18.x) in a test-infra PR. Confirm it's intentional / rebased from a release bump, or split it out, so the lockfile churn isn't mis-attributed to this PR. Non-blocking.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ErrorReportBanner is a fourth exported component with no coverage. A one-line smoke test (renders the message + the "Report it on GitHub" link) would round out the suite while the stack is fresh. Optional.


Generated by Claude Code

Ovtcharov added 2 commits May 29, 2026 09:33
The Agent UI had zero component test infrastructure — no test runner, no
DOM assertions, no API mocking. This adds the full foundation so future
component tests can be written:

- vitest + jsdom + @testing-library/react + user-event + jest-dom + msw
- vitest.config.ts with jsdom environment and CSS support
- src/test/setup.ts for jest-dom matcher registration
- 15 passing tests against UnsupportedFeature (utility + render + interaction)
- Add test-webui-vitest job to test_electron.yml so Vitest tests gate PRs
- Switch globals: true → globals: false (explicit imports are already used)
- Register afterEach cleanup in setup.ts (required when globals are off)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops DevOps/infrastructure changes p2 low priority tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent UI: Vitest + RTL + MSW component tests inside src/gaia/apps/webui/

2 participants