test: just debugging the tests#3634
Conversation
jrichter1
requested review from
a team,
Eswaraiahsapram,
christoph-jerolimov,
ciiay,
divyanshiGupta,
its-mitesh-kumar,
karthikjeeyar and
rohitkrai03
as code owners
|
🤖 Finished Review · ✅ Success · Started 8:11 AM UTC · Completed 8:20 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3634 +/- ##
=======================================
Coverage 54.03% 54.03%
=======================================
Files 2325 2325
Lines 89195 89195
Branches 24936 24924 -12
=======================================
Hits 48193 48193
- Misses 40707 40708 +1
+ Partials 295 294 -1
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ReviewFindingsMedium
Labels: PR is explicitly not ready to merge; adding do-not-merge/work-in-progress to match stated intent. Previous runReviewFindingsCritical
Medium
Low
Previous run (2)ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 9:13 AM UTC · Completed 9:28 AM UTC |
| yarn backstage-repo-tools knip-reports | ||
| ``` | ||
|
|
||
| 1 |
There was a problem hiding this comment.
[medium] scope pollution
Five README files across five unrelated workspaces have had the character 1 appended as junk content with no functional or documentation value.
Suggested fix: Remove all five junk README modifications from this PR.
| await page.locator('a').filter({ hasText: 'Home' }).click(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[medium] scope mismatch
The waitForComponent helper is the only substantive change but is bundled with five unrelated junk README changes and submitted without a linked issue.
Suggested fix: Submit the waitForComponent utility in a standalone PR linked to an issue.
| const baseUrl = process.env.PLAYWRIGHT_URL ?? 'http://localhost:7007'; | ||
| const api = `${baseUrl}/api/catalog/entities/by-query?limit=20`; | ||
| const retries = timeout / 1000; | ||
|
|
There was a problem hiding this comment.
[medium] error handling gap
waitForComponent silently resolves without error when the component is never found within the timeout period. Callers assume the component exists after this call returns.
Suggested fix: Throw an error after the loop completes without finding the component.
| for (let i = 0; i < retries; i++) { | ||
| const response = await fetch(api); | ||
| const json = await response.json(); | ||
|
|
There was a problem hiding this comment.
[medium] error handling gap
The fetch call and response.json() inside the retry loop have no error handling. The function will throw an unhandled exception on the first iteration rather than retrying.
Suggested fix: Wrap the fetch/json() calls in a try-catch inside the loop body.
| const api = `${baseUrl}/api/catalog/entities/by-query?limit=20`; | ||
| const retries = timeout / 1000; | ||
|
|
||
| for (let i = 0; i < retries; i++) { |
There was a problem hiding this comment.
[low] off-by-one / precision
const retries = timeout / 1000 performs floating-point division producing fractional loop bounds for non-multiple-of-1000 timeouts.
Suggested fix: Use Math.ceil(timeout / 1000) or Math.floor(timeout / 1000).
| timeout = 5000, | ||
| ): Promise<void> { | ||
| const baseUrl = process.env.PLAYWRIGHT_URL ?? 'http://localhost:7007'; | ||
| const api = `${baseUrl}/api/catalog/entities/by-query?limit=20`; |
There was a problem hiding this comment.
[low] naming-conventions
The variable retries represents the number of iterations (attempts), not the number of retries.
Suggested fix: Rename to maxAttempts or attempts.
fullsend-ai-review
Bot
removed
the
ready-for-merge
jrichter1
force-pushed
the
run-the-stupid-tests
branch
from
305d9e3 to
74f8ba8
Compare
|
|
🤖 Finished Review · ✅ Success · Started 9:42 AM UTC · Completed 9:54 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
1 participant
Hey, I just made a Pull Request!
do not merge, obviously
✔️ Checklist