test(dashboard): cover SSE message processors and add first component test - #248
Draft
walidozich wants to merge 6 commits into
Draft
test(dashboard): cover SSE message processors and add first component test#248walidozich wants to merge 6 commits into
walidozich wants to merge 6 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Draft: stacked on #243. This branch is based on #243, which is not merged yet, so GitHub shows its commits here too. The only new work in this PR is the final commit,
test(dashboard): cover SSE build, log, and generic message processors(one new file, +946).I will rebase onto
mainand mark this ready once #243 lands. Opening it now so the coverage is visible and reviewable rather than sitting on my machine.New commits in this PR:
test(dashboard): cover SSE build, log, and generic message processorsandtest(dashboard): add first component test covering Button.Summary
Two things:
apps/dashboard/src/lib/sseMessageProcessors.ts, the 419-line module that turns raw SSE stream payloads into the callbacks driving every live build and log view in the dashboard. It previously had no test coverage.components/ui/Button), which is the first committed test to exercise the jsdom project added in test(dashboard): add Vitest infrastructure and first test suites #243.Motivation
This module sits directly under the real-time deploy UI. If
parseMessagemisclassifies a payload orhandleMessagefails to fire a callback, a build silently stops updating, or a failure never surfaces to the user. None of that was protected by a test.It is also a good testing target: the three factories are callback-driven and pure, so they need no DOM, no network, and no timers.
Related issue
Refs #216. Tests do not require prior issue agreement per CONTRIBUTING.md.
Changes
apps/dashboard/src/lib/sseMessageProcessors.test.ts(new, 83 tests)createBuildMessageProcessor(~57 tests): every recognizedtypebranch inparseMessage, plusnull/undefined/empty/malformed input; all 11 optional callbacks firing with correct arguments; that omitting callbacks never throws; theonPromptandonServiceStatusdefault fallbacks (title || "Action Required",status ?? "pending"); and thehandleMessageboolean return contract across every recognized type.createLogMessageProcessor(~19 tests): thelog,connected,end, anderrorbranches, the base64 versus plain-text log fallback, and malformed payloads.createGenericMessageProcessor(7 tests): complete coverage of its small surface.apps/dashboard/src/components/ui/button.test.tsx(new, 10 tests)Covers click dispatch, the disabled state gating interaction,
asChildslot polymorphism, ref forwarding, arbitrary prop pass-through, andclassNamemerging.It deliberately does not assert exact Tailwind class strings, since those break on any cosmetic change while protecting nothing. Variants are compared against each other (destructive must differ from ghost) rather than against literals, and queries go through accessible roles. It is written to be the copy-paste reference for future component tests, and the file says so.
Verification
$ bun run --cwd apps/dashboard test ✓ unit src/lib/sseMessageProcessors.test.ts (83 tests) ✓ dom src/components/ui/button.test.tsx (10 tests) Test Files 10 passed (10) Tests 209 passed (209)Note the
domtag on the component test anduniton the rest: that is the environment split from #243 doing its job, with only the.tsxfile paying for jsdom.Proving the tests can fail. Neutering the
onFailuredispatch at line 254:Exactly the two guarding tests fail, with no collateral cascade.
The component test was mutation-checked twice as well. Forcing
asChildto always render abuttonfails exactly the 2 slot tests; droppingref={ref}fails exactly the 1 ref test. Sources restored withgit checkout --and confirmed clean viagit diff. Typecheck and Prettier both pass.Notes for reviewers: two pre-existing findings
Not fixed here, since behavior changes need an agreed issue first. The tests characterize current behavior. Happy to open an issue for either.
1. Spread-after-literal makes the fallback labels unreachable. All three
parseMessagefunctions are written as:The spread comes after the literal, so any
typealready on the payload overrides the intended label:Since SSE payloads almost always carry a
type, the"unknown"label in the build and log processors, and the"message"default in the generic one, are effectively unreachable. It fails silently as misclassification rather than a crash, which is presumably why it went unnoticed. The fix is a one-line reorder, but it is a behavior change so I have left it alone.2.
handleMessagenever returnsfalse. TheSSEMessageProcessorcontract inhooks/useSSEStream.tsdocumentsfalseas "stop processing", but every branch in the build and log processors falls through to an unconditionalreturn true. That may well be deliberate today; flagging it in case a future case (a hard container failure, say) is expected to halt stream consumption.Checklist
bun run test,bun run --cwd <workspace> lint, andbun formatall pass locally