{{ message }}
Repository navigation
test(tide): pin the lgtm gate re-check after an unknown-mergeability retry (evaluateMerge, tide.ts:198-206) - #557
Open
hivecommons-hive[bot] wants to merge 1 commit into
Conversation
tryMergePullRequest re-reads a pull request whose mergeability is still unknown. Two cases were missing at the caller level: the lgtm label disappearing on the re-read must end the wait at that read (no 2 s and 4 s retries), and a re-read that turns clean while losing lgtm must not merge. Dropping the blockedReason(pr, tide) re-check after fetchMergeability in evaluateMerge survived every existing unit test (982) and the rebuilt bundle suite (528); the second case fails on that mutant. Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com>
hivecommons-hive
Bot
added
the
hold
Contributor
Author
This was referenced Oct 21, 2026
Open
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Read against main@e3119ad. This looks correct to me, and the second case pins a gate that genuinely has no other caller-level guard.
- The re-check exists only at
src/plugins/tide.ts:203(reason = blockedReason(pr, tide) ?? …afterfetchMergeability); nothing downstream re-reads labels beforemergeOnce, so deleting that re-check would indeed merge a PR whoselgtmvanished during the unknown-mergeability wait. The "survived 982 unit + 528 bundle tests" claim is consistent with what the existing suite asserts — the nearest case (tide.test.ts:257area) only covers unknown-with-lgtm-retained. - First case:
servePull(pull(['lgtm'], unknown), pull([], unknown))—fetchMergeability'sretryIf: candidate => blockedReason(candidate, tide) === undefined(tide.ts:199) goes false on the re-read, so exactly 2 GETs and onesleep(1000)is what the source produces; the absence-of-give-up-line assertion is a nice touch. - Second case: re-read is clean but label-less, so it exercises the
??left side specifically — the merge-blocking mutant the body names. - Helpers used (
servePull,observeMerge, mockedsleep) behave as the test assumes (tide.test.ts:46-62,72).
Fixes/Refs semantics: the body says "Fixes #554" but then states #554 must stay open — note GitHub will close #554 on merge with that keyword. If the tracker is meant to survive, the line should read Refs #554. Worth a one-word edit before merge.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
Contributor
Author
github-actions
Bot
added
kind/cleanup
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
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.
Test Improvement
Adds two
tryMergePullRequestcases to__tests__/plugins/tide.test.tsfor the mid-retry gate change that #554 ("Judge tests by behavior") asks for: mergeability isunknownon the first read and thelgtmlabel is gone on the re-read.sleep(1000),skipping pr #1: missing lgtm, no merge, and no "still unknown after 3 retries" log.missing lgtm.Why these two, and not more coverage
Neither adds line/branch coverage; both are justified by mutants, as #554 requires:
blockedReason(pr, tide) ??re-check afterfetchMergeabilityinevaluateMerge(src/plugins/tide.ts:203) survived all 982 existing unit tests and, afternpm run build && npm run pack, all 528 bundle tests. With that mutant a pull request whoselgtmis removed while GitHub computes mergeability would merge. The second case fails on it.|| !retryIf(pr)infetchMergeabilitywas already caught by the directfetchMergeabilityunit case (tide.test.ts:134-140), but not through the caller; the first case pins it at thetryMergePullRequestlevel with read and sleep counts.Files/functions claimed:
__tests__/plugins/tide.test.ts(tryMergePullRequestdescribe block) only. Disjoint from #555 (fakeGithub/ownersGroups/blunderbussDelay), #553 (auth arms), #556 (coverage config).Related Issue
Fixes #554 — this ships the "focused integration scenario where mergeability is initially unknown and LGTM disappears on the next read" checkbox under P2: Judge tests by behavior. The tracker stays open: its other sections (CI enforcement, release delivery, harness/mutation runner, remaining cross-request contracts) are not touched here and the issue states a single test PR must not close it.
Verification
npx vitest run __tests__/plugins/tide.test.ts— 107 passednpx eslint __tests__/plugins/tide.test.ts— cleanFiled by quality agent (hold-gated mode). Human review required.
— hive: agent=quality backend=copilot model=claude-fable-5.1 requested_by=@mrbobbytables