tests: make Toolbar typesafe #822
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
Signed-off-by: Carlos Feria <[email protected]>
Contributor
Reviewer's GuideThis PR refactors the Toolbar helper to be fully type-safe by parameterizing its available filters at build time, introducing a mapping from filter keys to value types, adding type-guard functions, unifying all filter application into a single generic applyFilter method, and updating all page objects and tests to supply filter schemas and use the new API. 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 there - I've reviewed your changes - here's some feedback:
- Consider making the
filtersargument required (remove the default empty object) so you can’t accidentally build a Toolbar with no filter definitions. - In
applyFilter, add a finalelseordefaultcase that throws when no filter type matches to avoid silently dropping invalid filter/value calls. - Enhance the type-guard functions to verify the runtime shape of the filter values (e.g. check
dateRangehasfromandto,multiSelectis an array) so misconfigurations fail fast.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the `filters` argument required (remove the default empty object) so you can’t accidentally build a Toolbar with no filter definitions.
- In `applyFilter`, add a final `else` or `default` case that throws when no filter type matches to avoid silently dropping invalid filter/value calls.
- Enhance the type-guard functions to verify the runtime shape of the filter values (e.g. check `dateRange` has `from` and `to`, `multiSelect` is an array) so misconfigurations fail fast.
## Individual Comments
### Comment 1
<location> `e2e/tests/ui/pages/package-list/filter.spec.ts:38` </location>
<code_context>
// Architecture
- await toolbar.applyMultiSelectFilter("Architecture", ["S390", "No Arch"]);
+ await toolbar.applyFilter({ Architecture: ["S390", "No Arch"] });
await table.waitUntilDataIsLoaded();
await table.verifyTableHasNoData();
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for applying multiple filters simultaneously.
Add a test that applies multiple filters together to verify compound filtering works as expected.
</issue_to_address>
### Comment 2
<location> `e2e/tests/ui/pages/vulnerability-list/filter.spec.ts:24` </location>
<code_context>
// Severity filter
- await toolbar.applyMultiSelectFilter("CVSS", ["Unknown", "Medium"]);
+ await toolbar.applyFilter({ CVSS: ["Unknown", "Medium"] });
await table.waitUntilDataIsLoaded();
await table.verifyColumnContainsText("ID", "CVE-2024-26308");
</code_context>
<issue_to_address>
**suggestion (testing):** No test for empty filter application.
Add a test for `applyFilter({})` to ensure the Toolbar handles empty filters without errors or unintended behavior.
Suggested implementation:
```typescript
// Severity filter
// Empty filter application
await toolbar.applyFilter({});
await table.waitUntilDataIsLoaded();
// Optionally, verify that the table is loaded and contains expected data or is not empty
// For example, check that the table has at least one row
expect(await table.getRowCount()).toBeGreaterThan(0);
// Severity filter
await toolbar.applyFilter({ CVSS: ["Unknown", "Medium"] });
await table.waitUntilDataIsLoaded();
await table.verifyColumnContainsText("ID", "CVE-2024-26308");
```
- If `table.getRowCount()` is not available, replace it with the appropriate method to check that the table is loaded and not empty.
- Adjust the assertion to match your application's expected behavior after applying an empty filter (e.g., all rows visible, no errors).
</issue_to_address>
### Comment 3
<location> `e2e/tests/ui/pages/advisory-list/filter.spec.ts:31` </location>
<code_context>
// Labels filter
- await toolbar.applyLabelsFilter("Label", ["type=cve"]);
+ await toolbar.applyFilter({ Label: ["type=cve"] });
await table.waitUntilDataIsLoaded();
await table.verifyColumnContainsText("ID", "CVE-2024-26308");
</code_context>
<issue_to_address>
**suggestion (testing):** No test for filter value edge cases (e.g., empty string, empty array).
Please add tests for filters with empty strings and empty arrays to verify correct UI behavior.
Suggested implementation:
```typescript
// Labels filter
// Edge case: empty string
await toolbar.applyFilter({ Label: [""] });
await table.waitUntilDataIsLoaded();
// Expectation: table should show all data or no data depending on filter logic
// Example: verify that at least one row is present (adjust as needed)
await table.verifyTableHasRows();
// Edge case: empty array
await toolbar.applyFilter({ Label: [] });
await table.waitUntilDataIsLoaded();
// Expectation: table should show all data or no data depending on filter logic
// Example: verify that at least one row is present (adjust as needed)
await table.verifyTableHasRows();
```
- Adjust the expectation in `verifyTableHasRows()` to match your application's intended behavior for empty filters. If the table should be empty, use a method like `verifyTableIsEmpty()` instead.
- If your test framework has a more specific way to check for "no filter applied" or "empty filter", use that instead of the generic row check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
carlosthe19916
added
the
backport release/0.4.z
mrrajan
requested changes
Nov 5, 2025
Contributor
mrrajan
left a comment
mrrajan
left a comment
There was a problem hiding this comment.
@carlosthe19916 While running the tests locally, there are couple of errors occurred for CVSS score and severity which is due to known issue. But the sorting validation on SBOM list page and package details page - ui/pages/sbom-list/sort.spec.ts:13 and ui/pages/sbom-details/packages/sort.spec.ts:13 failed. Even though these errors not related to these changes, just highlighting that the sorting has been changed on the UI.
I am running against upstream release/0.4.z branch.
Collaborator
Author
Yeah, I don't think we should merge this PR until the e2e tests are back to healthy, I would like to avoid adding more commits and take the risk, let's wait. On the other hand, this PR is targeting the main branch therefore it should be tested against it not against |
Signed-off-by: Carlos Feria <[email protected]>
carlosthe19916
removed
the
backport release/0.4.z
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #822 +/- ##
==========================================
- Coverage 62.46% 61.59% -0.88%
==========================================
Files 170 170
Lines 3080 3080
Branches 698 698
==========================================
- Hits 1924 1897 -27
- Misses 892 927 +35
+ Partials 264 256 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
This PR enhances the Toolbar definition so we define the available filters at the time of instantiating the Toolbar, then when we apply filters we can only use the filters defined at the beginning. We enhance type safety and use the power of Typescript.
The following is how a toolbar will be instantiated after this PR:
Note: the current e2e tests are failing due to a backend issue that does not allow uploading parallel files, the backend engineers are working on fixing it. When we review this PR we can focus on the code and it won't be merged until the e2e tests are healthy again