Repository navigation
fix(lgtm): keep evaluating the remaining pull requests when one evaluation fails - #375
Merged
Merged
Conversation
…ation fails The lgtm cron's doc comment and docs/automatic-merging.md say every eligible pull request is attempted and the run fails at the end listing the failures. That held for merges GitHub refused, but an evaluation that threw (for example a 500 re-reading one pull request) made the page throw `error processing pr: ...`, so the remaining pages were never evaluated. A throwing evaluation is now logged with core.error as `could not evaluate pr #<n>: <error>` and collected with the refused merges; the job goes on with the remaining pull requests and pages and fails at the end with the existing summary, e.g. `1 pull request(s) could not be merged: cncf#7 (could not evaluate: HttpError: ...)`. A listing of the open pull requests that cannot be read still stops the job (`could not get PRs`). Tests: the unit case and the bundle case (cncf#335) that pinned the fail-fast behaviour now assert that a later pull request (same page, and the next page) is still evaluated and merged and that the run fails naming the broken one. docs/cron-jobs.md and docs/automatic-merging.md describe the evaluation-failure arm. Signed-off-by: Jeffrey Sica <me@jeefy.dev>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Bug. The
lgtmcron is documented (doc comment insrc/cronJobs/lgtm.ts,docs/automatic-merging.md) to attempt every eligible pull request and fail at the end listing the failures. That held for merges GitHub refused, but an evaluation that threw for one pull request (e.g. a 500 re-reading it) made the page throwerror processing pr: ..., so later pull requests on later pages were never evaluated. No commit, comment or doc says the fail-fast was intended (it dates from the 2020 "Catch error cases" commit; #335 only pinned it with a test).Fix. A throwing evaluation is logged with
core.errorascould not evaluate pr #<n>: <error>and collected with the refused merges; the job continues with the remaining pull requests and pages, then fails with the existing summary, e.g.1 pull request(s) could not be merged: #7 (could not evaluate: HttpError: ...). A listing that cannot be read still stops the job (could not get PRs).Tests changed (they pinned the old behaviour).
__tests__/cronJobTest/lgtm.test.ts: "fails the run when evaluating a pr throws" replaced by two cases: a sibling on the same page is still merged and the run fails naming the broken PR; paging continues after the broken PR and the summary names both an evaluation failure and a refused merge.__tests__/bundle/cronLgtmFailureArms.test.ts(test(bundle): drive the lgtm cron's listing-500, closed-pr and evaluation-throw arms through dist/index.js #335): "evaluation throw ... stops paging" now asserts page 2 is listed, its PR is merged, and the run fails naming #2.Docs:
docs/cron-jobs.md,docs/automatic-merging.mddescribe the evaluation-failure arm.dist/index.jsregenerated withnpm run all.Verified locally:
npm run all(1847 tests),npm run test:coverage(thresholds pass), secondnpm run packleavesdist/clean,npm run test:coverage:e2e(214 tests;src/cronJobs/lgtm.ts100 %).