Scan SBOM test scenarios #772
Merged
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
Reviewer's GuideThis PR extends the SBOM scanning test suite by adding end-to-end BDD feature files and step definitions, introducing reusable download and CSV validation helpers, refactoring global API setup for dynamic content types, improving the ToolbarTable helper, expanding test asset constants, and creating a dedicated SbomScanPage page object for streamlined interactions. 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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- This feature file is very large and repetitive; consider splitting it into smaller, focused feature files (e.g., unsupported formats, filtering, actions) to improve maintainability.
- Several scenarios share identical steps that could be consolidated into a Background or refactored using Scenario Outline to reduce duplication.
- Scenario names are duplicated (e.g., 'Verify Generate Vulnerability Report Screen') and should be made unique to avoid confusion when running tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This feature file is very large and repetitive; consider splitting it into smaller, focused feature files (e.g., unsupported formats, filtering, actions) to improve maintainability.
- Several scenarios share identical steps that could be consolidated into a Background or refactored using Scenario Outline to reduce duplication.
- Scenario names are duplicated (e.g., 'Verify Generate Vulnerability Report Screen') and should be made unique to avoid confusion when running tests.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 @@
## main #772 +/- ##
==========================================
+ Coverage 57.76% 61.59% +3.82%
==========================================
Files 163 170 +7
Lines 2872 3080 +208
Branches 654 698 +44
==========================================
+ Hits 1659 1897 +238
+ Misses 976 927 -49
- Partials 237 256 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
9041436 to
8dda8ec
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Please remove the stray console.log("debug cellText:", result) in the step definitions to avoid leftover debug output in your tests.
- The verifyDownload helper’s extension check isn’t actually asserting the result—add a proper expect(...).toBeTruthy() or toMatch(...) so failures are caught.
- These feature files are extremely large with a lot of repeated scenarios and placeholders—consider splitting them into smaller, logical pieces or consolidating outlines to keep them maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Please remove the stray console.log("debug cellText:", result) in the step definitions to avoid leftover debug output in your tests.
- The verifyDownload helper’s extension check isn’t actually asserting the result—add a proper expect(...).toBeTruthy() or toMatch(...) so failures are caught.
- These feature files are extremely large with a lot of repeated scenarios and placeholders—consider splitting them into smaller, logical pieces or consolidating outlines to keep them maintainable.
## Individual Comments
### Comment 1
<location> `e2e/tests/ui/features/@sbom-scan/scan-sbom.step.ts:61` </location>
<code_context>
+ },
+);
+
+When("User Clicks on Browse files Button", async ({ page }) => {
+ await page.getByRole("button", { name: "Browse Files" }).click();
+});
</code_context>
<issue_to_address>
**suggestion (testing):** No test for drag-and-drop file upload interaction.
Please add a test that simulates drag-and-drop file upload and checks the resulting UI and processing.
Suggested implementation:
```typescript
When("User Clicks on Browse files Button", async ({ page }) => {
await page.getByRole("button", { name: "Browse Files" }).click();
});
When("User drags and drops a file onto the upload area", async ({ page }) => {
// Locate the drop area (adjust selector as needed)
const dropArea = await page.locator('[data-testid="file-drop-area"]');
// Simulate drag-and-drop file upload
await dropArea.setInputFiles('tests/fixtures/sample-sbom.json');
});
Then("The uploaded file should appear in the file list and start processing", async ({ page }) => {
// Check that the file appears in the UI
await expect(page.getByText("sample-sbom.json")).toBeVisible();
// Check for processing indicator (adjust selector/text as needed)
await expect(page.getByTestId("processing-indicator")).toBeVisible();
});
```
- Ensure that the selector `[data-testid="file-drop-area"]` matches your actual drop area. Adjust as needed.
- Place a sample SBOM file at `tests/fixtures/sample-sbom.json` or update the path to match your test fixture location.
- Adjust `"processing-indicator"` and other selectors/texts to match your application's UI.
- If your file list or processing indicator uses different test IDs or text, update the assertions accordingly.
</issue_to_address>
### Comment 2
<location> `e2e/tests/ui/features/@sbom-scan/scan-sbom.step.ts:380-391` </location>
<code_context>
if (column === "Qualifiers") {
// Use the reusable helper for comma-delimited values
await verifyCommaDelimitedValues(cell, expected, "xpath=//td/span");
} else {
// For other columns, check if expected is empty string (for empty columns)
if (expected === "") {
const cellText = await cell.textContent();
await expect(cellText?.trim() || "").toBe("");
} else {
await expect(cell).toContainText(expected);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into `else if` ([`merge-else-if`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-else-if))
```suggestion
if (column === "Qualifiers") {
// Use the reusable helper for comma-delimited values
await verifyCommaDelimitedValues(cell, expected, "xpath=//td/span");
}
else if (expected === "") {
const cellText = await cell.textContent();
await expect(cellText?.trim() || "").toBe("");
}
else {
await expect(cell).toContainText(expected);
}
```
<br/><details><summary>Explanation</summary>Flattening if statements nested within else clauses generates code that is
easier to read and expand upon.
</details>
</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
force-pushed
the
scan_sbom_feature
branch
from
77dd548 to
20ad10a
Compare
mrrajan
requested review from
carlosthe19916,
matejnesuta,
queria and
vobratil
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Replace fixed waitForTimeout calls with explicit waits for specific element conditions to improve stability and avoid flakiness.
- Consolidate or remove duplicate scan-sbom.feature files and overlapping scenarios to reduce redundancy and simplify maintenance.
- Update SbomScanPage methods like errorVulnerabilitiesHeading/body to use await expect(...).toBeVisible() so failures are properly reported.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replace fixed waitForTimeout calls with explicit waits for specific element conditions to improve stability and avoid flakiness.
- Consolidate or remove duplicate scan-sbom.feature files and overlapping scenarios to reduce redundancy and simplify maintenance.
- Update SbomScanPage methods like errorVulnerabilitiesHeading/body to use await expect(...).toBeVisible() so failures are properly reported.
## Individual Comments
### Comment 1
<location> `e2e/tests/ui/pages/sbom-scan/SbomScanPage.ts:36-37` </location>
<code_context>
+ return this._page.getByRole("button", { name: "Actions" });
+ }
+
+ async errorVulnerabilitiesHeading(header: string) {
+ this._page.getByRole("heading", { name: header }).isVisible();
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Missing await in errorVulnerabilitiesHeading and errorVulnerabilitiesBody.
Please add 'await' before isVisible() in both methods to ensure correct test behavior.
</issue_to_address>
### Comment 2
<location> `e2e/tests/ui/features/@sbom-scan/scan-sbom.step.ts:361-370` </location>
<code_context>
if (column === "Qualifiers") {
await verifyCommaDelimitedValues(cell, expected, "xpath=//span");
} else {
if (expected === "") {
const cellText = await cell.textContent();
await expect(cellText?.trim() || "").toBe("");
} else {
await expect(cell).toContainText(expected);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into `else if` ([`merge-else-if`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-else-if))
```suggestion
if (column === "Qualifiers") {
await verifyCommaDelimitedValues(cell, expected, "xpath=//span");
}
else if (expected === "") {
const cellText = await cell.textContent();
await expect(cellText?.trim() || "").toBe("");
}
else {
await expect(cell).toContainText(expected);
}
```
<br/><details><summary>Explanation</summary>Flattening if statements nested within else clauses generates code that is
easier to read and expand upon.
</details>
</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
36
to
37
| async errorVulnerabilitiesHeading(header: string) { | ||
| this._page.getByRole("heading", { name: header }).isVisible(); |
There was a problem hiding this comment.
issue (bug_risk): Missing await in errorVulnerabilitiesHeading and errorVulnerabilitiesBody.
Please add 'await' before isVisible() in both methods to ensure correct test behavior.
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
c28273b to
b5f4168
Compare
There was a problem hiding this comment.
- Aren't
ui/features/scan-sbom.featureandui/features/@sbom-scan/scan-sbom.featurethe same? Shouldn't we deleteui/features/scan-sbom.featurefrom this PR?
I gave a quick look and I added comments to each section. Given that the tests are failing a second review will be needed once CI is green.
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
ffa9bb7 to
2eb23a9
Compare
|
Thanks for the review @carlosthe19916 I have refactored the code - Please review and let me know. |
Signed-off-by: mrrajan <[email protected].>
Signed-off-by: mrrajan <[email protected].> Assisted-by: Cursor
Signed-off-by: mrrajan <[email protected].>
Signed-off-by: mrrajan <[email protected].>
Signed-off-by: mrrajan <[email protected].>
Signed-off-by: mrrajan <[email protected].>
Signed-off-by: mrrajan <[email protected].>
mrrajan
force-pushed
the
scan_sbom_feature
branch
2 times, most recently
from
ea832bc to
2edfac3
Compare
Signed-off-by: mrrajan <[email protected].>
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
2edfac3 to
951d7cd
Compare
carlosthe19916
requested changes
Oct 23, 2025
There was a problem hiding this comment.
@mrrajan sorry for my delayed review
- Please remove the comments
// here - Also address the minor suggestion on the
PackageDetailsPagefile.
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
ea8aa02 to
b842c26
Compare
Signed-off-by: mrrajan <[email protected].>
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
b842c26 to
228d804
Compare
carlosthe19916
approved these changes
Oct 29, 2025
There was a problem hiding this comment.
@mrrajan LGTM, thanks. I am approving this PR
- Before your merge the PR please address the minor comments below and push the merge button
- It would be good to backport the changes to the release branch, so please backport it too.
| Name: "keycloak-core", | ||
| Version: "18.0.6.redhat-00001", | ||
| }); | ||
| await expect(await tableRow.count()).toBeGreaterThan(0); |
There was a problem hiding this comment.
Remove the first await because it has no effect.
Suggested change
| await expect(await tableRow.count()).toBeGreaterThan(0); | |
| expect(await tableRow.count()).toBeGreaterThan(0); |
I believe the IDE should help to find those things.
There was a problem hiding this comment.
count() returns a promise - so await is necessary
| Name: "keycloak-core", | ||
| Version: "18.0.6.redhat-00001", | ||
| }); | ||
| await expect(await tableRow.count()).toBeGreaterThan(0); |
There was a problem hiding this comment.
Same as above, let;s remove the first await
Suggested change
| await expect(await tableRow.count()).toBeGreaterThan(0); | |
| expect(await tableRow.count()).toBeGreaterThan(0); |
There was a problem hiding this comment.
count() returns a promise - so await is necessary
There was a problem hiding this comment.
Current code:
await expect(await tableRow.count()).toBeGreaterThan(0);- The current code has 2
await. The firstawaithas not effect. The second one does. - As you pointed out
count()returns a promise, therefore the secondawaitdoes apply correctly. But the first one does not.
There was a problem hiding this comment.
If we leave the current code it won't break anything. I suggested to remove the first await just in the spirit of having clean code and not get confused if in the future we review the code again... as mentioned before I think any standard IDE should be able to warn us about this kind of things.
There was a problem hiding this comment.
Ah! I see. Thanks! I will have this change part of my next PR.
e2e/tests/ui/pages/Helpers.ts
Outdated
Comment on lines
120
to
127
| await expect | ||
| .soft( | ||
| missing.length === 0, | ||
| missing.length > 0 | ||
| ? `Missing expected values: ${missing.join(", ")}. Actual values: [${actualValues.join(", ")}]` | ||
| : "Values did not match expected pattern", | ||
| ) | ||
| .toBeTruthy(); |
There was a problem hiding this comment.
Same here. Remove await as it has no effect. That is a synchronous operation so await makes no sense.
|
Thanks carlos - I have committed couple of changes and added comments on couple. |
mrrajan
added
the
backport release/0.4.z
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: mrrajan <[email protected].>
mrrajan
force-pushed
the
scan_sbom_feature
branch
from
607a20d to
aae73a9
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Oct 29, 2025
Signed-off-by: mrrajan <[email protected].> (cherry picked from commit 52fdc56)
|
Successfully created backport PR for |
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.
Added Test scenarios for manual validation.
Please note the placeholders will be updated during validation.
Summary by Sourcery
Add end-to-end SBOM scanning test scenarios and supporting helpers and page objects
New Features:
Enhancements:
Tests: