Add optional delay for blunderbuss

__tests__/bundle/blunderbussDelay.test.ts

@@ -0,0 +1,243 @@
+import type { FakeGithub } from './fakeGithub'
+import { Buffer } from 'node:buffer'
+import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'
+
+import labelFileContents from '../fixtures/labels/labelFileContentsResp.json'
+import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json'
+import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json'
+import { blobSha } from '../utils/ownersData'
+import { start } from './fakeGithub'
+import { comment, configReads, helpersFor, repo, token } from './helpers'
+import { runBundle } from './runBundle'
+
+vi.setConfig({ testTimeout: 30_000 })
+
+describe('dist/index.js delayed blunderbuss', () => {
+  let gh: FakeGithub
+  const { routeOwners, expectRequests } = helpersFor(() => gh)
+  const owners = { OWNERS: 'reviewers:\n- bob\n' }
+  const timelinePath = `${repo}/issues/1/timeline`
+
+  beforeAll(async () => {
+    gh = await start()
+  })
+  afterEach(() => gh.reset())
+  afterAll(() => gh.close())
+
+  function configure(section = '  delay: 1h\n') {
+    const file = structuredClone(labelFileContents)
+    file.content = Buffer.from(`blunderbuss:\n${section}`).toString('base64')
+    gh.route('GET', '/repos/Codertocat/.project/contents/prow.yaml', { status: 200, body: file })
+  }
+
+  function ago(minutes: number) {
+    return new Date(Date.now() - minutes * 60_000).toISOString()
+  }
+
+  function pull(overrides: Record<string, unknown> = {}) {
+    return {
+      ...structuredClone(pullReqListPulls[0]),
+      number: 1,
+      created_at: ago(70),
+      updated_at: ago(70),
+      labels: [],
+      requested_reviewers: [],
+      requested_teams: [],
+      assignees: [],
+      draft: false,
+      user: { login: 'Codertocat' },
+      base: { ref: 'master', sha: 'basesha' },
+      head: { sha: 'sha1', repo: { full_name: 'Codertocat/Hello-World' } },
+      ...overrides,
+    }
+  }
+
+  function routeSweep(pr = pull(), timeline: unknown[] = [], reviews: unknown[] = []) {
+    gh.route('GET', new RegExp(`^${repo}/pulls\\?`), { status: 200, body: [pr] })
+    gh.route('GET', `${repo}/git/trees/master`, {
+      status: 200,
+      body: { sha: 'master', truncated: false, tree: [{ path: 'OWNERS', type: 'blob', sha: blobSha('OWNERS') }] },
+    })
+    routeOwners(owners, ['x.go'], pr)
+    gh.route('GET', timelinePath, { status: 200, body: timeline })
+    gh.route('GET', `${repo}/pulls/1/reviews`, { status: 200, body: reviews })
+    gh.route('POST', `${repo}/pulls/1/requested_reviewers`, { status: 201, body: {} })
+    // Recent candidates still run the other sweep plugins.
+    gh.route('GET', `${repo}/issues/1`, { status: 200, body: { labels: [] } })
+    gh.route('GET', `${repo}/labels`, { status: 200, body: [{ name: 'approved' }, { name: 'lgtm' }] })
+    gh.route('GET', `${repo}/issues/1/comments`, { status: 200, body: [] })
+    gh.route('POST', `${repo}/issues/1/comments`, { status: 201, body: {} })
+  }
+
+  function runSweep() {
+    return runBundle({ eventName: 'schedule', payload: {}, inputs: { ...token, jobs: 'sweep' }, apiUrl: gh.url })
+  }
+
+  function requested() {
+    return gh.requestsMatching('POST', /\/requested_reviewers$/).map(r => r.body)
+  }
+
+  function timelineReads() {
+    return gh.requestsMatching('GET', /\/timeline\?per_page=100$/)
+  }
+
+  function reviewReads() {
+    return gh.requestsMatching('GET', /\/reviews\?per_page=1$/)
+  }
+
+  it.each(['opened', 'ready_for_review'])('%s defers reviewer assignment to the scheduled sweep', async (action) => {
+    configure()
+    routeOwners(owners, ['x.go'])
+
+    const result = await runBundle({
+      eventName: 'pull_request',
+      payload: { ...pullReqOpenedEvent, action },
+      inputs: token,
+      apiUrl: gh.url,
+    })
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(result.stdout).toContain(`#1 ${action}; blunderbuss.delay is set`)
+    expect(requested()).toEqual([])
+    expect(timelineReads()).toEqual([])
+  })
+
+  it('/auto-cc still immediately requests reviewers on a draft when delay is configured', async () => {
+    configure()
+    routeOwners(owners, ['x.go'], { draft: true })
+    gh.route('POST', `${repo}/pulls/1/requested_reviewers`, { status: 201, body: {} })
+    const payload = comment('/auto-cc')
+    Object.assign(payload.issue, { pull_request: { url: `${gh.url}${repo}/pulls/1` } })
+
+    const result = await runBundle({
+      eventName: 'issue_comment',
+      payload,
+      inputs: { ...token, 'prow-commands': '/auto-cc' },
+      apiUrl: gh.url,
+    })
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(requested()).toEqual([{ reviewers: ['bob'] }])
+    expect(timelineReads()).toEqual([])
+  })
+
+  it('requests reviewers for an untouched due pull outside the normal lookback without running other sweep steps', async () => {
+    configure()
+    routeSweep()
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(result.stdout).toContain('sweep: 0 candidates updated since')
+    expect(result.stdout).toContain('sweep: 1 more checked for delayed reviewers')
+    expect(requested()).toEqual([{ reviewers: ['bob'] }])
+    expect(timelineReads()).toHaveLength(1)
+    expect(reviewReads()).toHaveLength(1)
+    expect(gh.requests.filter(r => r.method !== 'GET').map(r => r.path)).toEqual([`${repo}/pulls/1/requested_reviewers`])
+    expect(gh.requestsMatching('GET', /\/issues\/1\/comments|\/actions\/runs/)).toEqual([])
+  })
+
+  it.each([
+    ['a draft', { draft: true }, [], 'is a draft', 0],
+    ['a manual reviewer', { requested_reviewers: [{ login: 'alice' }] }, [], 'already has requested reviewers', 0],
+    ['a manual team', { requested_teams: [{ slug: 'sdk' }] }, [], 'already has requested reviewers', 0],
+    ['an existing review', {}, [{ id: 7, user: { login: 'alice' }, state: 'COMMENTED' }], 'already has reviews', 1],
+  ])('does not assign a due pull with %s', async (_name, overrides, reviews, message, reads) => {
+    configure()
+    routeSweep(pull(overrides), [], reviews)
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(result.stdout).toContain(`sweep: #1 ${message}`)
+    expect(requested()).toEqual([])
+    expect(timelineReads()).toHaveLength(reads)
+    expect(reviewReads()).toHaveLength(reads)
+  })
+
+  it.each([
+    ['not yet due', 30, 30, 'is not due for reviewers', 0],
+    ['opened before the due window', 180, 70, 'outside the due window', 1],
+  ])('leaves a pull %s alone', async (_name, created, updated, message, reads) => {
+    configure()
+    routeSweep(pull({ created_at: ago(created), updated_at: ago(updated) }))
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(result.stdout).toContain(message)
+    expect(requested()).toEqual([])
+    expect(timelineReads()).toHaveLength(reads)
+    expect(reviewReads()).toEqual([])
+  })
+
+  it.each([
+    ['due', 70, true],
+    ['recently marked ready again', 10, false],
+  ])('uses the latest ready_for_review clock for a former draft that is %s', async (_name, latest, due) => {
+    configure()
+    routeSweep(pull({ created_at: ago(300), updated_at: ago(latest) }), [
+      { event: 'ready_for_review', created_at: ago(latest) },
+      { event: 'ready_for_review', created_at: ago(240) },
+      { event: 'labeled', created_at: ago(5) },
+      { event: 'ready_for_review' },
+    ])
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(timelineReads()).toHaveLength(1)
+    expect(reviewReads()).toHaveLength(due ? 1 : 0)
+    expect(requested()).toEqual(due ? [{ reviewers: ['bob'] }] : [])
+    if (!due) {
+      expect(result.stdout).toContain('outside the due window')
+      expect(result.stdout).toContain('sweep: #1 evaluated')
+    }
+  })
+
+  it('uses creation time and requests on a due draft when ignore_drafts is false', async () => {
+    configure('  delay: 1h\n  ignore_drafts: false\n')
+    routeSweep(pull({ draft: true }))
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(requested()).toEqual([{ reviewers: ['bob'] }])
+    expect(timelineReads()).toEqual([])
+  })
+
+  it('fails closed when a delayed pull timeline cannot be read', async () => {
+    configure()
+    gh.route('GET', timelinePath, { status: 403, body: { message: 'timeline denied' } })
+    routeSweep()
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(1)
+    expect(result.errors.join('\n')).toContain('sweep: 1 pull request(s) failed: #1 (blunderbuss: could not read the timeline: HttpError: timeline denied)')
+    expect(timelineReads()).toHaveLength(1)
+    expect(reviewReads()).toEqual([])
+    expect(requested()).toEqual([])
+  })
+
+  it.each([
+    ['3600', 'blunderbuss.delay must be a duration string such as 1h or 30m'],
+    ['later', 'invalid blunderbuss.delay \'later\': expected a duration such as 5s, 2m or 500ms'],
+    ['25h', 'blunderbuss.delay must not be longer than 24h'],
+  ])('refuses invalid delay %s before listing or mutating pulls', async (delay, message) => {
+    configure(`  delay: ${delay}\n`)
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(1)
+    expect(result.errors.join('\n')).toContain(`Codertocat/.project:prow.yaml: ${message}`)
+    expectRequests(configReads({ org: '.project' }), [])
+  })
+})

