tests: Fix race condition in one of the Scan Sbom tests#931
tests: Fix race condition in one of the Scan Sbom tests#931vobratil merged 3 commits intoguacsec:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the SBOM scan cancellation E2E test to merge spinner verification and cancel action into a single step, and introduces Playwright request routing to delay API calls so the cancel button can be reliably located and clicked, eliminating a race condition-induced flake. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider wrapping the
page.route/page.unroutecalls in atry/finallyblock so the route is always removed even ifexpectProcessingSpinnerorclickCancelProcessingthrows, preventing the handler from leaking into later steps. - Routing
**/api/**with a hard-coded 10s delay is quite broad and slow; you might want to narrow the URL pattern to only the specific SBOM-related request(s) and/or reduce the delay to the minimum needed to reliably click cancel.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider wrapping the `page.route`/`page.unroute` calls in a `try/finally` block so the route is always removed even if `expectProcessingSpinner` or `clickCancelProcessing` throws, preventing the handler from leaking into later steps.
- Routing `**/api/**` with a hard-coded 10s delay is quite broad and slow; you might want to narrow the URL pattern to only the specific SBOM-related request(s) and/or reduce the delay to the minimum needed to reliably click cancel.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #931 +/- ##
==========================================
+ Coverage 64.47% 65.01% +0.53%
==========================================
Files 195 195
Lines 3341 3341
Branches 753 753
==========================================
+ Hits 2154 2172 +18
+ Misses 890 872 -18
Partials 297 297 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vobratil
left a comment
There was a problem hiding this comment.
Thanks for the fix! I just have a question about the fix in one of the comments. Not really sure that it's a good idea, but I'm happy to be proven wrong.
| await scanPage.expectProcessingSpinner(header, cancelLabel); | ||
|
|
||
| // Block all API requests to keep the page in processing state long enough to click cancel | ||
| await page.route("**/api/**", async (route) => { |
There was a problem hiding this comment.
This seems highly sketchy to me. Have you tried this with multiple workers locally? On CI we seem to run all tests with a single worker, so this shouldn't be an issue, but I would guess that blocking all API requests could easily break a lot of tests. I don't know, maybe you've tested this and it works. But maybe, at the minimum, we should only apply this to more specific API endpoints?
There was a problem hiding this comment.
Honestly I would not expect this to block all of the workers. I have not actually found any Playwright command like that. I will double-check though.
There was a problem hiding this comment.
Looks like the tests are using around 10 walkers when run locally, so it was already executed without any problems. I also did some digging and each of the workers should have its own context and this command only manipulates the context of the specific worker. I think this should be safe.
There was a problem hiding this comment.
On second thought that makes sense. If this only blocks the one worker, then no additional API requests in the background should be necessary. If you have this tested, then let's just roll with it.
vobratil
left a comment
There was a problem hiding this comment.
After a short discussion, this one seems go to go! Thank you, @matejnesuta!
This PR fixes Scan SBOM test, which tests cancelling of the SBOM analysis.
Even though this test passes on the Github CI, it was occasionally failing on my local setup. There is slight chance that the Scan Sbom result page loads faster than Playwright is able to query the locators for aborting the analysis, which causes the test to flake. Given that Trustify on OCP/RHEL should be faster than in the Github CI, I think it is reasonable to expect this flake on downstream builds as well.
This fix merges 2 Gherkin steps together and adds a
page.route()mechanism, which delays any network request on the loading page by 10 seconds, allowing Playwright to properly query theCancel Reportbutton.Summary by Sourcery
Tests: