Attempts to fix site-with-errors tests timing out#152
Attempts to fix site-with-errors tests timing out#152lindseywild wants to merge 5 commits intomainfrom
Conversation
Co-authored-by: lindseywild <35239154+lindseywild@users.noreply.github.com>
…to missing PRs Co-authored-by: lindseywild <35239154+lindseywild@users.noreply.github.com>
Co-authored-by: lindseywild <35239154+lindseywild@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adjusts test expectations and retry behavior to reduce flakiness/timeouts when validating cached results for the site-with-errors scenario.
Changes:
- Make
pullRequestoptional in cachedResulttest typing. - Update
site-with-errorstest to avoid assuming every cached result contains a PR URL; only fetch PRs when present. - Increase default retry attempts in the
fixGitHub Action retry helper.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/types.d.ts | Makes Result.pullRequest optional to reflect real cache contents. |
| tests/site-with-errors.test.ts | Stops unconditionally reading/fetching PR URLs; fetches PRs only when present and adds a PR-count assertion. |
| .github/actions/fix/src/retry.ts | Raises default retry attempts for exponential-backoff retries. |
Comments suppressed due to low confidence (1)
tests/site-with-errors.test.ts:150
- The
filterensurespullRequest?.urlexists, but TypeScript can’t narrow the type across this chain, so the code relies onpullRequest!non-null assertions. Consider using a type-predicate filter (or filtering onpullRequestitself) to eliminate the non-null assertions and keep the code type-safe if this logic changes later.
results
.filter(({pullRequest}) => !!pullRequest?.url)
.map(async ({pullRequest}) => {
const pullRequestUrl = pullRequest!.url
const {owner, repo, pullNumber} =
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function retry<T>( | ||
| fn: () => Promise<T | null | undefined> | T | null | undefined, | ||
| maxAttempts = 6, | ||
| maxAttempts = 10, |
There was a problem hiding this comment.
Increasing the default maxAttempts from 6 to 10 significantly increases worst-case retry time with the current exponential backoff (baseDelay 2s, capped at 30s), potentially adding multiple minutes of waiting and worsening timeouts in the action. Consider keeping the default smaller and/or passing a higher maxAttempts only at the call site(s) that need it, or making it configurable via input/env so retries don’t unexpectedly extend runtimes.
| maxAttempts = 10, | |
| maxAttempts = 6, |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.