Open
Conversation
Contributor
Reviewer's GuideAdds end-to-end Playwright BDD coverage for the CycloneDX SBOM "Download License Report" workflow, including new helpers for triggering and validating downloads, extracting the TAR.GZ license archive, parsing TSV/CSV contents, and asserting package-level license metadata, along with small enhancements to the shared DetailsPage and Helpers utilities. 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 2 issues, and left some high level feedback:
- In
extractLicenseReport, thetar -xzf ${downloadedFilePath} -C ${extractionPath}command is built without quoting or escaping paths, which will break on paths with spaces or special characters; consider using proper shell escaping orspawnwith argument arrays instead ofexecwith string interpolation. - The step definitions rely on shared module-level variables like
downloadedFilename,extractionPath, andselectedPackageRow, which can lead to subtle interference between scenarios (especially under parallel execution); it would be more robust to keep this state in the test context/fixtures or a per-scenario world object. - In
license-export_cdx.step.tsthere is now both direct usage ofclickAndVerifyDownloadand the newdownloadLicenseReporthelper (which in turn usesclickAndDownload); consider standardizing on one approach to avoid duplicated download logic and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `extractLicenseReport`, the `tar -xzf ${downloadedFilePath} -C ${extractionPath}` command is built without quoting or escaping paths, which will break on paths with spaces or special characters; consider using proper shell escaping or `spawn` with argument arrays instead of `exec` with string interpolation.
- The step definitions rely on shared module-level variables like `downloadedFilename`, `extractionPath`, and `selectedPackageRow`, which can lead to subtle interference between scenarios (especially under parallel execution); it would be more robust to keep this state in the test context/fixtures or a per-scenario world object.
- In `license-export_cdx.step.ts` there is now both direct usage of `clickAndVerifyDownload` and the new `downloadLicenseReport` helper (which in turn uses `clickAndDownload`); consider standardizing on one approach to avoid duplicated download logic and make future changes easier.
## Individual Comments
### Comment 1
<location path="e2e/tests/ui/features/@license-export_cdx/license-export_cdx.step.ts" line_range="23-28" />
<code_context>
+
+export const { Given, When, Then } = createBdd(test);
+
+let downloadedFilename: string;
+let downloadedFilePath: string;
+let extractionPath: string;
+let packageLicenseFilePath: string;
+let licenseReferenceFilePath: string;
+let selectedPackageRow: Record<string, string>;
+
+Given(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Shared mutable state between steps can cause flakiness and cross-scenario leakage; consider scoping state per scenario/test.
These `let` variables are effectively global to all steps in this module, so scenarios can interfere with each other (especially under parallel execution or reuse within a worker). Please move this state to a per-scenario context (e.g., Playwright test fixtures or the BDD world/context) or, at minimum, fully reset each variable in `Before`/`After` hooks so no scenario can see stale values from another.
Suggested implementation:
```typescript
export const { Given, When, Then, Before } = createBdd(test);
```
```typescript
let downloadedFilename: string;
let downloadedFilePath: string;
let extractionPath: string;
let packageLicenseFilePath: string;
let licenseReferenceFilePath: string;
let selectedPackageRow: Record<string, string>;
Before(async () => {
downloadedFilename = "";
downloadedFilePath = "";
extractionPath = "";
packageLicenseFilePath = "";
licenseReferenceFilePath = "";
selectedPackageRow = {};
});
Given(
```
</issue_to_address>
### Comment 2
<location path="e2e/tests/ui/pages/LicenseExportHelpers.ts" line_range="26-33" />
<code_context>
+ return downloadPath;
+};
+
+export const extractLicenseReport = async (downloadedFilePath: string): Promise<string> => {
+ const execAsync = promisify(exec);
+ const extractionPath = path.join(path.dirname(downloadedFilePath), "extracted");
+ if (!fs.existsSync(extractionPath)) {
+ fs.mkdirSync(extractionPath);
+ }
+
+ await execAsync(`tar -xzf ${downloadedFilePath} -C ${extractionPath}`);
+ return extractionPath;
+};
</code_context>
<issue_to_address>
**suggestion:** Extraction helper assumes a `tar` binary and paths without spaces; consider hardening and/or adding tests for these edge cases.
This helper shells out to `tar` and interpolates `downloadedFilePath` directly into the command, which will fail if the download/extraction paths contain spaces or shell metacharacters, or if `tar` is missing or incompatible. To keep e2e tests robust, consider switching to a Node tar library, or at least add tests covering paths with spaces and asserting a clear error when `tar` fails.
Suggested implementation:
```typescript
export const extractLicenseReport = async (downloadedFilePath: string): Promise<string> => {
const execFileAsync = promisify(execFile);
const extractionPath = path.join(path.dirname(downloadedFilePath), "extracted");
if (!fs.existsSync(extractionPath)) {
// Use recursive to be robust against nested paths and concurrent tests
fs.mkdirSync(extractionPath, { recursive: true });
}
try {
await execFileAsync("tar", ["-xzf", downloadedFilePath, "-C", extractionPath]);
} catch (error: any) {
const stderr = error?.stderr ?? "";
throw new Error(
`Failed to extract license report with tar: ${stderr || error?.message || "unknown error"}`,
);
}
return extractionPath;
}
```
1. Ensure `execFile` is imported at the top of `e2e/tests/ui/pages/LicenseExportHelpers.ts`:
- Add `execFile` to the existing `child_process` import, e.g.:
- `import { execFile } from "child_process";`
2. Add e2e tests that:
- Verify extraction works when the download path and extraction directory contain spaces.
- Assert that when `tar` fails (e.g., corrupt archive or `tar` missing in PATH, if you can simulate it), the thrown error includes the clear message `"Failed to extract license report with tar"`.
</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
+23
to
+28
| let downloadedFilename: string; | ||
| let downloadedFilePath: string; | ||
| let extractionPath: string; | ||
| let packageLicenseFilePath: string; | ||
| let licenseReferenceFilePath: string; | ||
| let selectedPackageRow: Record<string, string>; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Shared mutable state between steps can cause flakiness and cross-scenario leakage; consider scoping state per scenario/test.
These let variables are effectively global to all steps in this module, so scenarios can interfere with each other (especially under parallel execution or reuse within a worker). Please move this state to a per-scenario context (e.g., Playwright test fixtures or the BDD world/context) or, at minimum, fully reset each variable in Before/After hooks so no scenario can see stale values from another.
Suggested implementation:
export const { Given, When, Then, Before } = createBdd(test);let downloadedFilename: string;
let downloadedFilePath: string;
let extractionPath: string;
let packageLicenseFilePath: string;
let licenseReferenceFilePath: string;
let selectedPackageRow: Record<string, string>;
Before(async () => {
downloadedFilename = "";
downloadedFilePath = "";
extractionPath = "";
packageLicenseFilePath = "";
licenseReferenceFilePath = "";
selectedPackageRow = {};
});
Given(
Comment on lines
+26
to
+33
| export const extractLicenseReport = async (downloadedFilePath: string): Promise<string> => { | ||
| const execAsync = promisify(exec); | ||
| const extractionPath = path.join(path.dirname(downloadedFilePath), "extracted"); | ||
| if (!fs.existsSync(extractionPath)) { | ||
| fs.mkdirSync(extractionPath); | ||
| } | ||
|
|
||
| await execAsync(`tar -xzf ${downloadedFilePath} -C ${extractionPath}`); |
Contributor
There was a problem hiding this comment.
suggestion: Extraction helper assumes a tar binary and paths without spaces; consider hardening and/or adding tests for these edge cases.
This helper shells out to tar and interpolates downloadedFilePath directly into the command, which will fail if the download/extraction paths contain spaces or shell metacharacters, or if tar is missing or incompatible. To keep e2e tests robust, consider switching to a Node tar library, or at least add tests covering paths with spaces and asserting a clear error when tar fails.
Suggested implementation:
export const extractLicenseReport = async (downloadedFilePath: string): Promise<string> => {
const execFileAsync = promisify(execFile);
const extractionPath = path.join(path.dirname(downloadedFilePath), "extracted");
if (!fs.existsSync(extractionPath)) {
// Use recursive to be robust against nested paths and concurrent tests
fs.mkdirSync(extractionPath, { recursive: true });
}
try {
await execFileAsync("tar", ["-xzf", downloadedFilePath, "-C", extractionPath]);
} catch (error: any) {
const stderr = error?.stderr ?? "";
throw new Error(
`Failed to extract license report with tar: ${stderr || error?.message || "unknown error"}`,
);
}
return extractionPath;
}- Ensure
execFileis imported at the top ofe2e/tests/ui/pages/LicenseExportHelpers.ts:- Add
execFileto the existingchild_processimport, e.g.:import { execFile } from "child_process";
- Add
- Add e2e tests that:
- Verify extraction works when the download path and extraction directory contain spaces.
- Assert that when
tarfails (e.g., corrupt archive ortarmissing in PATH, if you can simulate it), the thrown error includes the clear message"Failed to extract license report with tar".
a-oren
force-pushed
the
tests/license-explorer
branch
5 times, most recently
from
864b6d3 to
b886f7e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #942 +/- ##
==========================================
+ Coverage 64.98% 67.18% +2.19%
==========================================
Files 195 218 +23
Lines 3339 3828 +489
Branches 751 873 +122
==========================================
+ Hits 2170 2572 +402
- Misses 872 918 +46
- Partials 297 338 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a-oren
force-pushed
the
tests/license-explorer
branch
from
b886f7e to
26694fa
Compare
a-oren
force-pushed
the
tests/license-explorer
branch
from
26694fa to
03fd60f
Compare
Collaborator
|
@a-oren seems like this PR is passing CI, good job! Please assign the QE engineers as reviewers, otherwise they might not see this PR |
mrrajan
requested changes
Mar 9, 2026
e2e/tests/ui/helpers/DetailsPage.ts
Outdated
| ).toBeVisible(); | ||
| } | ||
|
|
||
| async verifyActionIsVisibleInMenu(actionName: string) { |
Contributor
There was a problem hiding this comment.
I see the verifyActionIsVisibleInMenu now deals only with verification part and user actions is under openActionsMenu. Which is good comparing to, verifyActionIsAvailable. But shall we move the verification part into assertions? This ensures type safety and consistency.
| fs.mkdirSync(extractionPath); | ||
| } | ||
|
|
||
| await execAsync(`tar -xzf ${downloadedFilePath} -C ${extractionPath}`); |
|
|
||
| const savePath = path.join( | ||
| os.tmpdir(), | ||
| `license-report-${Date.now()}-${download.suggestedFilename()}`, |
Contributor
There was a problem hiding this comment.
Suggestion - Sanitize the filepath to replace special characters like "/","" and "."
Author
|
@mrrajan Thanks for the review, I added your recommendation |
a-oren
force-pushed
the
tests/license-explorer
branch
from
31d7378 to
b4b2858
Compare
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
5 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.
test(e2e): add license export tests for CycloneDX SBOMs
Add end-to-end tests that verify the Download License Report workflow
for CycloneDX SBOMs, covering:
SBOM search results page and the SBOM Explorer page
version, purl, cpe, and license columns
Supporting changes:
package row lookup utilities
Summary by Sourcery
Add end-to-end UI tests for downloading and validating CycloneDX SBOM license reports and introduce shared helpers for license report downloads and menu interactions.
New Features:
Enhancements:
Tests: