test(e2e): add BDD tests for CVSS display and vulnerability detail tabs - #1110
Open
xinredhat wants to merge 1 commit into
Open
test(e2e): add BDD tests for CVSS display and vulnerability detail tabs#1110xinredhat wants to merge 1 commit into
xinredhat wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideAdds Playwright BDD end-to-end coverage for CVSS display in the vulnerability and SBOM explorers, plus related SBOMs/advisories tabs, backed by new test datasets for multi-source CVSS scoring. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- Several helpers (e.g., DependencySubTable.verifyRowByName and CvssAssessmentPanel.verifySeverityBadge) rely on cell indices or global text matching, which makes the tests sensitive to layout/text changes; consider targeting cells via data-labels or scoping locators to their section to reduce brittleness.
- CvssBreakdownPopover.verifyRowValues filters rows using page-level locators rather than scoping the match to the breakdown table, which could accidentally match other content on the page; it would be more robust to use
haslocators derived from the table itself. - The new feature files hard-code specific vulnerability/advisory IDs and SBOM names, so any dataset updates will break multiple scenarios at once; if possible, centralize these test fixtures or add a helper to look up IDs by semantic description to make future data changes easier to manage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several helpers (e.g., DependencySubTable.verifyRowByName and CvssAssessmentPanel.verifySeverityBadge) rely on cell indices or global text matching, which makes the tests sensitive to layout/text changes; consider targeting cells via data-labels or scoping locators to their section to reduce brittleness.
- CvssBreakdownPopover.verifyRowValues filters rows using page-level locators rather than scoping the match to the breakdown table, which could accidentally match other content on the page; it would be more robust to use `has` locators derived from the table itself.
- The new feature files hard-code specific vulnerability/advisory IDs and SBOM names, so any dataset updates will break multiple scenarios at once; if possible, centralize these test fixtures or add a helper to look up IDs by semantic description to make future data changes easier to manage.
## Individual Comments
### Comment 1
<location path="e2e/tests/ui/pages/vulnerability-details/sboms/DependencySubTable.ts" line_range="20-29" />
<code_context>
+ return new DependencySubTable(page, table);
+ }
+
+ async verifyRowByName(
+ depName: string,
+ depType: string,
+ depNamespace: string,
+ depVersion: string,
+ ) {
+ const row = this._table.locator("tbody tr").filter({
+ has: this._page.getByRole("link", { name: depName, exact: true }),
+ });
+ await expect(row).toBeVisible();
+
+ const cells = row.locator("td");
+ await expect(cells.nth(0)).toHaveText(depType);
+ if (depNamespace) {
+ await expect(cells.nth(1)).toHaveText(depNamespace);
+ }
+ await expect(cells.nth(3)).toHaveText(depVersion);
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Add explicit assertions for the namespace cell in `verifyRowByName`, including the empty namespace case
Currently the test only checks `cells.nth(1)` when `depNamespace` is truthy, so rows with an empty or undefined namespace aren’t validated. Please add explicit expectations for both cases: assert the namespace cell text when `depNamespace` is set, and when it’s empty or a sentinel value, assert that the cell is empty or shows the expected "no namespace" indicator. This will ensure the SBOM dependency rendering is fully covered.
Suggested implementation:
```typescript
async verifyRowByName(
depName: string,
depType: string,
depNamespace: string | null | undefined,
depVersion: string,
) {
const row = this._table.locator("tbody tr").filter({
has: this._page.getByRole("link", { name: depName, exact: true }),
});
await expect(row).toBeVisible();
const cells = row.locator("td");
await expect(cells.nth(0)).toHaveText(depType);
const namespaceCell = cells.nth(1);
if (depNamespace) {
// Explicitly validate the rendered namespace when provided.
await expect(namespaceCell).toHaveText(depNamespace);
} else {
// Explicitly validate the "no namespace" rendering (empty cell / indicator).
await expect(namespaceCell).toHaveText("");
}
await expect(cells.nth(3)).toHaveText(depVersion);
}
```
If the UI renders a specific "no namespace" indicator (e.g. "—" or "No namespace") instead of an empty string, replace `await expect(namespaceCell).toHaveText("");` with that exact expected text so the assertion matches the SBOM dependency rendering.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/0.5.z #1110 +/- ##
==============================================
Coverage 51.26% 51.26%
==============================================
Files 256 256
Lines 5516 5516
Branches 1671 1671
==============================================
Hits 2828 2828
Misses 2423 2423
Partials 265 265
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:
|
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several selectors rely on visible text (e.g., heading names, column labels, button names); consider centralizing these strings or using more stable attributes/test-ids to reduce brittleness when UI copy changes.
- The
DependencySubTable.verifyRowByNamemethod hardcodes column indices for type/namespace/version; using data-labels or named accessors instead would make the tests more resilient to table layout changes. - For CVSS tooltip and popover content, the assertions use full, exact strings; if the explanatory copy is expected to evolve, consider asserting on key phrases rather than the entire text to avoid frequent test updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several selectors rely on visible text (e.g., heading names, column labels, button names); consider centralizing these strings or using more stable attributes/test-ids to reduce brittleness when UI copy changes.
- The `DependencySubTable.verifyRowByName` method hardcodes column indices for type/namespace/version; using data-labels or named accessors instead would make the tests more resilient to table layout changes.
- For CVSS tooltip and popover content, the assertions use full, exact strings; if the explanatory copy is expected to evolve, consider asserting on key phrases rather than the entire text to avoid frequent test updates.
## Individual Comments
### Comment 1
<location path="e2e/tests/ui/pages/sbom-details/vulnerabilities/CvssBreakdownPopover.ts" line_range="48-57" />
<code_context>
+ await expect(rows).toHaveCount(expectedRows);
+ }
+
+ async verifyRowValues(score: string, severity: string, version: string) {
+ await expect(this._table).toBeVisible();
+ const row = this._table
+ .locator("tbody tr")
+ .filter({
+ has: this._page.getByText(severity, { exact: true }),
+ })
+ .filter({
+ has: this._page.locator('td[data-label="Score"]', { hasText: score }),
+ });
+ await expect(row).toBeVisible();
+ await expect(row.locator('td[data-label="Version"]')).toHaveText(version);
+ }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Row matching logic for CVSS breakdown could be more tightly scoped to avoid accidental matches
In `verifyRowValues`, the row filter uses `this._page.getByText` and a generic `td[data-label="Score"]` locator, which depend on a page-wide text search. If additional tables or similar text appear in the popover, the matcher could pick the wrong row. Please scope these `has` locators to `this._table` (e.g., `this._table.getByText(...)`) or use more specific cell selectors for both severity and score within the row to keep the test robust against layout changes.
Suggested implementation:
```typescript
async verifyRowCount(expectedRows: number) {
await expect(this._table).toBeVisible();
const rows = this._table.locator("tbody tr");
await expect(rows).toHaveCount(expectedRows);
}
async verifyRowValues(score: string, severity: string, version: string) {
await expect(this._table).toBeVisible();
const row = this._table
.locator("tbody tr")
.filter({
has: this._table.locator('td[data-label="Severity"]', { hasText: severity }),
})
.filter({
has: this._table.locator('td[data-label="Score"]', { hasText: score }),
});
await expect(row).toBeVisible();
await expect(row.locator('td[data-label="Version"]')).toHaveText(version);
}
static async fromCurrentPage(page: Page) {
```
If the severity column uses a different `data-label` (e.g. `"Severity"` may need to be adjusted to match the actual markup), update the selector in `td[data-label="Severity"]` accordingly. Also, if the score cell text includes additional formatting (like parentheses or units), you may want to switch `hasText: score` to a regex or adjust the expected `score` string to match the rendered value exactly.
</issue_to_address>
### Comment 2
<location path="e2e/tests/ui/pages/vulnerability-details/sboms/DependencySubTable.ts" line_range="14-15" />
<code_context>
+ );
+ }
+
+ static async fromCurrentPage(page: Page) {
+ const popover = page.getByRole("dialog", {
+ name: "CVSS Score Breakdown",
</code_context>
<issue_to_address>
**issue:** DependencySubTable helper does not associate the sub-table with a specific SBOM row
The locator `tr:has(> td[colspan]) table` always selects the first expanded dependency sub-table, so if multiple SBOM rows are expanded, the helper can attach to the wrong table and yield misleading test results. Please tighten the association by passing the parent SBOM row (or a more specific locator) into `fromCurrentPage`, or by including the SBOM name in the locator, so that `verifyRowByName` and `verifyNameLink` target the correct SBOM’s dependencies.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
xinredhat
force-pushed
the
TC-4635
branch
2 times, most recently
from
June 30, 2026 12:30
9160960 to
badef08
Compare
mrrajan
requested changes
Jul 14, 2026
Contributor
There was a problem hiding this comment.
Hi @xinredhat, Thanks for the PR and I regret for the late in response.
I have requested changes and the most common ones are,
verify*methods with embedded expect calls inside page objects. The project assertion architecture recommends to use custom typed assertions instead of the playwright built-in ones. The custom expect inassertions/index.tsusesmergeExpectswith overloaded typedExpect - so when we pass a Table, we will getTableMatcherswith type-checked column names and the same goes forPaginationMatchers. I request to follow the project assertion architecture. Either move assertions to the step definitions with custom matchers and page object accessors to perform data verification or create new custom assertion matchers inassertions/for CVSS specific checks.- Please keep the test data specific validations as it is in the
mainbranch.
xinredhat
force-pushed
the
TC-4635
branch
2 times, most recently
from
July 15, 2026 13:03
f2f168b to
6a29a21
Compare
mrrajan
requested changes
Jul 22, 2026
mrrajan
left a comment
mrrajan
left a comment
Contributor
There was a problem hiding this comment.
@xinredhat On the step definitions, Please add missing type annotations. For example, lines 21, 28, 36, 45, 52 on cvss-assessment.step.ts missing type annotations. Same for other files are well.
|
|
||
| Then("The CVSS Score Breakdown popover is visible", async ({ page }) => { | ||
| const popover = await CvssBreakdownPopover.fromCurrentPage(page); | ||
| await expect(popover._popover).toBeVisible(); |
Contributor
There was a problem hiding this comment.
For the internal properties, please use public page object methods like getPopover(). cvss-score-breakdown.step.ts:24,26,47
|
|
||
| Then("The CVSS Assessment heading is visible", async ({ page }) => { | ||
| const panel = await CvssAssessmentPanel.fromCurrentPage(page); | ||
| await expect(panel._heading).toBeVisible(); |
Contributor
There was a problem hiding this comment.
Should expose isHeadingVisible() / isHeadingNotVisible() on the page object
Author
There was a problem hiding this comment.
Don't use manual assertions that are not awaiting the expect. When using assertions such as isVisible() the test won't wait a single second, it will just check the locator is there and return immediately. Use web first assertions such as toBeVisible() instead.
https://playwright.dev/docs/best-practices#use-web-first-assertions
Add 28 E2E BDD tests covering 5 gaps identified in TC-4635: - Gap 1: CVSS Score Breakdown popover on SBOM Vulnerabilities tab (8 tests) - Gap 2: CVSS score, severity, and version tag on Vulnerability list page (4 tests) - Gap 3: CVSS Assessment panel on Vulnerability details page (3 tests) - Gap 4: Related SBOMs tab with status, supplier, and dependency expansion (7 tests) - Gap 5: Related Advisories tab with multi-type advisory validation (6 tests) Co-Authored-By: Claude
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.
Add 28 E2E BDD tests covering 5 gaps identified in TC-4635:
Assisted-by: Claude
Summary by Sourcery
Add end-to-end BDD coverage for CVSS-related UI and vulnerability detail views across SBOM and vulnerability explorer pages.
Tests: