test: expand UI test coverage for high-logic components#425
Conversation
Add new test files and expand existing ones: - NodeStateIndicator: all NodeState variants, labels, indicator rendering - TemplateSelector: filtering, search, selection, empty states, a11y - JsonStreamDisplay: stream parsing, packet types, error handling - useContextMenu: additional edge cases (no-op click, alternating menus) - converter service: MP4 streaming, blob strategy, multi-upload, error edge cases - dag utilities: wouldCreateCycle, topoLevels, orderedNames, layout edge cases Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| const dot = document.querySelector('div[class]'); | ||
| expect(dot).toBeInTheDocument(); |
There was a problem hiding this comment.
🟡 Indicator-dot test selects the wrapper instead of the dot
The selector document.querySelector('div[class]') is too broad: NodeStateIndicator always renders an outer <div className="nodrag"> around the indicator (ui/src/components/NodeStateIndicator.tsx:449-456), so this assertion passes by finding that wrapper even if the actual StateIndicator dot is removed or not rendered. This makes the new “renders an indicator dot” coverage ineffective for the behavior it claims to protect.
Prompt for agents
Tighten the NodeStateIndicator dot test so it identifies the actual indicator element rather than any div with a class. The current test in ui/src/components/NodeStateIndicator.test.tsx can pass by matching the outer nodrag wrapper from NodeStateIndicator.tsx. Consider adding an accessible/test-specific marker to the indicator in ui/src/components/NodeStateIndicator.tsx or querying a more precise, intentional element, then assert that one element is present for each state.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in 4967617. The test now traverses the DOM precisely (.nodrag → IndicatorWrapper → StateIndicator) and asserts the dot's 10×10px dimensions instead of matching any div[class].
| renderIndicator('Running', { stats }); | ||
| // Error badge is only visible when tooltip is open (isTooltipOpen), so it should not be present | ||
| }); |
There was a problem hiding this comment.
🟡 Error-badge test has no assertion
This test renders a Running indicator with errored > 0 stats but never asserts that the error badge is absent, so it will pass even if the component starts rendering the badge while the tooltip is closed. The only statement after renderIndicator is a comment, leaving the regression scenario untested.
Prompt for agents
Add a real assertion to the error-badge test in ui/src/components/NodeStateIndicator.test.tsx. The test should render stats with errored > 0 and then assert that the error badge is not present while the tooltip is closed. If the badge has no reliable selector today, add an intentional test/accessibility marker to the badge in NodeStateIndicator.tsx and use that in the test.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in 4967617. The test now asserts that the IndicatorWrapper has exactly 1 child (the StateIndicator dot only, no ErrorBadge) when the tooltip is closed.
| it('warns and skips if stream is already locked', async () => { | ||
| const stream = makeStream(['{"ok":true}\n']); | ||
| stream.getReader(); | ||
|
|
||
| render(<JsonStreamDisplay stream={stream} />); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.queryByText('ok')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
📝 Info: Locked-stream test intentionally exercises the component’s early return path
The locked-stream case calls stream.getReader() and never releases it before rendering. I checked JsonStreamDisplay.tsx, where the effect explicitly checks stream.locked, logs a warning, sets loading false, and returns before attempting pipeThrough (ui/src/components/converter/JsonStreamDisplay.tsx:273-281). That means the test is not accidentally leaking a stream reader into the component path; it is targeting the guard branch. The reader still remains locked for the lifetime of the test, but the setup cleanup unmounts the component after each test and this stream is local to the test.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| describe('convertFile - MSE Streaming (MP4)', () => { | ||
| it('should handle MP4 streaming when MSE supports the type', async () => { | ||
| (globalThis as { MediaSource?: unknown }).MediaSource = { | ||
| isTypeSupported: vi.fn().mockReturnValue(true), | ||
| } as never; | ||
|
|
||
| const mockBody = new ReadableStream({ | ||
| start(controller) { | ||
| controller.enqueue(new Uint8Array([0x00, 0x00, 0x00, 0x1c])); | ||
| controller.close(); | ||
| }, | ||
| }); | ||
|
|
||
| (fetch as ReturnType<typeof vi.fn>).mockResolvedValue({ | ||
| ok: true, | ||
| headers: new Headers({ 'Content-Type': 'video/mp4' }), | ||
| body: mockBody, | ||
| } as Response); | ||
|
|
||
| const result = await convertFile(MOCK_YAML, MOCK_UPLOAD, 'playback'); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.useStreaming).toBe(true); | ||
| expect(result.contentType).toBe('video/mp4'); | ||
| }); | ||
|
|
||
| it('should fall back to blob for MP4 when MSE is unavailable', async () => { | ||
| delete (globalThis as { MediaSource?: unknown }).MediaSource; | ||
|
|
||
| const mockBlob = new Blob(['video data'], { type: 'video/mp4' }); | ||
| global.URL.createObjectURL = vi.fn().mockReturnValue('blob:mp4-url'); | ||
|
|
||
| (fetch as ReturnType<typeof vi.fn>).mockResolvedValue({ | ||
| ok: true, | ||
| headers: new Headers({ 'Content-Type': 'video/mp4' }), | ||
| body: new ReadableStream(), | ||
| blob: vi.fn().mockResolvedValue(mockBlob), | ||
| } as never); | ||
|
|
||
| const result = await convertFile(MOCK_YAML, MOCK_UPLOAD, 'playback'); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.useStreaming).toBe(false); | ||
| expect(result.mediaUrl).toBe('blob:mp4-url'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('convertFile - WebM blob strategy', () => { | ||
| it('should force blob playback when webmPlayback is "blob"', async () => { | ||
| (globalThis as { MediaSource?: unknown }).MediaSource = { | ||
| isTypeSupported: vi.fn().mockReturnValue(true), | ||
| } as never; | ||
|
|
||
| const mockBlob = new Blob(['webm data'], { type: 'audio/webm' }); | ||
| global.URL.createObjectURL = vi.fn().mockReturnValue('blob:webm-blob-url'); | ||
|
|
||
| (fetch as ReturnType<typeof vi.fn>).mockResolvedValue({ | ||
| ok: true, | ||
| headers: new Headers({ 'Content-Type': 'audio/webm' }), | ||
| body: new ReadableStream(), | ||
| blob: vi.fn().mockResolvedValue(mockBlob), | ||
| } as never); | ||
|
|
||
| const result = await convertFile(MOCK_YAML, MOCK_UPLOAD, 'playback', undefined, { | ||
| webmPlayback: 'blob', | ||
| }); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.useStreaming).toBe(false); | ||
| }); |
There was a problem hiding this comment.
📝 Info: Converter test global MediaSource mutations are restored by existing teardown
The new MP4/WebM streaming tests mutate globalThis.MediaSource, including deleting it for fallback coverage. I verified converter.test.ts stores the original value in beforeEach and restores it in afterEach (ui/src/services/converter.test.ts:43-56), so these additions should not leak MediaSource state into later tests in this file.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| it('renders empty state when no templates match filters', () => { | ||
| render(<TemplateSelector {...defaultProps} templates={[]} />); | ||
| expect(screen.getByText('No pipelines match your filters.')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🚩 TemplateSelector empty-state wording is shared for no templates and filtered-out templates
The new test asserts No pipelines match your filters. even when templates={[]}. The component uses the same empty state whenever both filtered system and user template lists are empty (ui/src/components/converter/TemplateSelector.tsx:327-329), so this is consistent with current behavior. It may be a product-copy question whether an unconfigured/empty template catalog should say something different from an active filter miss, but it is not a functional bug in this PR’s test coverage.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…ssertion - Tighten indicator-dot test to traverse the DOM structure precisely (nodrag wrapper > IndicatorWrapper > StateIndicator) and verify 10x10px dimensions instead of matching any div with a class attribute - Add real assertion to error-badge test: verify the IndicatorWrapper has exactly 1 child (the dot only, no ErrorBadge) when tooltip is closed - Fix import ordering (radix-ui before testing-library) to satisfy eslint import/order rule in CI Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
- Add data-testid to StateIndicator and ErrorBadge, query by testid
instead of DOM traversal + getComputedStyle
- Scope TemplateSelector count assertions using within() instead of
fragile getAllByText('1')
- Replace manual globalThis.MediaSource mutations with vi.stubGlobal
and vi.unstubAllGlobals in afterEach
- Assert componentsLogger.warn in locked-stream test to verify full
warn-and-skip contract
- Filter createElement mock by tag name to avoid intercepting all calls
- Use 'as unknown as Pipeline' instead of dishonest 'as never' cast
- Fold SimpleEdge into main dag import
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
The import/order rule in eslint-plugin-import does not support the
'import { type Foo }' syntax and throws TypeError. Keep the type
import as a separate 'import type' statement.
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Expand UI test coverage from 41 to 44 test files (~821 lines added), targeting the 6 highest-logic components identified in the issue.
New test files:
NodeStateIndicator.test.tsx— state variants, indicator dot viadata-testid, error badge, stop reasonsTemplateSelector.test.tsx— rendering, filtering, search, selection, empty statesJsonStreamDisplay.test.tsx— stream parsing, packet types, error handling, locked-stream contractExpanded test files:
converter.test.ts— MSE streaming (MP4/WebM), blob strategy, multi-upload, download, error handlingdag.test.ts— cycle detection, topo levels, pipeline layout, edge casesuseContextMenu.test.ts— open/close, positioning, click-outside, scroll handlingCode quality fixes (review feedback):
data-testid="state-dot"anddata-testid="error-badge"instead of DOM traversal +getComputedStylewithin()instead of fragilegetAllByText('1')globalThis.MediaSourcemutations withvi.stubGlobal+vi.unstubAllGlobalscomponentsLogger.warnin locked-stream test (full warn-and-skip contract)createElementmock by tag name to avoid intercepting all callsas unknown as Pipelineinstead of dishonestas nevercastReview & Testing Checklist for Human
data-testidattributes onStateIndicatorandErrorBadgedon't interfere with existing component styling or behavior (ui/src/components/NodeStateIndicator.tsx:454-455)bun run testlocally to confirm all 643 tests passwithin()scoped queries in TemplateSelector correctly find the count badges within section headersNotes
Only source file modified is
NodeStateIndicator.tsx(added twodata-testidprops). All other changes are test-only.Link to Devin session: https://staging.itsdev.in/sessions/52a329bf2069460bbd7348914aad5a8c
Requested by: @streamer45
Devin Review
4f5fc78(HEAD is7678cb9)