test(e2e): fix downstream failures for UI tests regarding the Importe… - #1203
test(e2e): fix downstream failures for UI tests regarding the Importe…#1203matejnesuta wants to merge 4 commits into
Conversation
Reviewer's GuideAdds helper utilities to ensure required importer configurations exist via the backend API before running UI importer tests, and wires these helpers into relevant Playwright BDD steps to prevent downstream failures where importers are missing. 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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The logic for resolving base URL, building headers (including the OIDC token) and performing GET/POST requests is duplicated between
ensureImporterExistsandensureAllImportersExist; consider extracting a small helper to centralize this and reduce the chance of future inconsistencies. - Both helpers rely on
page.waitForTimeout(1000)after reloads; it would be more robust to wait for a specific UI condition (e.g., table selector/state) or network idle instead of a fixed delay to reduce flakiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for resolving base URL, building headers (including the OIDC token) and performing GET/POST requests is duplicated between `ensureImporterExists` and `ensureAllImportersExist`; consider extracting a small helper to centralize this and reduce the chance of future inconsistencies.
- Both helpers rely on `page.waitForTimeout(1000)` after reloads; it would be more robust to wait for a specific UI condition (e.g., table selector/state) or network idle instead of a fixed delay to reduce flakiness.
## Individual Comments
### Comment 1
<location path="e2e/tests/ui/helpers/Importer.ts" line_range="123-91" />
<code_context>
+ }
+ }
+
+ if (anyCreated) {
+ await page.reload();
+ await page.waitForTimeout(1000);
+ }
+};
</code_context>
<issue_to_address>
**suggestion (testing):** Replace fixed sleep with a UI-level wait to reduce flakiness
Since this helper uses `page.reload()` followed by `waitForTimeout(1000)`, the test may become flaky under load or on slower environments. Prefer waiting on a specific UI condition (e.g. the importer row appearing or a stable table selector) instead of a fixed delay, so the test only proceeds once the UI is actually ready.
Suggested implementation:
```typescript
if (anyCreated) {
await page.reload();
// Wait for the page to be fully loaded and the importer table to be visible
await Promise.all([
page.waitForLoadState("networkidle"),
page.waitForSelector('[data-testid="importers-table"]', { state: "visible" }),
]);
}
};
```
1. Ensure that the selector `[data-testid="importers-table"]` exists in the importer UI. If your app uses a different data-testid or selector for the importer list/table, update the selector in `waitForSelector` accordingly (e.g. a row selector like `[data-testid="importer-row"]`).
2. If your project prefers a different load state (e.g. `"domcontentloaded"` instead of `"networkidle"`), adjust `waitForLoadState` to match your existing Playwright patterns.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| data: importerConfig, | ||
| }); | ||
| await page.reload(); | ||
| await page.waitForTimeout(1000); |
There was a problem hiding this comment.
suggestion (testing): Replace fixed sleep with a UI-level wait to reduce flakiness
Since this helper uses page.reload() followed by waitForTimeout(1000), the test may become flaky under load or on slower environments. Prefer waiting on a specific UI condition (e.g. the importer row appearing or a stable table selector) instead of a fixed delay, so the test only proceeds once the UI is actually ready.
Suggested implementation:
if (anyCreated) {
await page.reload();
// Wait for the page to be fully loaded and the importer table to be visible
await Promise.all([
page.waitForLoadState("networkidle"),
page.waitForSelector('[data-testid="importers-table"]', { state: "visible" }),
]);
}
};- Ensure that the selector
[data-testid="importers-table"]exists in the importer UI. If your app uses a different data-testid or selector for the importer list/table, update the selector inwaitForSelectoraccordingly (e.g. a row selector like[data-testid="importer-row"]). - If your project prefers a different load state (e.g.
"domcontentloaded"instead of"networkidle"), adjustwaitForLoadStateto match your existing Playwright patterns.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1203 +/- ##
==========================================
+ Coverage 54.19% 54.20% +0.01%
==========================================
Files 255 255
Lines 5715 5715
Branches 1774 1774
==========================================
+ Hits 3097 3098 +1
Misses 2355 2355
+ Partials 263 262 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vobratil
left a comment
vobratil
left a comment
There was a problem hiding this comment.
@matejnesuta I appreciate what you're trying to do here, but I'm not sure I agree with the approach. Currently we have a problem with the importer tests failing, because we don't have the importers set up. But that's often on purpose, since for a lot of testing it's just nicer to have a clean instance. I don't think importers should be automatically enabled every time we run the testsuite. Instead, I would like to propose conditionally skipping the importer tests, if importers are not enabled. We have an example of a conditionally skipped test here:
If you don't agree, feel free to bring up the topic at the nearest QE sync and we can see what others think.
| }, | ||
| }; | ||
|
|
||
| const getOidcAccessToken = (page: Page): Promise<string | null> => |
There was a problem hiding this comment.
We shouldn't make another method for this, since we already have on here:
trustify-ui/e2e/tests/api/fixtures.ts
Line 23 in e1f27d0
There was a problem hiding this comment.
If it's not suitable for the use case for any reason, please just try to alter the method first, if possible.
There was a problem hiding this comment.
I rewrote the thing. Now the token is obtain directly from the session right after the UI login is successful. I hope it is fine, I felt kind of weird reusing API fixtures in the UI suite and it also caused some issues on downstream setups.
| importerName: string, | ||
| importerConfig: ImporterConfig, | ||
| ) => { | ||
| const accessToken = await getOidcAccessToken(page); |
There was a problem hiding this comment.
Actually, this whole thing with getting an access token should be unnecessary, since we should be using our custom axios fixture that takes care of this automatically. Calling the API should be enough.
| * sessionStorage is accessible. Reloads once if any importer was created. | ||
| */ | ||
| export const ensureAllImportersExist = async (page: Page): Promise<void> => { | ||
| const accessToken = await getOidcAccessToken(page); |
|
I am bit concerned about adding actual proper importers mid-test too, as mentioned by Vilem, they could pull in data which can break the other tests. The other side of the problem is of course not having any Importer results in all these Importer tests to fail. What I was thinking about is, if we could ensure we have exact for-tests importer(s) added (as part of setup) - in a way where such importer(s) would not pull in any data - or only such data which do not collide. As other problem importers may cause depending on the setup/env is the amount of resources and time consumed. So I would advocate for only very controlled ones:
|
|
As a part of this PR, I created a repository with 50000 fake advisories*. These advisories do not reference any real packages, so they should not cause any conflicts for the rest of the test suite. I hope it is fine this way. *Well, it contains 4 different fake advisories copied multiple times, just with different CVE-XXXX-XXXXX ID. |
3 participants
Summary
Some of the importer UI tests fail on downstream setups, as the downstream setup does not contain specific Importer objects, which are expected by the tests. This PR fixes the issue.
Related Issues
TC-5453
Type of Change
Testing
Summary by Sourcery
Ensure importer UI end-to-end tests provision their required data so they pass consistently across deployment environments.
Bug Fixes:
Enhancements:
Tests: