test(e2e): add SBOM Groups BDD test coverage#1108
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds comprehensive SBOM Groups UI BDD coverage, enriches SBOM Groups list/detail page objects and shared UI helpers, and introduces extensive SBOM Groups API tests and helpers for CRUD, hierarchy, assignments, sorting, filtering, and pagination. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1108 +/- ##
==========================================
+ Coverage 53.04% 53.57% +0.52%
==========================================
Files 269 269
Lines 5904 5904
Branches 1849 1849
==========================================
+ Hits 3132 3163 +31
+ Misses 2465 2447 -18
+ Partials 307 294 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- There is repeated setup logic for ensuring groups (and parent/child relationships) exist across multiple steps; consider extracting this into shared helper utilities to reduce duplication and keep test maintenance easier.
- The invalid group ID error assertion uses a broad heading regex (
/error|not found|something went wrong/i); tightening this to a known error message or a more specific selector would make the test less flaky and more resilient to copy changes. - Several selectors rely on visible text patterns (e.g.,
text=/\d+ SBOMs?/and label text like "Product"); if UI copy changes are expected, you may want to pivot to more stable attributes or test-ids to avoid brittle tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is repeated setup logic for ensuring groups (and parent/child relationships) exist across multiple steps; consider extracting this into shared helper utilities to reduce duplication and keep test maintenance easier.
- The invalid group ID error assertion uses a broad heading regex (`/error|not found|something went wrong/i`); tightening this to a known error message or a more specific selector would make the test less flaky and more resilient to copy changes.
- Several selectors rely on visible text patterns (e.g., `text=/\d+ SBOMs?/` and label text like "Product"); if UI copy changes are expected, you may want to pivot to more stable attributes or test-ids to avoid brittle tests.
## Individual Comments
### Comment 1
<location path="e2e/tests/ui/features/@sbom-groups/sbom-groups.step.ts" line_range="598-602" />
<code_context>
+ await page.goto("/sbom-groups/invalid-group-id-12345");
+});
+
+Then("An error state is displayed for the invalid group", async ({ page }) => {
+ const errorHeading = page.getByRole("heading", {
+ name: /error|not found|something went wrong/i,
+ });
+ await expect(errorHeading).toBeVisible({ timeout: 10000 });
+});
+
</code_context>
<issue_to_address>
**suggestion (testing):** Error state assertion for invalid group ID is quite generic and may produce false positives
This assertion currently matches any heading with `/error|not found|something went wrong/i`, which could pick up unrelated page errors and cause false positives. To make the test reliably about SBOM group errors, assert against a more specific selector or message for the SBOM groups error UI (e.g., a dedicated data-testid or fixed heading text), and optionally check that the URL stays on the invalid group route. That way the test proves the invalid group ID triggers the intended error state, not just any generic error on the page.
Suggested implementation:
```typescript
// Invalid group ID handling
When("User navigates to group details with invalid ID", async ({ page }) => {
await page.goto("/sbom-groups/invalid-group-id-12345");
});
Then("An error state is displayed for the invalid group", async ({ page }) => {
// Assert we stay on the invalid group route
await expect(page).toHaveURL(/\/sbom-groups\/invalid-group-id-12345$/);
// Assert the dedicated SBOM group error UI is shown
const sbomGroupError = page.getByTestId("sbom-group-error");
await expect(sbomGroupError).toBeVisible();
});
Then("The SBOM Groups table shows all groups", async ({ page }) => {
const table = page.getByRole("treegrid", { name: "sbom-groups-table" });
await expect(table).toBeVisible();
const rows = table.getByRole("row");
await expect(rows.first()).toBeVisible();
});
Then(
```
1. Ensure the SBOM group error UI in the application uses `data-testid="sbom-group-error"` on the container element (e.g., the alert or error panel). If a different test id or fixed heading text is already used (such as `"sbom-group-not-found-heading"` or a specific error message string), update the selector in this step definition to match the existing UI.
2. If the invalid-group route differs (for example, it includes a workspace/org prefix or uses a different path segment), adjust the regular expression in `toHaveURL` to match the actual route shape.
</issue_to_address>
### Comment 2
<location path="e2e/tests/ui/features/@sbom-groups/sbom-groups.step.ts" line_range="445" />
<code_context>
Then(
"The SBOM Groups table shows filtered results containing {string}",
async ({ page }, searchTerm: string) => {
</code_context>
<issue_to_address>
**suggestion (testing):** SBOM count assertion doesn’t distinguish zero vs non-zero counts or missing content
The `Then "The SBOM count is displayed for group {string}"` step only asserts that text matching `"\d+ SBOMs?"` is visible, which passes for `0 SBOMs` and could miss cases where the count is absent but similar text appears. If the behavior requires at least one SBOM, please assert on the parsed numeric value (e.g., `> 0`). Otherwise, add a dedicated scenario for `0 SBOMs` so both zero and non-zero states are explicitly covered.
Suggested implementation:
```typescript
Then(
"The SBOM count is displayed for group {string}",
async ({ page }, groupName: string) => {
const groupRow = page.getByRole("row", { name: new RegExp(groupName, "i") });
await expect(groupRow).toBeVisible();
const sbomCountLocator = groupRow.getByText(/\d+\s+SBOMs?/);
const sbomCountText = await sbomCountLocator.textContent();
expect(sbomCountText).not.toBeNull();
const match = sbomCountText!.match(/(\d+)\s+SBOMs?/);
expect(match).not.toBeNull();
const sbomCount = Number.parseInt(match![1], 10);
expect(sbomCount).toBeGreaterThan(0);
},
);
```
1. Ensure the DOM structure for the SBOM count matches the locator assumptions (count text within the group row and formatted like `"N SBOMs"`). If the count is in a different element (e.g., a specific column), adjust `sbomCountLocator` accordingly (e.g., `groupRow.getByRole("cell", { name: /\d+\s+SBOMs?/ })`).
2. If the product must support a valid `0 SBOMs` state, add a dedicated scenario to your feature file (e.g., `"The SBOM count shows zero SBOMs for group {string}"`) that asserts `sbomCount === 0` instead of `> 0`, using the same parsing logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mrrajan
marked this pull request as draft
June 26, 2026 12:41
mrrajan
force-pushed
the
TC-3811
branch
6 times, most recently
from
July 7, 2026 06:03
d7e3329 to
a683422
Compare
mrrajan
force-pushed
the
TC-3811
branch
4 times, most recently
from
July 9, 2026 16:56
819e6f4 to
3003ccf
Compare
mrrajan
marked this pull request as ready for review
July 9, 2026 16:56
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Methods like
hasLabelBadgeandhasSbomCountonSbomGroupListPagereturnLocators rather than booleans, which is slightly misleading given their names; consider renaming them (e.g.getLabelBadgeLocator,getSbomCountLocator) or changing them to actually perform existence checks. - The new
toHaveBulkSelectedCountmatcher inToolbarMatchershardcodes#bulk-selected-items-checkbox, which may be brittle if the DOM changes; it might be more robust to tie this to the existing toolbar structure or a data-testid rather than a raw ID.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Methods like `hasLabelBadge` and `hasSbomCount` on `SbomGroupListPage` return `Locator`s rather than booleans, which is slightly misleading given their names; consider renaming them (e.g. `getLabelBadgeLocator`, `getSbomCountLocator`) or changing them to actually perform existence checks.
- The new `toHaveBulkSelectedCount` matcher in `ToolbarMatchers` hardcodes `#bulk-selected-items-checkbox`, which may be brittle if the DOM changes; it might be more robust to tie this to the existing toolbar structure or a data-testid rather than a raw ID.
## Individual Comments
### Comment 1
<location path="e2e/tests/api/features/sbom-groups.ts" line_range="563-568" />
<code_context>
+ await cleanupGroups(axios, createdGroupIds.splice(0));
+ });
+
+ const findFirstSbomId = async (axios: import("axios").AxiosInstance) => {
+ const response = await axios.get("/api/v3/sbom", {
+ params: { limit: 1, offset: 0 },
+ });
+ expect(response.data.items.length).toBeGreaterThan(0);
+ return response.data.items[0].id as string;
+ };
+
</code_context>
<issue_to_address>
**suggestion:** Deduplicate `findFirstSbomId` / `findTwoSbomIds` helpers across describe blocks
These helpers are redefined in multiple `describe` blocks, which makes SBOM lookup logic harder to maintain consistently. Please extract them into a shared helper (in this file or a small module) and reuse them across the hierarchy and assignments suites so the tests stay DRY and are easier to update if the SBOM listing API changes.
Suggested implementation:
```typescript
const findFirstSbomId = async (axios: import("axios").AxiosInstance) => {
const response = await axios.get("/api/v3/sbom", {
params: { limit: 1, offset: 0 },
});
expect(response.data.items.length).toBeGreaterThan(0);
return response.data.items[0].id as string;
};
const findTwoSbomIds = async (axios: import("axios").AxiosInstance) => {
const response = await axios.get("/api/v3/sbom", {
params: { limit: 2, offset: 0 },
});
expect(response.data.items.length).toBeGreaterThanOrEqual(2);
return [response.data.items[0].id as string, response.data.items[1].id as string];
};
test.describe("SBOM Group hierarchy with assignments", () => {
const createdGroupIds: string[] = [];
test.afterEach(async ({ axios }) => {
await cleanupGroups(axios, createdGroupIds.splice(0));
});
```
```typescript
test.afterEach(async ({ axios }) => {
await cleanupGroups(axios, createdGroupIds.splice(0));
});
test.describe("SBOM Group CRUD", () => {
```
The `findFirstSbomId` and `findTwoSbomIds` helpers are now defined once at the top level and should be reused across all `describe` blocks in this file:
1. Remove any other local `const findFirstSbomId = ...` and `const findTwoSbomIds = ...` definitions from other `describe` blocks, and update those tests to call the shared top-level helpers instead.
2. If this file currently imports `AxiosInstance` differently (e.g. `import type { AxiosInstance } from "axios";`), you may want to adjust the helper signatures to match the existing import style rather than using `import("axios").AxiosInstance` inline.
3. Ensure all call sites (in hierarchy and CRUD suites) now call the shared `findFirstSbomId(axios)` / `findTwoSbomIds(axios)` helpers, so that SBOM lookup logic is maintained in a single place.
</issue_to_address>
### Comment 2
<location path="e2e/tests/api/features/sbom-groups.ts" line_range="907-913" />
<code_context>
+ });
+});
+
+// Added Patch tests - disabled for now
+// test.describe("SBOM Group PATCH assignments", () => {
+// const createdGroupIds: string[] = [];
+
+// test.afterEach(async ({ axios }) => {
+// await cleanupGroups(axios, createdGroupIds.splice(0));
+// });
+
+// const findFirstSbomId = async (axios: import("axios").AxiosInstance) => {
</code_context>
<issue_to_address>
**question (testing):** Clarify or re-enable the commented-out PATCH assignment regression tests
The entire `SBOM Group PATCH assignments` test suite, including cases like TC-5036 and TC-5058, is commented out, removing regression coverage for partial updates and bulk PATCH semantics. If PATCH support is intentionally disabled, please add a brief note (e.g., ticket/feature flag reference) and consider moving these tests to a `skip`/`todo` suite. If PATCH is supported in the API, please re-enable these tests (or a targeted subset) so assignment behavior remains covered by automation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+907
to
+913
| // Added Patch tests - disabled for now | ||
| // test.describe("SBOM Group PATCH assignments", () => { | ||
| // const createdGroupIds: string[] = []; | ||
|
|
||
| // test.afterEach(async ({ axios }) => { | ||
| // await cleanupGroups(axios, createdGroupIds.splice(0)); | ||
| // }); |
Contributor
There was a problem hiding this comment.
question (testing): Clarify or re-enable the commented-out PATCH assignment regression tests
The entire SBOM Group PATCH assignments test suite, including cases like TC-5036 and TC-5058, is commented out, removing regression coverage for partial updates and bulk PATCH semantics. If PATCH support is intentionally disabled, please add a brief note (e.g., ticket/feature flag reference) and consider moving these tests to a skip/todo suite. If PATCH is supported in the API, please re-enable these tests (or a targeted subset) so assignment behavior remains covered by automation.
Contributor
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as question — asks for clarification about commented-out PATCH assignment tests; no code change needed. No sub-task created.
mrrajan
marked this pull request as draft
July 9, 2026 16:59
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review (4569251325) — Classified as suggestion (3 items):
No sub-tasks created. |
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review (4665095148) — Classified as suggestion (2 items):
No sub-tasks created. |
Contributor
Author
Verification Report for TC-3811 (commit 3003ccf)
Overall: WARNSummary: The PR substantially exceeds the Jira task requirements — adding 11 BDD scenarios, comprehensive API test coverage, and page object improvements. All CI checks pass and all feasible acceptance criteria are satisfied. The WARN verdicts are minor:
None of the reviewer suggestions (from sourcery-ai[bot]) were upgraded to code change requests — no matching CONVENTIONS.md patterns or established codebase conventions were found. This comment was AI-generated by sdlc-workflow/verify-pr v0.11.0. |
mrrajan
marked this pull request as ready for review
July 10, 2026 09:49
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several helper methods in
SbomGroupListPage(e.g.hasLabelBadge,hasSbomCount) are markedasyncand typed as returningPromise<Locator>but only synchronously construct and return aLocator; consider droppingasync/Promisehere to avoid confusion and clarify that these methods are purely selector builders. - A number of UI scenarios rely on hard-coded SBOM names like
curl,openssl-3,ubi9-minimal-container, andclaude-4-opus; if those fixtures change these tests will become brittle, so it may be worth centralizing or deriving these names from API/fixture helpers to make the tests more resilient.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several helper methods in `SbomGroupListPage` (e.g. `hasLabelBadge`, `hasSbomCount`) are marked `async` and typed as returning `Promise<Locator>` but only synchronously construct and return a `Locator`; consider dropping `async`/`Promise` here to avoid confusion and clarify that these methods are purely selector builders.
- A number of UI scenarios rely on hard-coded SBOM names like `curl`, `openssl-3`, `ubi9-minimal-container`, and `claude-4-opus`; if those fixtures change these tests will become brittle, so it may be worth centralizing or deriving these names from API/fixture helpers to make the tests more resilient.
## Individual Comments
### Comment 1
<location path="e2e/tests/ui/features/@sbom-groups/sbom-groups.step.ts" line_range="63-72" />
<code_context>
+ await modal.submit();
});
Then(
- "The SBOM Groups table contains {string}",
- async ({ page }, groupName: string) => {
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the "table does not contain" assertion to fail if the row still exists in the DOM
This step currently uses `await expect(row).not.toBeVisible();`, which will pass if the locator matches zero rows—even if the group still exists under a different representation or the locator broke. To align with the scenario intent that the row is removed from the table, please assert on count, e.g.:
```ts
const listPage = await SbomGroupListPage.fromCurrentPage(page);
const row = listPage.getGroupRow(groupName);
await expect(row).toHaveCount(0);
```
Optionally, you can add a visibility check after the count if needed.
Suggested implementation:
```typescript
Then(
"The SBOM Groups table does not contain {string}",
async ({ page }, groupName: string) => {
const listPage = await SbomGroupListPage.fromCurrentPage(page);
const row = listPage.getGroupRow(groupName);
// Assert that the row has been removed from the DOM entirely
await expect(row).toHaveCount(0);
},
);
```
If the existing step text is slightly different (for example, `"The SBOM Groups table contains {string}"` with an inverted expectation), adjust the `Then(...)` string in the SEARCH/REPLACE block to match the exact wording currently in your file. Also ensure that:
1. `SbomGroupListPage` is imported at the top of this file (e.g., `import { SbomGroupListPage } from '../../page-objects/sbom-group-list.page';` or whatever path is used elsewhere).
2. `expect` from `@playwright/test` is already imported; if not, add `import { expect } from '@playwright/test';` alongside your other Playwright imports.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
carlosthe19916
requested changes
Jul 13, 2026
carlosthe19916
left a comment
carlosthe19916
left a comment
Collaborator
There was a problem hiding this comment.
Do not use: "timeout" statements, avoid explicit timeout-based waits because they make tests slower, less reliable, and dependent on machine performance. Playwright already provides automatic waiting for elements, navigation, assertions, and actions, which makes tests more deterministic and less flaky.
Comment on lines
+46
to
+48
| await expect(this._pagination.locator("input")).toHaveValue("1", { | ||
| timeout: 5000, | ||
| }); |
Collaborator
There was a problem hiding this comment.
Why is it that we need to add timeout here?
The whole Pagination class is independent of the any asynchronous call like REST APIs.
If changing the page triggers a Table or any other component to be re-rendered then the "wait/timeout" action should be written in the Table but not here.
Contributor
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5189 created to address this feedback.
| name: "Clear all filters", | ||
| }); | ||
| await expect(clearButton).toBeVisible(); | ||
| if (!(await clearButton.isVisible())) return; |
Collaborator
There was a problem hiding this comment.
Let's not mix logic. The function should clear filters, verifying whether or not the filters exist should be done outside this function. Prefer composition logic over conditional expansion
Contributor
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5190 created to address this feedback.
Comment on lines
+179
to
+182
| // Give PatternFly label-group animations time to fully complete | ||
| await expect(this._toolbar.locator(".pf-m-label-group")).toHaveCount(0, { | ||
| timeout: 10000, | ||
| }); |
Collaborator
There was a problem hiding this comment.
Don't add this. The whole Toolbar class is independent of any asynchronous event. If the toolbar triggers other components to be re-rendered asynchronously then the "wait" or async logic should be there not here
Contributor
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5189 created to address this feedback.
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review (4670363250) — Classified as suggestion (2 items):
No sub-tasks created. |
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @carlosthe19916 review (4682471904) — Classified as code change request — sub-task TC-5189 created to remove explicit timeout waits from Pagination and Toolbar page objects. |
Contributor
Author
Verification Report for TC-3811 (commit c0d2035)
Overall: WARNIssues requiring attention:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.2. |
carlosthe19916
approved these changes
Jul 14, 2026
mrrajan
added
the
backport release/0.5.z
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 14, 2026
Add missing E2E test coverage for SBOM Groups feature: - Product label badge visibility in list and detail pages - Hierarchical tree expand/collapse behavior - Parent group selection in create and edit flows - Invalid group ID error handling - SBOM count display in group list - Breadcrumb navigation on detail page - Edit group parent assignment and removal - Sorting by name column Implements TC-3811 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Wrap the three assignment describe blocks in a serial parent to prevent parallel workers from racing on shared SBOMs via PUT replace semantics. Extract duplicated findFirstSbomId/findTwoSbomIds helpers to the shared helpers module. Add 8 new tests covering assignment edge cases (bulk error paths, PATCH clear/idempotent-remove/add-remove-same-group, delete-group-cleans-assignments, filter-by-parent). Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Playwright provides automatic waiting for elements and assertions, making explicit timeouts unnecessary in shared infrastructure classes. Implements TC-5189 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
Remove conditional guard from clearAllFilters() so the method only performs the clear action. Callers are responsible for checking filter existence before calling. Playwright auto-waiting on click() handles element readiness. Implements TC-5190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Contributor
|
Successfully created backport PR for |
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
2 participants
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.
Summary
SbomGroupDetailPage,AddToGroupModal; enhanced:SbomGroupListPage,GroupFormModal,DetailsPageLayout,ConfirmDialogsbom-group-helpers.tsclearAllFilters(TC-5190)UI Scenarios Added
API Test Suites
Not Implemented (features not yet in codebase)
Test plan
npm run e2e:test:uito verify all UI scenarios passnpm run e2e:test:apito verify all API tests passImplements TC-3811