tests: add tests to sbom/advisory upload page #892
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
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Contributor
Reviewer's GuideAdds reusable Playwright page objects, matchers, and helpers for testing SBOM and advisory file uploads, wires aria-labels into the shared UploadFiles React component so it can be targeted in tests, and introduces end-to-end coverage for success, error, removal, and invalid-file flows on both upload pages. Sequence diagram for SBOM upload E2E test using FileUpload page objectsequenceDiagram
actor Tester
participant PlaywrightTest as Playwright_test
participant SBOMUploadPage as SBOM_upload_page
participant FileUpload as File_upload_page_object
participant Browser as Browser_UI
participant UploadFiles as UploadFiles_component
participant Backend as Backend_API
Tester->>PlaywrightTest: start_Sbom_upload_test()
PlaywrightTest->>SBOMUploadPage: new_SBOMUploadPage(page)
PlaywrightTest->>SBOMUploadPage: goto()
SBOMUploadPage->>Browser: navigate_to_sbom_upload_url()
PlaywrightTest->>FileUpload: new_FileUpload(page, ariaLabel_sbom_uploader)
note over FileUpload,Browser: FileUpload locates uploader via aria-label sbom-uploader
PlaywrightTest->>FileUpload: uploadFiles([sbom1.json, sbom2.json])
FileUpload->>Browser: select_files_in_dropzone()
Browser->>UploadFiles: onFileDrop(files)
UploadFiles->>Backend: POST_upload_requests(files)
Backend-->>UploadFiles: responses_per_file
UploadFiles-->>Browser: update_status_items()
PlaywrightTest->>FileUpload: waitForUploadComplete()
FileUpload->>Browser: wait_for_status_items_complete()
PlaywrightTest->>FileUpload: assertUploadSummary(2)
FileUpload->>Browser: read_summary_counts()
Browser-->>FileUpload: summary_data
FileUpload-->>PlaywrightTest: summary_matches
PlaywrightTest->>FileUpload: assertFileStatus(sbom1.json, success)
FileUpload->>Browser: read_status_for_file(sbom1.json)
Browser-->>FileUpload: status_success
PlaywrightTest->>FileUpload: assertFileStatus(sbom2.json, success)
FileUpload->>Browser: read_status_for_file(sbom2.json)
Browser-->>FileUpload: status_success
PlaywrightTest-->>Tester: report_test_passed()
Class diagram for updated UploadFiles component and new file upload test utilitiesclassDiagram
class UploadFiles {
+uploads File[]
+handleUpload(file File) Promise~void~
+handleRemoveUpload(file File) void
+extractSuccessMessage(response AxiosResponse) string
+extractErrorMessage(error AxiosError) string
+props MultipleFileUploadProps
}
class SbomUpload {
+uploads File[]
+handleUpload(file File) Promise~void~
+handleRemoveUpload(file File) void
}
class AdvisoryUpload {
+uploads File[]
+handleUpload(file File) Promise~void~
+handleRemoveUpload(file File) void
}
class MultipleFileUploadProps {
+ariaLabel string
+otherProps any
}
class FileUpload {
+page Page
+rootLocator Locator
+uploadFiles(files string[]) Promise~void~
+removeFile(fileName string) Promise~void~
+waitForUploadComplete() Promise~void~
+assertUploadSummary(expectedCount number) Promise~void~
+assertFileStatus(fileName string, status string) Promise~void~
}
class SBOMUploadPage {
+page Page
+fileUpload FileUpload
+goto() Promise~void~
}
class AdvisoryUploadPage {
+page Page
+fileUpload FileUpload
+goto() Promise~void~
}
class UploadTestHelpers {
+uploadValidFilesSequential(pageObject FileUpload, filePaths string[]) Promise~void~
+uploadValidFilesParallel(pageObject FileUpload, filePaths string[]) Promise~void~
+uploadInvalidFile(pageObject FileUpload, filePath string) Promise~void~
+removeUploadedFile(pageObject FileUpload, fileName string) Promise~void~
}
class FileUploadMatchers {
+toHaveUploadSummary(fileUpload FileUpload, expectedCount number) Promise~void~
+toHaveFileStatus(fileUpload FileUpload, fileName string, status string) Promise~void~
}
SbomUpload --> UploadFiles : uses
AdvisoryUpload --> UploadFiles : uses
MultipleFileUploadProps <|-- UploadFiles : receives_subset
SBOMUploadPage *-- FileUpload : composes
AdvisoryUploadPage *-- FileUpload : composes
UploadTestHelpers --> FileUpload : operates_on
FileUploadMatchers --> FileUpload : asserts_on
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
43 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #892 +/- ##
==========================================
+ Coverage 63.65% 64.30% +0.65%
==========================================
Files 184 187 +3
Lines 3260 3295 +35
Branches 742 746 +4
==========================================
+ Hits 2075 2119 +44
+ Misses 893 881 -12
- Partials 292 295 +3 ☔ View full report in Codecov by Sentry. 🚀 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 1 issue, and left some high level feedback:
- The
UploadFilescomponent’spropsprop is quite generically named and shadows the React convention; consider renaming it to something more explicit likeuploaderPropsormultipleFileUploadPropsto make call sites clearer. - The custom matchers in
FileUploadMatchersreach into theFileUploadpage object’s_uploaderfield; exposing dedicated methods or accessors onFileUploadfor the needed locators would keep the page-object abstraction cleaner and avoid relying on its internal structure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `UploadFiles` component’s `props` prop is quite generically named and shadows the React convention; consider renaming it to something more explicit like `uploaderProps` or `multipleFileUploadProps` to make call sites clearer.
- The custom matchers in `FileUploadMatchers` reach into the `FileUpload` page object’s `_uploader` field; exposing dedicated methods or accessors on `FileUpload` for the needed locators would keep the page-object abstraction cleaner and avoid relying on its internal structure.
## Individual Comments
### Comment 1
<location> `e2e/tests/ui/pages/common/upload-test-helpers.ts:52-61` </location>
<code_context>
+ }
+ });
+
+export const testUploadFilesSequentially = (
+ testName: string,
+ {
+ files,
+ getConfig,
+ }: {
+ files: {
+ path: string;
+ status: "success" | "danger";
+ }[];
+ getConfig: ({ page }: { page: Page }) => Promise<UploadTestConfig>;
+ },
+) =>
+ test(`Upload sequentially- ${testName}`, async ({ page }) => {
+ const config = await getConfig({ page });
+ const fileUploader = config.fileUploader;
+
+ let successfulFilesCount = 0;
+ for (let index = 0; index < files.length; index++) {
+ const file = files[index];
+ if (file.status === "success") {
+ successfulFilesCount++;
+ }
+
+ // Upload file
+ await fileUploader.uploadFiles([file.path]);
+
+ // Summary status
+ if (successfulFilesCount > 0) {
+ await expect(fileUploader).toHaveSummaryUploadStatus({
+ totalFiles: index + 1,
</code_context>
<issue_to_address>
**suggestion (testing):** Sequential upload helper doesn't assert summary status when there are no successful files yet
In `testUploadFilesSequentially`, the summary is only asserted when `successfulFilesCount > 0`, so we never validate the summary when all processed files have failed so far. Please also cover the `successfulFilesCount === 0` case (e.g., assert `0 of X files uploaded` or that the summary is hidden, depending on the intended UX) to make this behavior explicit in the test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
carlosthe19916
force-pushed
the
tests/upload-pages
branch
from
fd290e9 to
c35edbb
Compare
carlosthe19916
requested review from
CryptoRodeo,
mrrajan,
queria and
stanislavsemeniuk
mrrajan
requested changes
Jan 28, 2026
Contributor
mrrajan
left a comment
mrrajan
left a comment
There was a problem hiding this comment.
Thanks for the PR @carlosthe19916 I have requested some changes and added a suggestion for AdvisoryUploadPage.ts and SBOMUploadPage.ts files.
| ): Promise<MatcherResult> => { | ||
| try { | ||
| await baseExpect( | ||
| fileUpload._uploader.locator( |
Contributor
There was a problem hiding this comment.
Private field _uploader from FileUpload class directly accessed here. Either Please make it public or create and access it with getUploader() method.
Contributor
There was a problem hiding this comment.
Instead of having separate Upload Page class for advisory and sbom, shall we leverage FileUpload.ts file with the methods buildFromBrowserPath and getFileUploader parameterized with endpoints and locator?
| await login(page); | ||
| }); | ||
|
|
||
| testUploadFilesSequentially("CSAF file (.bz2)", { |
Contributor
There was a problem hiding this comment.
Change CSAF file to SBOM file
| testUploadFilesSequentially, | ||
| } from "../common/upload-test-helpers"; | ||
| import { SBOMUploadPage } from "./SBOMUploadPage"; | ||
|
|
Contributor
There was a problem hiding this comment.
Hardcoded test data file path can be extracted to constants and referenced for the directory path.
Suggested change
| const TEST_FILES = { | |
| QUARKUS_BOM: path.join(__dirname, "../../../common/dataset/sbom/quarkus-bom-2.13.8.Final-redhat-00004.json.bz2"), | |
| UBI9_MINIMAL: path.join(__dirname, "../../../common/dataset/sbom/ubi9-minimal-9.3-1361.json.bz2"), | |
| INVALID_JSON: path.join(__dirname, "../../../common/assets/invalid-file.json"), | |
| INVALID_TXT: path.join(__dirname, "../../../common/assets/invalid-file.txt"), | |
| }; |
| testUploadFilesSequentially, | ||
| } from "../common/upload-test-helpers"; | ||
| import { AdvisoryUploadPage } from "./AdvisoryUploadPage"; | ||
|
|
Contributor
There was a problem hiding this comment.
Same as SBOM file path suggestion. Export the directory path into constants.
| statusItem.locator(`.pf-v6-c-progress.pf-m-${expectedStatus.status}`), | ||
| ).toBeVisible(); | ||
| await baseExpect( | ||
| statusItem.locator(".pf-v6-c-progress__status", { hasText: "100%" }), |
Contributor
There was a problem hiding this comment.
Shall we move the locators of success | danger and upload percent elemet to page objects and use get methods for assertion?
For example, add getUploadProgressStatus method in FileUpload.ts as,
async getUploadProgressStatus(fileName: string) {
const item = await this.getUploadStatusItem(fileName);
return item.locator(".pf-v6-c-progress__status");
}
Similarly,
async getUploadProgressBar(fileName: string, expectedStatus: string) {
const item = await this.getUploadStatusItem(fileName);
return item.locator(`.pf-v6-c-progress.pf-m-${expectedStatus.status}`);
}
And call them for assertion as,
await baseExpect(
await fileUpload.getUploadProgressBar(expectedStatus.fileName)
).toBeVisible();
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.
Part of #889 (Phase 1)
FileUploadMatchersandFileUploadreusable componentsSummary by Sourcery
Add reusable file upload page objects and assertions and use them to introduce end-to-end coverage for SBOM and advisory upload flows.
New Features:
Enhancements:
Tests:
Chores: