mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
Review automation: fix comment-only reviewer drop and draft-stale thrash (#6658)
* review-checklist: don't lose a reviewer who only left comments GitHub un-requests a reviewer the instant they submit any review, including a comment-only one from batching several inline comments into a single submission — not just Approve/Request-changes. Unlike a stale approval, this never produces a DISMISSED transition, so the checklist had no way to tell "abandoned the area" apart from "still reviewing, just hasn't finished yet." This silently dropped the reviewer's mention from the checklist display the moment they left comments, and risked the next push's additive-fill picker handing their area to a completely different load-balanced reviewer (observed on PR #6645: jhendersonHDF's batched review comments repeatedly vanished him from src/test's rows mid-review). Track a sticky area assignee as still engaged whenever they have no APPROVED/CHANGES_REQUESTED/DISMISSED review on record, regardless of whether GitHub currently lists them as requested, and use that in the read-only display path, resolveAreaPicks, and the additive-fill picker. * draft-pr-policy: a checked keepalive checkbox is real activity too lastRealActivityAt() deliberately ignores bot comments so a metadata-only bump can't dodge the staleness check forever. But checking the "Still working on this" checkbox is an edit to the bot's own keepalive comment, not a new comment of the human's own — its author stays github-actions[bot], so the edit was invisible to the activity check too. Confirming via the checkbox removed the label and posted "Thanks for confirming" without ever moving the underlying 60-day clock, so the very next scheduled run saw the same stale last-activity timestamp and immediately re-flagged it — contradicting the checkbox's own promise that checking it "resets this". Observed on #6326: once its true last activity fell behind the 60-day window, and only the checkbox (never a new commit or comment) was used to confirm it, the label thrashed on and off on a roughly 1-2 day loop. Now a checked keepalive checkbox counts via the comment's updated_at (when it was toggled), same as any other real activity signal.
This commit is contained in:
@@ -39,6 +39,17 @@ async function findKeepAliveComment(github, owner, repo, issue_number) {
|
||||
// milestone, assignee, etc.), which would let a draft dodge the staleness check forever
|
||||
// without any real work happening. Use the latest commit/comment/review activity instead.
|
||||
// Bot comments are excluded — they represent automated activity, not real human progress.
|
||||
//
|
||||
// One exception: a checked keepalive checkbox (see KEEPALIVE_CHECKBOX) IS real human
|
||||
// activity, even though it's an edit to the bot's own comment rather than a new one of
|
||||
// the human's own — GitHub lets any collaborator toggle a task-list checkbox in-place
|
||||
// without changing the comment's author. Without counting it here, confirming via the
|
||||
// checkbox would remove the label and post "Thanks for confirming" without ever moving
|
||||
// the underlying clock, so the very next scheduled run would see the same stale
|
||||
// last-activity timestamp and immediately re-flag it — contradicting the checkbox's own
|
||||
// promise that checking it "resets this" (PR #6326 thrashed the label on a ~1-2 day loop
|
||||
// once its true last activity fell behind the 60-day window and only the checkbox, never
|
||||
// a new commit or comment, was being used to confirm it).
|
||||
async function lastRealActivityAt(github, owner, repo, pr) {
|
||||
const timestamps = [new Date(pr.created_at).getTime()];
|
||||
|
||||
@@ -50,7 +61,11 @@ async function lastRealActivityAt(github, owner, repo, pr) {
|
||||
|
||||
const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: pr.number, per_page: 100 });
|
||||
for (const c of comments) {
|
||||
if (c.user?.type !== "Bot") timestamps.push(new Date(c.created_at).getTime());
|
||||
if (c.user?.type !== "Bot") {
|
||||
timestamps.push(new Date(c.created_at).getTime());
|
||||
} else if (c.body?.includes(KEEPALIVE_MARKER) && KEEPALIVE_CHECKED_RE.test(c.body)) {
|
||||
timestamps.push(new Date(c.updated_at).getTime());
|
||||
}
|
||||
}
|
||||
|
||||
const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100 });
|
||||
|
||||
@@ -399,24 +399,40 @@ function buildBody(touchedAreas, approvedUsers, confirmedRequested, changeReques
|
||||
//
|
||||
// Precedence per area:
|
||||
// 1. A persisted sticky assignment (assignedByArea), if it's still a valid
|
||||
// owner of this area and still currently requested.
|
||||
// owner of this area and either still currently requested, or absent
|
||||
// from `existingRequested` only because GitHub un-requests a reviewer
|
||||
// the instant they submit ANY review — including a comment-only one
|
||||
// from batching several inline comments into a single "Comment"
|
||||
// submission, which never produces a DISMISSED transition the way a
|
||||
// stale APPROVED review does (see planSynchronizeSwaps). Without this,
|
||||
// an actively-reviewing sticky pick who has only left comments so far
|
||||
// would look identical to one who's abandoned the area (PR #6645:
|
||||
// jhendersonHDF's batched review comments repeatedly dropped him from
|
||||
// requested_reviewers mid-review). A sticky pick with an actual
|
||||
// APPROVED/CHANGES_REQUESTED/DISMISSED review on record does NOT get
|
||||
// this pass — that's a real state transition other logic already
|
||||
// handles (approval sign-off, change-request lines, synchronize swaps).
|
||||
// 2. The sole currently-requested owner, if exactly one — nothing to prune,
|
||||
// so nothing to re-pick either.
|
||||
// 3. A fresh load-balanced pick via chooseReviewers, for whatever's left.
|
||||
//
|
||||
// Pure — no I/O. Returns { picks: Map<label, login>, log: string[] }.
|
||||
function resolveAreaPicks(areas, {
|
||||
existingRequested, assignedByArea, prAuthor, reviewerLoad, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
existingRequested, assignedByArea, prAuthor, reviewerLoad, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER, allReviews,
|
||||
}) {
|
||||
const picks = new Map();
|
||||
const log = [];
|
||||
const needsFreshPick = [];
|
||||
const finalStateLogins = new Set(Object.keys(latestReviewStates(allReviews || [])));
|
||||
|
||||
for (const area of areas) {
|
||||
const sticky = assignedByArea.get(area.label);
|
||||
if (sticky && area.owners.includes(sticky) && existingRequested.has(sticky)) {
|
||||
const stickyStillEngaged = sticky && (existingRequested.has(sticky) || !finalStateLogins.has(sticky));
|
||||
if (sticky && area.owners.includes(sticky) && stickyStillEngaged) {
|
||||
picks.set(area.label, sticky);
|
||||
log.push(`Area "${area.label}": keeping sticky assignment ${sticky}`);
|
||||
log.push(existingRequested.has(sticky)
|
||||
? `Area "${area.label}": keeping sticky assignment ${sticky}`
|
||||
: `Area "${area.label}": keeping sticky assignment ${sticky} (still engaged via comment-only review)`);
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -444,6 +460,33 @@ function resolveAreaPicks(areas, {
|
||||
return { picks, log };
|
||||
}
|
||||
|
||||
// Returns Map<areaLabel, login> of areas whose sticky assignment (see
|
||||
// ASSIGNED_PREFIX) is a valid owner who is NOT currently in GitHub's live
|
||||
// requested_reviewers set, but who hasn't given a review whose state governs
|
||||
// anything else (APPROVED/CHANGES_REQUESTED/DISMISSED are each already
|
||||
// tracked and displayed through their own mechanism). GitHub removes a
|
||||
// reviewer from requested_reviewers the instant they submit ANY review,
|
||||
// including a comment-only one from batching several inline comments into a
|
||||
// single "Comment" submission — with no corresponding DISMISSED transition
|
||||
// the way a stale APPROVED review gets (see planSynchronizeSwaps). Used by
|
||||
// both the read-only reflect-current-state path and the additive-fill path
|
||||
// so a mid-review reviewer never silently vanishes from the checklist
|
||||
// display, nor has their area handed to a brand-new load-balanced pick,
|
||||
// merely for having left comments so far (PR #6645).
|
||||
//
|
||||
// Pure — no I/O.
|
||||
function stillEngagedAssignees(areas, { assignedByArea, existingRequested, allReviews }) {
|
||||
const finalStateLogins = new Set(Object.keys(latestReviewStates(allReviews || [])));
|
||||
const result = new Map();
|
||||
for (const area of areas) {
|
||||
const sticky = assignedByArea.get(area.label);
|
||||
if (sticky && area.owners.includes(sticky) && !existingRequested.has(sticky) && !finalStateLogins.has(sticky)) {
|
||||
result.set(area.label, sticky);
|
||||
}
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
// ── GitHub API helpers ────────────────────────────────────────────────────────
|
||||
|
||||
// Removes auto-assignable reviewers (per prunableOwners — see the
|
||||
@@ -690,8 +733,22 @@ async function coordinateReviewers(github, context, core, {
|
||||
// ── read-only events ─────────────────────────────────────────────────────
|
||||
if (context.eventName === 'pull_request_review' || context.eventName === 'workflow_run') {
|
||||
core.info('Read-only event — reflecting current reviewer assignments');
|
||||
// Excluded owners are dropped first so a still-engaged sticky pick is
|
||||
// only honored while they remain a legitimate (non-excluded) owner —
|
||||
// see the eligibleAreas comment further down for why this mirrors that
|
||||
// filtering rather than checking touchedAreas directly.
|
||||
const readOnlyEligibleAreas = touchedAreas.map(area => ({
|
||||
...area,
|
||||
owners: area.owners.filter(o => !updatedExcluded.has(o)),
|
||||
}));
|
||||
const stillEngaged = stillEngagedAssignees(readOnlyEligibleAreas, {
|
||||
assignedByArea: updatedAssigned, existingRequested, allReviews,
|
||||
});
|
||||
for (const [label, login] of stillEngaged) {
|
||||
core.info(`Area "${label}": ${login} still engaged via comment-only review — showing as pending reviewer`);
|
||||
}
|
||||
return {
|
||||
confirmedRequested: new Set(existingRequested),
|
||||
confirmedRequested: new Set([...existingRequested, ...stillEngaged.values()]),
|
||||
excludedReviewers: updatedExcluded,
|
||||
manuallyAdded: updatedManuallyAdded,
|
||||
assignedReviewers: updatedAssigned,
|
||||
@@ -817,7 +874,7 @@ async function coordinateReviewers(github, context, core, {
|
||||
// during the draft period, or picked by an earlier coordination pass)
|
||||
// for whoever has the lightest queue right now.
|
||||
const { picks, log } = resolveAreaPicks(eligibleAreas, {
|
||||
existingRequested, assignedByArea: updatedAssigned,
|
||||
existingRequested, assignedByArea: updatedAssigned, allReviews,
|
||||
prAuthor, reviewerLoad, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
});
|
||||
for (const msg of log) core.info(msg);
|
||||
@@ -876,7 +933,7 @@ async function coordinateReviewers(github, context, core, {
|
||||
const algorithmAreas = avalancheAreas.filter(a => !forcedAreas.includes(a));
|
||||
|
||||
const { picks, log: pruneLog } = resolveAreaPicks(algorithmAreas, {
|
||||
existingRequested, assignedByArea: updatedAssigned,
|
||||
existingRequested, assignedByArea: updatedAssigned, allReviews,
|
||||
prAuthor, reviewerLoad, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
});
|
||||
for (const msg of pruneLog) core.info(msg);
|
||||
@@ -948,7 +1005,21 @@ async function coordinateReviewers(github, context, core, {
|
||||
// Non-draft, any other event: fill in a load-balanced reviewer only for
|
||||
// areas that don't already have one requested. Never removes anyone already
|
||||
// on the PR.
|
||||
const { selected, log } = chooseReviewers(eligibleAreas, {
|
||||
//
|
||||
// Areas whose sticky pick is still engaged via a comment-only review (see
|
||||
// stillEngagedAssignees) are held back from chooseReviewers entirely —
|
||||
// otherwise a mid-review reviewer silently un-requested by GitHub for
|
||||
// leaving a batch of comments would look "uncovered" and have their area
|
||||
// handed to a completely different load-balanced pick (PR #6645).
|
||||
const stillEngaged = stillEngagedAssignees(eligibleAreas, {
|
||||
assignedByArea: updatedAssigned, existingRequested, allReviews,
|
||||
});
|
||||
const areasNeedingFill = eligibleAreas.filter(a => !stillEngaged.has(a.label));
|
||||
for (const [label, login] of stillEngaged) {
|
||||
core.info(`Area "${label}": ${login} still engaged via comment-only review — not re-picking`);
|
||||
}
|
||||
|
||||
const { selected, log } = chooseReviewers(areasNeedingFill, {
|
||||
prAuthor,
|
||||
existingRequested, // real existing set — areas with an owner already present are skipped
|
||||
reviewerLoad,
|
||||
@@ -959,7 +1030,7 @@ async function coordinateReviewers(github, context, core, {
|
||||
if (selected.size === 0) {
|
||||
core.info('Every touched area already has a reviewer — nothing to add');
|
||||
return {
|
||||
confirmedRequested: new Set(existingRequested),
|
||||
confirmedRequested: new Set([...existingRequested, ...stillEngaged.values()]),
|
||||
excludedReviewers: updatedExcluded,
|
||||
manuallyAdded: updatedManuallyAdded,
|
||||
assignedReviewers: updatedAssigned,
|
||||
@@ -967,12 +1038,12 @@ async function coordinateReviewers(github, context, core, {
|
||||
}
|
||||
|
||||
const confirmed = await requestReviewers(github, core, pr, selected);
|
||||
for (const area of eligibleAreas) {
|
||||
for (const area of areasNeedingFill) {
|
||||
const pick = [...confirmed].find(l => area.owners.includes(l));
|
||||
if (pick) updatedAssigned.set(area.label, pick);
|
||||
}
|
||||
return {
|
||||
confirmedRequested: new Set([...existingRequested, ...confirmed]),
|
||||
confirmedRequested: new Set([...existingRequested, ...confirmed, ...stillEngaged.values()]),
|
||||
excludedReviewers: updatedExcluded,
|
||||
manuallyAdded: updatedManuallyAdded,
|
||||
assignedReviewers: updatedAssigned,
|
||||
@@ -1245,6 +1316,7 @@ module.exports.computeChangesRequested = computeChangesRequested;
|
||||
module.exports.buildChangeRequestFileMap = buildChangeRequestFileMap;
|
||||
module.exports.chooseReviewers = chooseReviewers;
|
||||
module.exports.resolveAreaPicks = resolveAreaPicks;
|
||||
module.exports.stillEngagedAssignees = stillEngagedAssignees;
|
||||
module.exports.buildBody = buildBody;
|
||||
module.exports.parseExcluded = parseExcluded;
|
||||
module.exports.serializeExcluded = serializeExcluded;
|
||||
|
||||
@@ -12,6 +12,7 @@ const {
|
||||
buildChangeRequestFileMap,
|
||||
chooseReviewers,
|
||||
resolveAreaPicks,
|
||||
stillEngagedAssignees,
|
||||
buildBody,
|
||||
parseExcluded,
|
||||
serializeExcluded,
|
||||
@@ -1350,13 +1351,18 @@ test('resolveAreaPicks: a valid sticky assignment is kept over a fresh load-bala
|
||||
assert.ok(log.some(l => l.includes('sticky')));
|
||||
});
|
||||
|
||||
test('resolveAreaPicks: a sticky assignment no longer requested falls back to a fresh pick', () => {
|
||||
// alice was the sticky pick but has since been removed from the PR
|
||||
// (e.g. an explicit removal) — must not "keep" someone who isn't there.
|
||||
test('resolveAreaPicks: a sticky assignment with a DISMISSED review on record falls back to a fresh pick', () => {
|
||||
// alice was the sticky pick but has an actual DISMISSED review on record
|
||||
// (e.g. a stale approval invalidated by a later push) and isn't currently
|
||||
// requested — a real state transition other logic already governs, so this
|
||||
// must not "keep" her. Contrast: a sticky pick who merely left comment-only
|
||||
// feedback and was silently un-requested by GitHub for it — no such
|
||||
// transition — DOES stay kept (see the "still engaged" tests below).
|
||||
const area = makeArea('fortran', ['alice', 'bob'], 50);
|
||||
const { picks } = resolveAreaPicks([area], {
|
||||
existingRequested: new Set(['bob']),
|
||||
assignedByArea: new Map([['fortran', 'alice']]),
|
||||
allReviews: [{ user: { login: 'alice' }, state: 'DISMISSED' }],
|
||||
prAuthor: 'charlie',
|
||||
reviewerLoad: {},
|
||||
LINE_THRESHOLD: 300, AREA_THRESHOLDS: {}, PUBLIC_HEADER: /public\.h$/,
|
||||
@@ -1364,6 +1370,26 @@ test('resolveAreaPicks: a sticky assignment no longer requested falls back to a
|
||||
assert.strictEqual(picks.get('fortran'), 'bob');
|
||||
});
|
||||
|
||||
test('resolveAreaPicks: a sticky assignment absent from requested_reviewers but with no final review state stays kept (PR #6645)', () => {
|
||||
// alice is the sticky pick, isn't currently requested (GitHub silently
|
||||
// un-requests a reviewer the instant they submit ANY review, including a
|
||||
// comment-only one), and has no APPROVED/CHANGES_REQUESTED/DISMISSED review
|
||||
// on record — just a COMMENTED one from batching several inline comments
|
||||
// into a single submission. She's still actively engaged and must not be
|
||||
// swapped out for a fresh pick.
|
||||
const area = makeArea('fortran', ['alice', 'bob'], 50);
|
||||
const { picks, log } = resolveAreaPicks([area], {
|
||||
existingRequested: new Set(['bob']),
|
||||
assignedByArea: new Map([['fortran', 'alice']]),
|
||||
allReviews: [{ user: { login: 'alice' }, state: 'COMMENTED' }],
|
||||
prAuthor: 'charlie',
|
||||
reviewerLoad: {},
|
||||
LINE_THRESHOLD: 300, AREA_THRESHOLDS: {}, PUBLIC_HEADER: /public\.h$/,
|
||||
});
|
||||
assert.strictEqual(picks.get('fortran'), 'alice');
|
||||
assert.ok(log.some(l => l.includes('still engaged via comment-only review')));
|
||||
});
|
||||
|
||||
test('resolveAreaPicks: a single already-requested owner is kept without invoking the load-balancer', () => {
|
||||
const area = makeArea('fortran', ['alice', 'bob'], 50);
|
||||
const { picks, log } = resolveAreaPicks([area], {
|
||||
@@ -1643,6 +1669,165 @@ asyncTest('coordinateReviewers: human-sender review_request_removed clears a pri
|
||||
assert.ok(!manuallyAdded.has('jhendersonHDF'));
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// stillEngagedAssignees — a sticky pick who submitted only comment-only
|
||||
// reviews (batching several inline comments into one "Comment" submission)
|
||||
// gets silently un-requested by GitHub with no DISMISSED transition to
|
||||
// react to, unlike a stale approval (PR #6645: jhendersonHDF's batched
|
||||
// review comments repeatedly dropped him from src/test's requested_reviewers
|
||||
// mid-review, which the checklist's read-only reflect-current-state path
|
||||
// and the additive-fill picker both read as "area abandoned").
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
test('stillEngagedAssignees: sticky pick missing from requested_reviewers with only a COMMENTED review stays engaged', () => {
|
||||
const area = makeArea('src', ['jhendersonHDF', 'mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map([['src', 'jhendersonHDF']]),
|
||||
existingRequested: new Set(),
|
||||
allReviews: [{ user: { login: 'jhendersonHDF' }, state: 'COMMENTED' }],
|
||||
});
|
||||
assert.strictEqual(engaged.get('src'), 'jhendersonHDF');
|
||||
});
|
||||
|
||||
test('stillEngagedAssignees: sticky pick missing from requested_reviewers with NO review at all also stays engaged', () => {
|
||||
// Not yet reviewed at all is the common case (assigned, hasn't looked yet
|
||||
// for some other reason) — same treatment as comment-only.
|
||||
const area = makeArea('src', ['jhendersonHDF', 'mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map([['src', 'jhendersonHDF']]),
|
||||
existingRequested: new Set(),
|
||||
allReviews: [],
|
||||
});
|
||||
assert.strictEqual(engaged.get('src'), 'jhendersonHDF');
|
||||
});
|
||||
|
||||
test('stillEngagedAssignees: sticky pick still currently requested is not included (nothing to restore)', () => {
|
||||
const area = makeArea('src', ['jhendersonHDF', 'mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map([['src', 'jhendersonHDF']]),
|
||||
existingRequested: new Set(['jhendersonHDF']),
|
||||
allReviews: [],
|
||||
});
|
||||
assert.strictEqual(engaged.size, 0);
|
||||
});
|
||||
|
||||
test('stillEngagedAssignees: sticky pick with an APPROVED review is not included (real sign-off, not comment-only)', () => {
|
||||
const area = makeArea('src', ['jhendersonHDF', 'mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map([['src', 'jhendersonHDF']]),
|
||||
existingRequested: new Set(),
|
||||
allReviews: [{ user: { login: 'jhendersonHDF' }, state: 'APPROVED' }],
|
||||
});
|
||||
assert.strictEqual(engaged.size, 0);
|
||||
});
|
||||
|
||||
test('stillEngagedAssignees: sticky pick with a DISMISSED review is not included (real transition, handled by synchronize swaps)', () => {
|
||||
const area = makeArea('src', ['jhendersonHDF', 'mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map([['src', 'jhendersonHDF']]),
|
||||
existingRequested: new Set(),
|
||||
allReviews: [{ user: { login: 'jhendersonHDF' }, state: 'DISMISSED' }],
|
||||
});
|
||||
assert.strictEqual(engaged.size, 0);
|
||||
});
|
||||
|
||||
test('stillEngagedAssignees: no sticky assignment for the area produces no entry', () => {
|
||||
const area = makeArea('src', ['jhendersonHDF', 'mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map(),
|
||||
existingRequested: new Set(),
|
||||
allReviews: [],
|
||||
});
|
||||
assert.strictEqual(engaged.size, 0);
|
||||
});
|
||||
|
||||
test('stillEngagedAssignees: sticky login no longer an owner of the area produces no entry', () => {
|
||||
// e.g. CODEOWNERS changed, or the sticky record is stale for this area.
|
||||
const area = makeArea('src', ['mattjala'], 50);
|
||||
const engaged = stillEngagedAssignees([area], {
|
||||
assignedByArea: new Map([['src', 'jhendersonHDF']]),
|
||||
existingRequested: new Set(),
|
||||
allReviews: [],
|
||||
});
|
||||
assert.strictEqual(engaged.size, 0);
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// coordinateReviewers — comment-only batched reviews must not un-cover an
|
||||
// area (PR #6645 scenario, reproduced end-to-end)
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
asyncTest('coordinateReviewers: read-only event still shows a sticky pick GitHub un-requested for a comment-only review', async () => {
|
||||
// jhendersonHDF is the settled reviewer for .github, but just batched
|
||||
// several inline comments into one "Comment" submission — GitHub already
|
||||
// dropped him from requested_reviewers by the time this (read-only)
|
||||
// workflow_run/pull_request_review pass runs. He must still show up as the
|
||||
// pending reviewer rather than the area looking unassigned.
|
||||
const github = makeGithubMock();
|
||||
const context = { eventName: 'workflow_run', payload: {} };
|
||||
const args = makeCoordinateBaseArgs({
|
||||
assignedReviewers: new Map([['.github', 'jhendersonHDF']]),
|
||||
allReviews: [{ user: { login: 'jhendersonHDF' }, state: 'COMMENTED' }],
|
||||
prData: {
|
||||
user: { login: 'lrknox' },
|
||||
draft: false,
|
||||
requested_reviewers: [], // GitHub already un-requested him for his review
|
||||
},
|
||||
});
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
assert.ok(confirmedRequested.has('jhendersonHDF'));
|
||||
assert.strictEqual(github.calls.removeRequestedReviewers.length, 0);
|
||||
assert.strictEqual(github.calls.requestReviewers.length, 0);
|
||||
});
|
||||
|
||||
asyncTest('coordinateReviewers: additive-fill does not re-pick a new reviewer for an area whose sticky pick only commented', async () => {
|
||||
// A later synchronize push (not opened/reopened/ready_for_review, no
|
||||
// avalanche, no dismissed reviewer) falls to the plain additive-fill path.
|
||||
// Without the fix, chooseReviewers would see .github as "uncovered" (no
|
||||
// owner in requested_reviewers) and load-balance a completely different
|
||||
// owner onto it — even though jhendersonHDF is still mid-review.
|
||||
const github = makeGithubMock();
|
||||
const context = { eventName: 'pull_request_target', payload: { action: 'synchronize', sender: { type: 'User' } } };
|
||||
const args = makeCoordinateBaseArgs({
|
||||
assignedReviewers: new Map([['.github', 'jhendersonHDF']]),
|
||||
allReviews: [{ user: { login: 'jhendersonHDF' }, state: 'COMMENTED' }],
|
||||
// Load-balancer would strongly prefer glennsong09 if it ran — proving
|
||||
// jhendersonHDF is kept because he's still engaged, not by coincidence.
|
||||
reviewerLoad: { hyoklee: 50, jhendersonHDF: 50, glennsong09: 0 },
|
||||
prData: {
|
||||
user: { login: 'lrknox' },
|
||||
draft: false,
|
||||
requested_reviewers: [], // no one currently requested for .github
|
||||
},
|
||||
});
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
assert.ok(confirmedRequested.has('jhendersonHDF'));
|
||||
assert.strictEqual(github.calls.requestReviewers.length, 0, 'No new reviewer should be requested for the area');
|
||||
});
|
||||
|
||||
asyncTest('coordinateReviewers: additive-fill still fills an area with no sticky assignment at all', async () => {
|
||||
// Sanity check the fix doesn't over-fire: an area with no sticky record
|
||||
// and no currently-requested owner is filled normally.
|
||||
const github = makeGithubMock();
|
||||
const context = { eventName: 'pull_request_target', payload: { action: 'synchronize', sender: { type: 'User' } } };
|
||||
const args = makeCoordinateBaseArgs({
|
||||
prData: {
|
||||
user: { login: 'lrknox' },
|
||||
draft: false,
|
||||
requested_reviewers: [],
|
||||
},
|
||||
});
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
assert.strictEqual(github.calls.requestReviewers.length, 1);
|
||||
assert.strictEqual(confirmedRequested.size, 1);
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// Summary
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user