SBOM Deletion Performance Tests #714
Open
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
Contributor
Reviewer's GuideAdd timing instrumentation to HTTP fixtures and introduce dedicated SBOM deletion performance tests, supported by new helper modules, refactored setup, and updated docs/config for performance test execution and report storage. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
queria
reviewed
Sep 9, 2025
vobratil
force-pushed
the
delete-perf-test
branch
2 times, most recently
from
7cde8e0 to
d5632e7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
=======================================
Coverage 58.72% 58.72%
=======================================
Files 163 163
Lines 2871 2871
Branches 653 653
=======================================
Hits 1686 1686
Misses 941 941
Partials 244 244 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vobratil
force-pushed
the
delete-perf-test
branch
5 times, most recently
from
a3f288e to
bc7db77
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Ensure the REPORT_DIR exists (e.g. mkdirSync) before writing performance CSVs to avoid errors when the directory is missing.
- Consider moving the AxiosRequestConfig/AxiosResponse type augmentations into a dedicated .d.ts file instead of inlining them in fixtures.ts for better type organization.
- The sequential SBOM deletion test is currently skipped—either enable it or add a comment explaining why it's excluded so it doesn’t get overlooked.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure the REPORT_DIR exists (e.g. mkdirSync) before writing performance CSVs to avoid errors when the directory is missing.
- Consider moving the AxiosRequestConfig/AxiosResponse type augmentations into a dedicated .d.ts file instead of inlining them in fixtures.ts for better type organization.
- The sequential SBOM deletion test is currently skipped—either enable it or add a comment explaining why it's excluded so it doesn’t get overlooked.
## Individual Comments
### Comment 1
<location> `e2e/tests/api/features/performance.ts:47` </location>
<code_context>
+ logger.info(`Uploaded ${sbomIds.length} SBOMs.`);
+});
+
+test.skip("@performance Delete / All / Sequential", async ({ axios }) => {
+ const currentTimeStamp = Date.now();
+ const reportFile = `${REPORT_FILE_PREFIX}sequential-${currentTimeStamp}.csv`;
</code_context>
<issue_to_address>
**question (testing):** The sequential deletion performance test is skipped.
If the test is intentionally skipped, please document the reason in the code or remove it to prevent confusion. Otherwise, enable the test to ensure both deletion methods are properly evaluated.
</issue_to_address>
### Comment 2
<location> `e2e/tests/api/features/performance.ts:35-45` </location>
<code_context>
+test.beforeEach(async ({ axios }) => {
+ logger.info("Uploading SBOMs before deletion performance tests.");
+
+ var uploads = await uploadSboms(axios, SBOM_FILES, SBOM_DIR);
+
+ uploads.forEach((upload) => sbomIds.push(upload.id));
+
+ sbomIds.forEach((id) => logger.info(id));
</code_context>
<issue_to_address>
**suggestion (testing):** No assertion on successful upload of SBOMs before deletion.
Add assertions to verify SBOM uploads succeed and IDs are valid before measuring deletion performance.
```suggestion
import { expect } from "@playwright/test";
test.beforeEach(async ({ axios }) => {
logger.info("Uploading SBOMs before deletion performance tests.");
var uploads = await uploadSboms(axios, SBOM_FILES, SBOM_DIR);
// Assert that uploads array is not empty and matches SBOM_FILES length
expect(uploads.length, "Number of uploaded SBOMs should match input files").toBe(SBOM_FILES.length);
uploads.forEach((upload, idx) => {
// Assert each upload has a valid id
expect(upload).toHaveProperty("id");
expect(typeof upload.id).toBe("string");
expect(upload.id.length, `Upload at index ${idx} should have a non-empty id`).toBeGreaterThan(0);
sbomIds.push(upload.id);
});
sbomIds.forEach((id) => logger.info(id));
logger.info(`Uploaded ${sbomIds.length} SBOMs.`);
});
```
</issue_to_address>
### Comment 3
<location> `e2e/tests/api/features/performance.ts:78-96` </location>
<code_context>
+
+ writeRequestDurationToFile(reportFile, "No.", "SBOM ID", "Duration [ms]");
+
+ const deletionPromises = sbomIds.map(async (sbomId) => {
+ const deletePromise = axios
+ .delete(`/api/v2/sbom/${sbomId}`)
+ .then((response) =>
+ writeRequestDurationToFile(
+ reportFile,
+ "n/a",
+ response.data.id,
+ String(response.duration),
+ ),
+ )
+ .catch((error) => {
+ logger.error(`SBOM with ID ${sbomId} could not be deleted.`, error);
+ });
+
+ return deletePromise;
+ });
+
+ await Promise.all(deletionPromises);
+});
+
</code_context>
<issue_to_address>
**suggestion (testing):** No assertion on successful deletion of SBOMs during parallel test.
Add assertions or a final check to verify all SBOMs were deleted, and explicitly report any failures.
```suggestion
const deletionResults: { sbomId: string; success: boolean; error?: any }[] = [];
const deletionPromises = sbomIds.map(async (sbomId) => {
try {
const response = await axios.delete(`/api/v2/sbom/${sbomId}`);
writeRequestDurationToFile(
reportFile,
"n/a",
response.data.id,
String(response.duration),
);
deletionResults.push({ sbomId, success: true });
} catch (error) {
logger.error(`SBOM with ID ${sbomId} could not be deleted.`, error);
deletionResults.push({ sbomId, success: false, error });
}
});
await Promise.all(deletionPromises);
// Assert all deletions were successful
const failedDeletions = deletionResults.filter(result => !result.success);
if (failedDeletions.length > 0) {
const failedIds = failedDeletions.map(result => result.sbomId).join(", ");
throw new Error(`Failed to delete SBOMs with IDs: ${failedIds}`);
}
```
</issue_to_address>
### Comment 4
<location> `e2e/tests/api/features/performance.ts:100-109` </location>
<code_context>
+});
+
+// Re-try deletion of all SBOMs in case some of the SBOMs didn't get deleted during the tests.
+test.afterEach(async ({ axios }) => {
+ logger.info("Cleaning up SBOMs after deletion performance tests.");
+
+ await deleteSboms(axios, sbomIds).then((success) => {
+ if (success) {
+ logger.info("All SBOMs were deleted successfully.");
+ } else {
+ logger.warn(
+ "One or more SBOMs could not be deleted. Check the logs and/or consider deleting the SBOMs manually.",
+ );
+ }
+ });
+
+ sbomIds = [];
+});
</code_context>
<issue_to_address>
**suggestion:** Cleanup logic does not handle partial failures robustly.
Currently, failures are only logged as a warning without identifying which SBOMs failed or attempting retries. Please update the cleanup to retry failed deletions and log the IDs of SBOMs that could not be deleted.
Suggested implementation:
```typescript
test.afterEach(async ({ axios }) => {
logger.info("Cleaning up SBOMs after deletion performance tests.");
const MAX_RETRIES = 3;
let remainingSbomIds = [...sbomIds];
let failedSbomIds: string[] = [];
for (let attempt = 1; attempt <= MAX_RETRIES && remainingSbomIds.length > 0; attempt++) {
logger.info(`SBOM cleanup attempt ${attempt} for IDs: ${remainingSbomIds.join(", ")}`);
// deleteSboms should return an array of IDs that failed deletion
failedSbomIds = await deleteSboms(axios, remainingSbomIds);
if (failedSbomIds.length === 0) {
logger.info("All SBOMs were deleted successfully.");
break;
} else {
logger.warn(
`Attempt ${attempt}: Failed to delete SBOMs with IDs: ${failedSbomIds.join(", ")}`
);
remainingSbomIds = failedSbomIds;
}
}
if (failedSbomIds.length > 0) {
logger.error(
`After ${MAX_RETRIES} attempts, the following SBOMs could not be deleted: ${failedSbomIds.join(", ")}. Please check the logs and/or consider deleting the SBOMs manually.`
);
}
sbomIds = [];
});
```
You must update the `deleteSboms` function so that it returns an array of SBOM IDs that failed deletion, rather than a boolean. If it currently returns a boolean, refactor it to collect and return the failed IDs.
</issue_to_address>
### Comment 5
<location> `e2e/tests/api/features/performance.ts:31` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 6
<location> `e2e/tests/api/features/performance.ts:38` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 7
<location> `e2e/tests/api/features/performance.ts:50` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 8
<location> `e2e/tests/api/features/performance.ts:52` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 9
<location> `e2e/tests/api/features/performance.ts:79-93` </location>
<code_context>
const deletePromise = axios
.delete(`/api/v2/sbom/${sbomId}`)
.then((response) =>
writeRequestDurationToFile(
reportFile,
"n/a",
response.data.id,
String(response.duration),
),
)
.catch((error) => {
logger.error(`SBOM with ID ${sbomId} could not be deleted.`, error);
});
return deletePromise;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return axios
.delete(`/api/v2/sbom/${sbomId}`)
.then((response) =>
writeRequestDurationToFile(
reportFile,
"n/a",
response.data.id,
String(response.duration),
),
)
.catch((error) => {
logger.error(`SBOM with ID ${sbomId} could not be deleted.`, error);
});
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 10
<location> `e2e/tests/api/helpers/delete.ts:6` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 11
<location> `e2e/tests/api/helpers/delete.ts:25-29` </location>
<code_context>
const allSuccessful = results.every(
(result) => result.status === "fulfilled" && result.value?.status === 200,
);
return allSuccessful;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return results.every(
(result) => result.status === "fulfilled" && result.value?.status === 200,
);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 12
<location> `e2e/tests/api/helpers/upload.ts:16-20` </location>
<code_context>
const promise = axios.post("/api/v2/sbom", fileStream, {
headers: { "Content-Type": "application/json+bzip2" },
});
return promise;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return axios.post("/api/v2/sbom", fileStream, {
headers: { "Content-Type": "application/json+bzip2" },
});
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</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.
vobratil
changed the title
carlosthe19916
requested changes
Sep 26, 2025
Collaborator
carlosthe19916
left a comment
carlosthe19916
left a comment
There was a problem hiding this comment.
@vobratil sorry for my late review. It took me some time to do it. Please read my comments below.
It is a nice addition of tests, well done. Just some minor code enhancements I am suggesting.
Let me know if you need me to expand on any of the points mentioned in my review
e2e/tests/api/helpers/upload.ts
Outdated
Comment on lines
23
to
24
| const responses = await Promise.all(uploads); | ||
| return responses.map((response) => response.data); |
Collaborator
There was a problem hiding this comment.
Being this a helper function, it is better for the helper not to hide info; When we do .map we are making the helper to hide the rest of parts of the response. Let the caller of this function deal with mapping and let the helper clean without mapping anything.
e2e/tests/api/helpers/upload.ts
Outdated
Comment on lines
8
to
9
| files: string[], | ||
| sbomDirPath: string, |
Collaborator
There was a problem hiding this comment.
Could we swap the order of these params? It is more readable if we do uploadSbom(axios, dir, file) since we read left to right the the directory path params are also defined left to right.
| "quay-v3.14.0-product.json.bz2", | ||
| ]; | ||
|
|
||
| var sbomIds: string[] = []; |
Collaborator
There was a problem hiding this comment.
Generally speaking let is preferred over var.
But The fact that we are using var here means that this variable is being modified somewhere which is dangerous, global variables should never be modified otherwise functions or code using that variable has a big risk of misusing it or having unexpected results
| String(response.duration), | ||
| ), | ||
| ) | ||
| .catch((error) => { |
Collaborator
There was a problem hiding this comment.
We could also record the failure as part of the report and not just log
Comment on lines
+8
to
+15
| for (const sbomId of sbomIds) { | ||
| try { | ||
| await axios.get(`/api/v2/sbom/${sbomId}`); | ||
| existingSbomIds.push(sbomId); | ||
| } catch (_error) { | ||
| logger.info(`SBOM with ID ${sbomId} does not exist anymore. Skipping.`); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
I think this is not necessary... since it is only cleaning we can call directly axios.delete, if the sbom does not exist, it will fail and nothing happens
e2e/tests/api/helpers/delete.ts
Outdated
Comment on lines
25
to
27
| const allSuccessful = results.every( | ||
| (result) => result.status === "fulfilled" && result.value?.status === 200, | ||
| ); |
Collaborator
There was a problem hiding this comment.
these block of code should be written in the caller of the function deleteSboms not inside.
| CURRENT_LOG_LEVEL >= LOG_LEVELS.error && console.error("[ERROR]", ...args); | ||
| }, | ||
| }; | ||
| export const REPORT_DIR = process.env.REPORT_DIR ?? "test-results/"; |
Collaborator
There was a problem hiding this comment.
btw, isn't it better not to have ENVs here? usually the directory names for outputs in many frameworks or languages are just defined. We do not expect them to change. If it hardcoded then we can safely add that directory to .gitignore, otherwise having a dynamic directory name means we don't know which directory to ignore and we need to ignore by file type like in this PR where we are ignoring csv files.
Maybe, we can use a hardcoded directory trustify-custom-results and add that directory to gitignore. but it is just a suggestion. I trust your judgement on this topic
Signed-off-by: Vilem Obratil <[email protected]>
… that returns SBOM IDs. Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
Signed-off-by: Vilem Obratil <[email protected]>
vobratil
force-pushed
the
delete-perf-test
branch
from
bc7db77 to
f4d2f6b
Compare
Signed-off-by: Vilem Obratil <[email protected]>
vobratil
force-pushed
the
delete-perf-test
branch
from
9db8c29 to
f2e740b
Compare
Signed-off-by: Vilem Obratil <[email protected]>
vobratil
force-pushed
the
delete-perf-test
branch
from
f2e740b to
8f4c632
Compare
Contributor
Author
|
@carlosthe19916 I've applied most of the changes you suggested. Some I've left as they were, because I didn't think they were essential, but mostly I've cleaned up the helpers and removed the error handling where not necessary. If you could give it one more quick look, I would be grateful. |
Collaborator
|
@vobratil thanks for the enhancements, it looks better. I think some important points in my previous review were not addressed, like the error handling. I have created a PR to your PR vobratil#1 to show how I propose errors to be handled. In a nutshell, I do think we should not have tests that always pass and never fail because the errors are caught by try/catch. In CI we only see green or red and we do not read logs. While logs are useful for development it is not used for determining the healthiness of the system. Please look at my PR and merge it at your discretion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
A small set of performance tests that measure the response times on deletion of SBOMs. I also added some helper functions and request/response interceptors that are needed in order to measure the time between the moment request was sent and the moment response was received. The results are saved to a CSV file that should help us visualize how the response times change with more requests and compare pre and post fix behavior.
By default, only the parallel test is enabled.
Summary by Sourcery
Add performance testing for SBOM deletions by instrumenting HTTP fixtures to capture request durations, introducing a dedicated performance test, and updating documentation to explain running performance tests.
Enhancements:
Documentation:
--grep @performancein the e2e READMETests: