Skip to content
This repository was archived by the owner on Sep 11, 2025. It is now read-only.

Comments

UI: Add tests for SBOM Upload page#55

Open
queria wants to merge 1 commit intotrustification:mainfrom
queria:upload-sbom
Open

UI: Add tests for SBOM Upload page#55
queria wants to merge 1 commit intotrustification:mainfrom
queria:upload-sbom

Conversation

@queria
Copy link
Collaborator

@queria queria commented May 19, 2025

Adds tests for Uploading SBOM file using UI.

TC-2340

Adds feature file and step implementation
for testing of page for Uploading SBOM.

TC-2340
Copy link
Contributor

@vobratil vobratil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some suggestions, most of them not really critical. But I would de-duplicated the function that verifies if a tab is selected and I'm not sure about keeping the SBOMs in a different location.

const TIMEOUT_PAGELOAD_IMPORT = 2_000;
const TIMEOUT_UPLOAD_DONE = 90_000;

const SBOM_SET = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about keeping these SBOMs out of the tests/common/assets/sbom folder. I actually prefer it your way, but I'm afraid that if we start doing this, eventually everyone will be doing it their own way and it's going to introduce chaos into the project structure. If we keep all of the SBOMs in one place, it's easier to keep track and do mass operations over them. Would be nice to get other people's opinion here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason i went with placing them next to test and not in common/assets ... as they are not 'common', they should NOT be uploaded e.g. during initial ingestion or such - they are specific meant only for this test of Upload functionality.

await page.goto("/importers"); // dont care which page here, importers is lot faster than Dashboard
await page.getByRole("link", { name: MENU_UPLOAD }).click();

const header = page.locator(`xpath=(//h1[text()="${HEADER_UPLOAD}"])`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a soft suggestion, but in general I would try not to rely on xpaths and CSS selectors if not necessary. You can let Playwright do the work, e.g. page.getByRole("heading"). That way we don't have to worry about paths and selectors changing as much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look into this, thanks!

};

const visitUploadPage = async ({ page }) => {
await page.goto("/importers"); // dont care which page here, importers is lot faster than Dashboard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary, actually? Isn't the side menu always visible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Menu on the left is not visible if there is no page loaded at start ... which seems there is not if auth is not enabled (so e.g. in dev/local or ci or such).

await page.getByRole("link", { name: MENU_UPLOAD }).click();

const header = page.locator(`xpath=(//h1[text()="${HEADER_UPLOAD}"])`);
await header.waitFor({ state: "visible", timeout: TIMEOUT_PAGELOAD_IMPORT });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I understand this - is this a timeout for loading the page? Why specifically change it here?


// PAGE CONTENT

Then("SBOM upload tab is selected", async ({ page }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this function here. I'd prefer not duplicating these, if possible.

async verifyTabIsSelected(tabName: string) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, i've missed it will switch to it.

When User uploads "single SBOM"
Then Total uploaded count is shown for "single SBOM"
And Results of uploading "single SBOM" are visible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your consideration - should we also check if the SBOM is present in the DB? And should we check for both the file and the DB record, as they are two separate things? I think the former would be much harder to check for in a deployed instance, though, so maybe it's out of scope for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i would consider these out of scope as these checks would need access to and knowledge of internal structures.
What could be better fit here is confirming upload/ingestion by search/viewing details of the sboms e.g. the SBOM Explorer/Details view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should have the steps for that implemented, so that should be easy to do.


// FILE UPLOAD

When("User uploads {string}", async ({ page }, data_set_key) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another soft suggestion - it would be easier to read without having to scroll up if instead of data_set_key the parameter was called something like option or sbom_subset.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants