test(boost): add catalog unit, integration, and Playwright coverage - #4501
test(boost): add catalog unit, integration, and Playwright coverage#4501HusneShabbir wants to merge 1 commit into
Conversation
Signed-off-by: HusneShabbir <husneshabbir447@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 4:35 PM UTC · Completed 4:45 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $4.78 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4501 +/- ##
==========================================
+ Coverage 61.62% 61.72% +0.10%
==========================================
Files 2569 2569
Lines 102831 102844 +13
Branches 28787 28787
==========================================
+ Hits 63367 63478 +111
+ Misses 38888 38790 -98
Partials 576 576
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Review — ApproveClean test-coverage PR that adds unit, integration, component, and Playwright e2e tests to the Boost workspace. No production code is modified. All test files were verified against their source implementations — assertions are correct, mock data shapes match real types, and the Playwright e2e locator strategy uses semantic ARIA roles. FindingsAll findings are low severity. None block merge. Style & conventions
Documentation currency
Dimension summaries
Labels: Test-only PR adding coverage to the Boost workspace. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
workspaces/boost/plugins/boost/src/components/catalog/AiCatalogPage.test.tsx(file-level): Line 412 · [low] code-organization
The two new test cases inline the full renderInTestApp + TestApiProvider + AiCatalogPage boilerplate instead of reusing the existing renderPage() helper at line 76. The helper could be extended to accept an optional routeEntries parameter.
| baseURL: process.env.PLAYWRIGHT_URL ?? 'http://localhost:3000', | ||
| screenshot: 'only-on-failure', | ||
| trace: 'on-first-retry', | ||
| navigationTimeout: 60_000, |
There was a problem hiding this comment.
[low] config-convention
testDir is specified per-project inside the projects array rather than at the top level of defineConfig. Other workspace Playwright configs in the repo place testDir at the top level.
fullsend-ai-review
Bot
added
ready-for-merge
| address && typeof address === 'object' ? address.port : undefined; | ||
| expect(port).toBeDefined(); | ||
|
|
||
| const response = await fetch(`http://127.0.0.1:${port}/api/boost/health`); |
There was a problem hiding this comment.
This won't catch a regression in the auth policy. startTestBackend wires mockServices.httpAuth, and MockHttpAuthService falls back to mockCredentials.user() when the request carries no Authorization header — so createCredentialsBarrier sees a valid user principal and calls next() regardless. Delete httpRouter.addAuthPolicy({ path: '/health', allow: 'unauthenticated' }) from plugin.ts and this test still gets a 200.
Sending none-credentials is what pins the policy down:
import { mockCredentials } from '@backstage/backend-test-utils';
const response = await fetch(`http://127.0.0.1:${port}/api/boost/health`, {
headers: { Authorization: mockCredentials.none.header() },
});Separately, boost.security.mode: 'development-only-no-auth' above is a no-op here — validateSecurityMode returns exactly that when the key is unset, and the plugin only logs the result.
| await page.goto('/ai-catalog'); | ||
| const enter = page.getByRole('button', { name: 'Enter' }); | ||
| const heading = page.getByRole('heading', { name: 'AI Catalog' }); | ||
| await Promise.race([ |
There was a problem hiding this comment.
Promise.race doesn't cancel the loser. With a fresh context there's no provider in localStorage, so the Guest card renders and enter wins — which leaves heading.waitFor({ timeout: 20_000 }) pending with its deadline counted from goto. The explicit expect(heading).toBeVisible({ timeout: 20_000 }) further down starts counting from the click instead, so on a cold first run where the app needs ~25s to reach the catalog, that assertion passes while the orphaned waitFor still rejects at 20s. Playwright's worker turns an unhandled rejection into a test failure, so it lands as a random red on whatever assertion happened to be running.
locator.or() avoids the dangling promise:
await expect(enter.or(heading).first()).toBeVisible({ timeout: 30_000 });Worth getting right because this PR is what switches Playwright on for boost in CI — the step is gated on playwright.config.ts existing, and it runs on both node 22 and 24.
| it('renders nothing when there is no summary content', async () => { | ||
| const { container } = await renderWithEntity(emptyEntity); | ||
| expect(screen.queryByText(msg.card.summaryTitle)).toBeNull(); | ||
| expect(container.querySelector('[class*="card"]')).toBeNull(); |
There was a problem hiding this comment.
This assertion can't fail. @backstage/ui renders Card with class="bui-Card Card_bui-Card__<hash>", and CSS attribute-value matching is case-sensitive, so [class*="card"] never matches bui-Card and querySelector returns null whatever the component did.
That leaves line 89 carrying the whole test — if SummaryCard ever regressed to returning an empty <Card> shell instead of null, both assertions would still pass. expect(container).toBeEmptyDOMElement() says what you actually mean.
| expect(container.querySelector('[class*="card"]')).toBeNull(); | ||
| }); | ||
|
|
||
| it('renders agent-only fields and available models', async () => { |
There was a problem hiding this comment.
The fixtures make the isAgent gate untestable by construction: skillEntity carries none of instructions/handoffDescription/enableRAG and agentEntity carries all three. You could delete all three isAgent ? ... : undefined guards in SummaryCard.tsx and every test in this file still passes.
Since the test is named "agent-only fields", a third fixture would pin down the "only" part — a type: 'skill' entity carrying instructions and enableRAG: true, asserting queryByText(msg.card.instructionsTitle) comes back null.
|
|
||
| expect(screen.getByText(msg.table.name)).toBeInTheDocument(); | ||
| expect(screen.getByText(msg.table.type)).toBeInTheDocument(); | ||
| expect(screen.getByText('Code Review Skill')).toBeInTheDocument(); |
There was a problem hiding this comment.
You're already rendering the link here — worth asserting where it points. entityHref builds /catalog/${namespace}/${kind.toLowerCase()}/${name} and has no test anywhere in the plugin; entityHelpers.test.ts covers entityRefHref, getAdoptionAction and applyEntityFilters, but not this one. It feeds every card link and every table row href, so a slip in the kind lowercasing or the namespace default would 404 every entity page with nothing going red.
expect(screen.getByRole('link', { name: 'Code Review Skill' })).toHaveAttribute(
'href',
'/catalog/default/airesource/code-review-skill',
);|
|
||
| expect(screen.getByText('Failed to load AI assets')).toBeInTheDocument(); | ||
| expect(screen.getByText('catalog render exploded')).toBeInTheDocument(); | ||
| expect(screen.getByText('Retry')).toBeInTheDocument(); |
There was a problem hiding this comment.
This checks the button is there but never that it does anything — handleRetry and its onPress wiring could both be removed and both tests stay green. Recovering from the error is the only stateful behaviour the component has.
A child that throws on its first render and succeeds afterwards would let you click Retry and assert the children come back.
| expect(screen.getByText('Code Review Skill')).toBeInTheDocument(); | ||
| }); | ||
| expect(screen.getByText(msg.table.name)).toBeInTheDocument(); | ||
| expect(screen.getByLabelText(msg.toolbar.viewTable)).toBeInTheDocument(); |
There was a problem hiding this comment.
nit: this one is true in grid view too — the ToggleButtonGroup renders both buttons unconditionally, so the label is in the document either way. msg.table.name on the line above is the only assertion that actually distinguishes table from grid. Asserting the toggle's selected state, or that the card links are gone, would cover the URL to view-mode contract.
| * limitations under the License. | ||
| */ | ||
|
|
||
| import { expect, test, type Page } from '@playwright/test'; |
There was a problem hiding this comment.
nit: nothing type-checks this file. The workspace tsconfig.json only includes packages/*/src, plugins/*/src, plugins/*/dev and plugins/*/migrations, so yarn tsc:full never sees e2e-tests/, and Playwright transpiles without type-checking. global-header, homepage and quickstart all add "e2e-tests" to their include — a one-line change would put these 241 new lines under the compiler.
2 participants
Description
This PR was planned and raised using the automation-coverage MCP against the Boost workspace (
workspaces/boost). That MCP reads coverage and git scope, then recommends the cheapest test layer that can catch each failure. UI locators for the plugin Playwright specs were generated with Playwright MCP (browser_snapshot→browser_generate_locator→ verify) against the live NFS app — not from a scenario paragraph,#rootARIA dumps,nth(), or generated react-aria ids.No published plugin
src/changed, so there is no changeset. Overlay and cluster e2e were not added: this work is plugin-source coverage only.Tests added by layer
Cheapest layer that can catch the failure wins. The same assertion is not duplicated downstream.
L1 — Unit (Jest)
plugins/boost/src/utils/categoryMeta.test.tsgetCategoryMetamapsskill→ Skills (case-insensitive) and falls back to Unknown;getAllCategoriesreturns every known id/label.L2 — Integration (
startTestBackend)plugins/boost-backend/src/plugin.integration.test.tsboostPluginwith a catalog mock and asserts unauthenticatedGET /api/boost/healthreturns200and{ status: 'ok' }.L3 — Component (React Testing Library)
plugins/boost/src/components/catalog/EmptyFilteredState.test.tsxonClearFilters.plugins/boost/src/components/catalog/AiCatalogTable.test.tsxplugins/boost/src/components/catalog/ErrorBoundary.test.tsxplugins/boost/src/components/catalog/entity/SummaryCard.test.tsxplugins/boost/src/components/catalog/entity/VersionListCard.test.tsxplugins/boost/src/components/catalog/AiCatalogPage.test.tsx?q=no-such-asset, and table view when?view=tableis in the URL (plus existing loading / grid / empty / error / search cases).L4 — Plugin Playwright e2e (Playwright MCP)
Workspace config:
playwright.config.ts,yarn test:e2e. Catalog API is mocked (page.route) so the spec does not need a live backend. Role locators only.e2e-tests/boost.AiCatalogPage.test.tstype=skill. Search keeps only matching cards and setsq. Table view lists both assets ingrid "Data table"and setsview=table. Unmatched search → empty-filtered copy; Clear filters restores both cards and dropsq.Not in this PR: overlay smoke/e2e, cluster-free e2e, cluster e2e.
Checklist