test: Add test to delete SBOM and Advisory#875
Conversation
Reviewer's GuideAdds end-to-end Playwright BDD scenarios and step definitions to cover deleting SBOMs and Advisories from both list and details pages, introduces a reusable ConfirmDialog helper, and slightly updates testing documentation and an existing SBOM sorting expectation. Sequence diagram for deleting an SBOM from list/details pagessequenceDiagram
actor Tester
participant BDDRunner
participant Playwright as PlaywrightTest
participant Browser
participant App as WebApp
participant ConfirmDialog
Tester->>BDDRunner: Run BDD delete SBOM scenario
BDDRunner->>Playwright: Execute step definitions
Playwright->>Browser: Launch and open SBOM list page
Browser->>App: GET /sboms
App-->>Browser: Render SBOM list
Note over Tester,Browser: Delete SBOM from list page
Tester->>Playwright: Step: user deletes SBOM from list
Playwright->>Browser: Click delete button for SBOM in list
Browser->>App: Open confirm dialog
App-->>Browser: Render confirm dialog
Playwright->>ConfirmDialog: new ConfirmDialog(page)
Playwright->>ConfirmDialog: confirm()
ConfirmDialog->>Browser: Click confirm button
Browser->>App: DELETE /sboms/{id}
App-->>Browser: 200 OK and updated list
Browser-->>Playwright: Updated SBOM list without deleted item
Note over Tester,Browser: Delete SBOM from details page
Tester->>Playwright: Step: user deletes SBOM from details
Playwright->>Browser: Navigate to SBOM details page
Browser->>App: GET /sboms/{id}
App-->>Browser: Render SBOM details
Playwright->>Browser: Click delete button on details page
Browser->>App: Open confirm dialog
App-->>Browser: Render confirm dialog
Playwright->>ConfirmDialog: confirm()
ConfirmDialog->>Browser: Click confirm button
Browser->>App: DELETE /sboms/{id}
App-->>Browser: 200 OK and redirect info
Browser-->>Playwright: Navigate back to SBOM list without deleted item
Playwright-->>Tester: Assert SBOM no longer visible
Sequence diagram for deleting an Advisory from list/details pagessequenceDiagram
actor Tester
participant BDDRunner
participant Playwright as PlaywrightTest
participant Browser
participant App as WebApp
participant ConfirmDialog
Tester->>BDDRunner: Run BDD delete Advisory scenario
BDDRunner->>Playwright: Execute step definitions
Playwright->>Browser: Open Advisory list page
Browser->>App: GET /advisories
App-->>Browser: Render Advisory list
Note over Tester,Browser: Delete Advisory from list page
Tester->>Playwright: Step: user deletes Advisory from list
Playwright->>Browser: Click delete button for Advisory in list
Browser->>App: Open confirm dialog
App-->>Browser: Render confirm dialog
Playwright->>ConfirmDialog: new ConfirmDialog(page)
Playwright->>ConfirmDialog: confirm()
ConfirmDialog->>Browser: Click confirm button
Browser->>App: DELETE /advisories/{id}
App-->>Browser: 200 OK and updated list
Browser-->>Playwright: Updated Advisory list without deleted item
Note over Tester,Browser: Delete Advisory from details page
Tester->>Playwright: Step: user deletes Advisory from details
Playwright->>Browser: Navigate to Advisory details page
Browser->>App: GET /advisories/{id}
App-->>Browser: Render Advisory details
Playwright->>Browser: Click delete button on details page
Browser->>App: Open confirm dialog
App-->>Browser: Render confirm dialog
Playwright->>ConfirmDialog: confirm()
ConfirmDialog->>Browser: Click confirm button
Browser->>App: DELETE /advisories/{id}
App-->>Browser: 200 OK and redirect info
Browser-->>Playwright: Navigate back to Advisory list without deleted item
Playwright-->>Tester: Assert Advisory no longer visible
Class diagram for ConfirmDialog helper and step definitionsclassDiagram
class ConfirmDialog {
- page
+ ConfirmDialog(page)
+ confirm()
+ cancel()
}
class SbomExplorerSteps {
+ openSbomList()
+ deleteSbomFromList(sbomName)
+ openSbomDetails(sbomName)
+ deleteSbomFromDetails(sbomName)
}
class AdvisoryExplorerSteps {
+ openAdvisoryList()
+ deleteAdvisoryFromList(advisoryName)
+ openAdvisoryDetails(advisoryName)
+ deleteAdvisoryFromDetails(advisoryName)
}
class SbomSearchSteps {
+ searchSbomByName(sbomName)
+ sortResultsBy(columnName)
+ verifyResultOrder(expectedOrder)
}
ConfirmDialog <.. SbomExplorerSteps : uses
ConfirmDialog <.. AdvisoryExplorerSteps : uses
SbomSearchSteps <.. SbomExplorerSteps : collaborates
SbomSearchSteps <.. AdvisoryExplorerSteps : collaborates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The step definition
When User Clicks on Actions button and Selects Delete option from the drop downis implemented in bothsbom-explorer.step.tsandadvisory-explorer.step.tswith identical behavior; consider extracting this into a shared helper or a single step definition to avoid duplication and potential ambiguity. - The logic for filtering and asserting an empty table after deletion is repeated in multiple step files for SBOMs and advisories; you could centralize this into a reusable helper (e.g.,
assertEntityNotPresentInTable) to keep the tests DRY and easier to maintain. - In
ConfirmDialog, you repeatedly look up the dialog and the confirm button by role/name; consider storing the dialog locator internally (and maybe parameterizing the confirm button label) so the helper is less brittle to UI text changes and avoids redundant queries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The step definition `When User Clicks on Actions button and Selects Delete option from the drop down` is implemented in both `sbom-explorer.step.ts` and `advisory-explorer.step.ts` with identical behavior; consider extracting this into a shared helper or a single step definition to avoid duplication and potential ambiguity.
- The logic for filtering and asserting an empty table after deletion is repeated in multiple step files for SBOMs and advisories; you could centralize this into a reusable helper (e.g., `assertEntityNotPresentInTable`) to keep the tests DRY and easier to maintain.
- In `ConfirmDialog`, you repeatedly look up the dialog and the confirm button by role/name; consider storing the dialog locator internally (and maybe parameterizing the confirm button label) so the helper is less brittle to UI text changes and avoids redundant queries.
## Individual Comments
### Comment 1
<location> `e2e/tests/ui/features/@sbom-search/sbom-search.step.ts:74-71` </location>
<code_context>
},
);
+
+When(
+ "User Selects Delete option from the toggle option from Advisory List Page",
+ async ({ page }) => {
+ const firstRow = page.locator("table tbody tr").first();
+ const kebabToggle = firstRow.getByRole("button", { name: "Kebab toggle" });
+ await kebabToggle.click();
+ await page.getByRole("menuitem", { name: "Delete" }).click();
+ },
+);
+
</code_context>
<issue_to_address>
**issue (testing):** SBOM delete step should target the specific SBOM under test instead of the first row.
Because this always deletes the first row, parameterized scenarios can end up deleting a different SBOM than the one under test, leading to misleading passes/failures. Instead, have the step locate the row for the scenario’s SBOM (e.g., `getByRole('row', { name: sbomName })` or a `SbomListPage` helper) and trigger Delete from there.
</issue_to_address>
### Comment 2
<location> `.github/chatmodes/playwright-tester.chatmode.md:37` </location>
<code_context>
- Automatically run test with:
```bash
cd $PROJECT_ROOT/e2e
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "run test" to "run tests" for grammatical correctness.
You could also phrase it as "Automatically run tests with:" to better match the plural and improve the sentence flow.
```suggestion
- Automatically run tests with:
```
</issue_to_address>
### Comment 3
<location> `.github/chatmodes/playwright-tester.chatmode.md:42` </location>
<code_context>
- npx playwright test --project='bdd' --trace on -g "scenario name here" --headed
+ npx playwright test --project='bdd' --trace on -g "scenario name here"
```
- In case of test failures, the above command launched HTML server to host the test output Press `Ctrl+C` to stop the server
</code_context>
<issue_to_address>
**suggestion (typo):** Tense, article usage, and punctuation could be improved in this sentence.
Consider rephrasing to: "In case of test failures, the above command launches an HTML server to host the test output. Press `Ctrl+C` to stop the server."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| (page as any).testContext?.generatedLabels || labelList | ||
| : labelList; | ||
| await detailsPage.verifyLabels(labelsToVerify, sbomName); | ||
| }, |
There was a problem hiding this comment.
issue (testing): SBOM delete step should target the specific SBOM under test instead of the first row.
Because this always deletes the first row, parameterized scenarios can end up deleting a different SBOM than the one under test, leading to misleading passes/failures. Instead, have the step locate the row for the scenario’s SBOM (e.g., getByRole('row', { name: sbomName }) or a SbomListPage helper) and trigger Delete from there.
| @@ -37,7 +37,7 @@ mode: 'agent' | |||
| - Automatically run test with: | |||
There was a problem hiding this comment.
suggestion (typo): Consider changing "run test" to "run tests" for grammatical correctness.
You could also phrase it as "Automatically run tests with:" to better match the plural and improve the sentence flow.
| - Automatically run test with: | |
| - Automatically run tests with: |
| npx playwright test --project='bdd' --trace on -g "scenario name here" --headed | ||
| npx playwright test --project='bdd' --trace on -g "scenario name here" | ||
| ``` | ||
| - In case of test failures, the above command launched HTML server to host the test output Press `Ctrl+C` to stop the server |
There was a problem hiding this comment.
suggestion (typo): Tense, article usage, and punctuation could be improved in this sentence.
Consider rephrasing to: "In case of test failures, the above command launches an HTML server to host the test output. Press Ctrl+C to stop the server."
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #875 +/- ##
==========================================
- Coverage 62.27% 61.45% -0.83%
==========================================
Files 176 176
Lines 3144 3144
Branches 717 717
==========================================
- Hits 1958 1932 -26
- Misses 916 952 +36
+ Partials 270 260 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7e62fe5 to
59555d5
Compare
carlosthe19916
left a comment
There was a problem hiding this comment.
Thanks for the PR. I have added some comments below. I only added comments to the Advisory section but the same should be applicable to the sboms section
e2e/tests/ui/features/@advisory-explorer/advisory-explorer.step.ts
Outdated
Show resolved
Hide resolved
e2e/tests/ui/features/@advisory-explorer/advisory-explorer.step.ts
Outdated
Show resolved
Hide resolved
e2e/tests/ui/features/@advisory-explorer/advisory-explorer.step.ts
Outdated
Show resolved
Hide resolved
e2e/tests/ui/features/@advisory-explorer/advisory-explorer.step.ts
Outdated
Show resolved
Hide resolved
| await expect( | ||
| page.getByRole("heading", { level: 1, name: "Advisories" }), | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
This can be replaced by:
const navigation = await Navigation.build(page);
await navigation.goToSidebar("Advisories");There was a problem hiding this comment.
This step verifies that the application automatically navigated to the “Advisories” screen by confirming that the header is visible
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.> Assisted-by: Cursor
mrrajan
left a comment
There was a problem hiding this comment.
Thanks @carlosthe19916 Except one, I have committed other changes. Please let me know your thoughts.
| await expect( | ||
| page.getByRole("heading", { level: 1, name: "Advisories" }), | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
This step verifies that the application automatically navigated to the “Advisories” screen by confirming that the header is visible
carlosthe19916
left a comment
There was a problem hiding this comment.
Thanks for the nice enhancements @mrrajan ! LGTM!
|
/backport |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release/0.4.z
git worktree add -d .worktree/backport-875-to-release/0.4.z origin/release/0.4.z
cd .worktree/backport-875-to-release/0.4.z
git switch --create backport-875-to-release/0.4.z
git cherry-pick -x cbe30967323f9caecf9ffafda8f99496b7b7b77a |
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
This PR includes,