__tests__/cronJobTest/sweep.test.ts

@@ -33,6 +33,7 @@ interface PullSpec {
   labels?: string[]
   draft?: boolean
   requested_reviewers?: string[]
+  requested_teams?: string[]
   head?: string
   mergeable_state?: string
   user?: string
@@ -51,6 +52,7 @@ function pull(spec: PullSpec) {
     created_at: spec.created ?? spec.updated ?? ago(10 * 60_000),
     updated_at: spec.updated ?? ago(10 * 60_000),
     requested_reviewers: (spec.requested_reviewers ?? []).map(login => ({ login })),
+    requested_teams: (spec.requested_teams ?? []).map(slug => ({ slug })),
     assignees: [],
     user: { login: spec.user ?? 'some-author' },
     head: { sha: spec.head ?? `sha${spec.number}` },
@@ -112,6 +114,9 @@ describe('resolveSweepLookback', () => {
     ['a non-string lookback', 'sweep:\n  lookback: 3600\n', 'x: sweep.lookback must be a duration string such as 1h or 30m'],
     ['a malformed lookback', 'sweep:\n  lookback: soon\n', 'x: invalid sweep.lookback \'soon\': expected a duration such as 5s, 2m or 500ms'],
     ['a zero lookback', 'sweep:\n  lookback: \'0\'\n', 'x: sweep.lookback must be longer than 0'],
+    ['a non-string blunderbuss.delay', 'blunderbuss:\n  delay: 3600\n', 'x: blunderbuss.delay must be a duration string such as 1h or 30m'],
+    ['a malformed blunderbuss.delay', 'blunderbuss:\n  delay: later\n', 'x: invalid blunderbuss.delay \'later\': expected a duration such as 5s, 2m or 500ms'],
+    ['a blunderbuss.delay over 24h', 'blunderbuss:\n  delay: 25h\n', 'x: blunderbuss.delay must not be longer than 24h'],
   ])('rejects %s', (_, text, error) => {
     expect(() => parseProwConfig('x', text)).toThrow(error)
   })
@@ -489,6 +494,82 @@ describe('sweep on a repository with OWNERS files', () => {
   })
 })
 
+describe('sweep with blunderbuss.delay', () => {
+  const owners = { OWNERS: 'approvers:\n- alice\nreviewers:\n- carol\n' }
+
+  function serve(specs: PullSpec[], timelines: Record<number, string[]> = {}, reviewed: number[] = []) {
+    const tree = Object.keys(owners).map(path => ({ path, type: 'blob', sha: blobSha(path) }))
+    const requested: number[] = []
+    const timelineReads: number[] = []
+    server.use(
+      prowYaml('blunderbuss:\n  delay: 1h\n'),
+      utils.defaultBranchTree(Object.keys(owners)),
+      http.get(`${repo}/git/trees/basesha`, utils.mockResponse(200, { sha: 'basesha', truncated: false, tree })),
+      http.get(`${repo}/pulls/:number/files`, utils.mockResponse(200, [{ filename: 'x.go', status: 'modified' }])),
+      ...Object.entries(owners).map(([path, contents]) => http.get(`${repo}/git/blobs/${blobSha(path)}`, utils.mockResponse(200, { encoding: 'base64', content: Buffer.from(contents).toString('base64') }))),
+      http.get(`${repo}/issues/:number`, utils.mockResponse(200, { labels: [] })),
+      http.get(`${repo}/issues/:number/comments`, utils.mockResponse(200, [])),
+      http.post(`${repo}/issues/:number/comments`, utils.mockResponse(201, {})),
+      http.get(`${repo}/issues/:number/timeline`, ({ params }) => {
+        timelineReads.push(Number(params.number))
+        return json((timelines[Number(params.number)] ?? []).map(at => ({ event: 'ready_for_review', created_at: at })))
+      }),
+      http.get(`${repo}/pulls/:number/reviews`, ({ params }) => json(reviewed.includes(Number(params.number)) ? [{ id: 1, state: 'COMMENTED', user: { login: 'carol' } }] : [])),
+      http.post(`${repo}/pulls/:number/requested_reviewers`, ({ params }) => {
+        requested.push(Number(params.number))
+        return json({}, 201)
+      }),
+      utils.repoHasLabels(['approved', 'lgtm']),
+    )
+    servePulls(specs)
+    return { requested, timelineReads }
+  }
+
+  it('requests on a pull request opened an hour ago that nobody touched since, outside the normal window', async () => {
+    const { requested } = serve([{ number: 1, created: ago(70 * 60_000), updated: ago(70 * 60_000) }])
+
+    await expect(sweep(context)).resolves.toEqual({ candidates: [], merged: [], enqueued: [], failures: [] })
+    expect(requested).toEqual([1])
+  })
+
+  it('does not request before the delay has passed, nor after the due window', async () => {
+    const { requested, timelineReads } = serve([
+      { number: 1, created: ago(30 * 60_000), updated: ago(30 * 60_000) },
+      { number: 2, created: ago(3 * hour), updated: ago(90 * 60_000) },
+    ])
+
+    await expect(sweep(context)).resolves.toMatchObject({ failures: [] })
+    expect(requested).toEqual([])
+    // #1 is not due yet: no timeline read; #2 was opened long ago and never marked ready
+    expect(timelineReads).toEqual([2])
+  })
+
+  it('leaves alone a pull request with a requested reviewer, a requested team, a review or that is a draft', async () => {
+    const { requested } = serve([
+      { number: 1, created: ago(70 * 60_000), updated: ago(70 * 60_000), requested_reviewers: ['bob'] },
+      { number: 2, created: ago(70 * 60_000), updated: ago(70 * 60_000), requested_teams: ['sdk'] },
+      { number: 3, created: ago(70 * 60_000), updated: ago(70 * 60_000) },
+      { number: 4, created: ago(70 * 60_000), updated: ago(70 * 60_000), draft: true },
+    ], {}, [3])
+
+    await expect(sweep(context)).resolves.toMatchObject({ failures: [] })
+    expect(requested).toEqual([])
+  })
+
+  it('times a former draft from its last ready_for_review', async () => {
+    const { requested } = serve([
+      { number: 1, created: ago(5 * hour), updated: ago(70 * 60_000) },
+      { number: 2, created: ago(70 * 60_000), updated: ago(10 * 60_000) },
+    ], {
+      1: [ago(4 * hour), ago(70 * 60_000)],
+      2: [ago(10 * 60_000)],
+    })
+
+    await expect(sweep(context)).resolves.toMatchObject({ candidates: [2], failures: [] })
+    expect(requested).toEqual([1])
+  })
+})
+
 describe('sweep and the ok-to-test label', () => {
   function serveRuns(runs: unknown[], approved: number[]) {
     server.use(

__tests__/plugins/blunderbuss.test.ts

@@ -122,9 +122,14 @@ describe('blunderbussSettings', () => {
       exclude_approvers: false,
       ignore_drafts: true,
       ignore_authors: [],
+      delay: 0,
     })
   })
 
+  it('reads delay as milliseconds', () => {
+    expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: { delay: '1h30m' }, sources: [] }).delay).toBe(90 * 60_000)
+  })
+
   it('lowercases ignore_authors', () => {
     expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: { ignore_authors: ['Dependabot[bot]'] }, sources: [] }).ignore_authors).toEqual(['dependabot[bot]'])
   })
@@ -225,6 +230,18 @@ describe('blunderbuss handler', () => {
     expect(debug).toHaveBeenCalledWith('blunderbuss: #1 is a draft, waiting for ready_for_review')
   })
 
+  it.each(['opened', 'ready_for_review'])('delay: %s requests nobody and leaves it to the sweep job', async (action) => {
+    serveConfig('blunderbuss:\n  delay: 1h\n')
+    servePr(owners, ['sdk/x.go'])
+    const observe = observeRequestReviewers()
+    const info = vi.spyOn(core, 'info').mockImplementation(() => {})
+
+    await blunderbuss(prEvent(action), first)
+
+    await expect(observe.notCalled()).resolves.toBe('not called')
+    expect(info).toHaveBeenCalledWith(`blunderbuss: #1 ${action}; blunderbuss.delay is set, so the sweep job requests reviewers once it is due and nobody has been asked`)
+  })
+
   it('ready_for_review: requests reviewers for the former draft', async () => {
     serveConfig()
     servePr(owners, ['sdk/x.go'])