diff --git a/packages/buildcop/__snapshots__/buildcop.js b/packages/buildcop/__snapshots__/buildcop.js index a84ff8f87e7..12926434f07 100644 --- a/packages/buildcop/__snapshots__/buildcop.js +++ b/packages/buildcop/__snapshots__/buildcop.js @@ -47,11 +47,17 @@ exports['buildcop app xunitXML comments on existing issue 1'] = { } exports['buildcop app xunitXML reopens issue for failing test 1'] = { + "labels": [ + "type: bug", + "priority: p1", + "buildcop:issue", + "buildcop: flaky" + ], "state": "open" } exports['buildcop app xunitXML reopens issue for failing test 2'] = { - "body": "spanner/spanner_snippets: TestSample failed\nbuildID: 123\nbuildURL: http://example.com\nstatus: failed" + "body": "Oops! Looks like this issue is still flaky. :grimace:\n\nI reopened the issue, but a human will need to close it again.\n\nspanner/spanner_snippets: TestSample failed\nbuildID: 123\nbuildURL: http://example.com\nstatus: failed" } exports['buildcop app xunitXML closes an issue for a passing test [Go] 1'] = { diff --git a/packages/buildcop/src/buildcop.ts b/packages/buildcop/src/buildcop.ts index 028f2b18310..3e1df66e3e2 100644 --- a/packages/buildcop/src/buildcop.ts +++ b/packages/buildcop/src/buildcop.ts @@ -32,11 +32,31 @@ import { GitHubAPI } from 'probot/lib/github'; import xmljs from 'xml-js'; import Octokit from '@octokit/rest'; -const BUILDCOP_LABELS = 'buildcop:issue'; -const LABELS = 'type: bug,priority: p1'; +// TODO: Consider adding a space between : and issue. We would have to update +// all existing issues with the new label (could be a one-time operation) +// and update tests. +const ISSUE_LABEL = 'buildcop:issue'; +const FLAKY_LABEL = 'buildcop: flaky'; +const BUG_LABELS = 'type: bug,priority: p1'; + +const LABELS_FOR_FLAKY_ISSUE = BUG_LABELS.split(',').concat([ + ISSUE_LABEL, + FLAKY_LABEL, +]); +const LABELS_FOR_NEW_ISSUE = BUG_LABELS.split(',').concat([ISSUE_LABEL]); const EVERYTHING_FAILED_TITLE = 'The build failed'; +const FLAKY_MESSAGE = `Looks like this issue is flaky. :worried: + +I'm going to leave this open and stop commenting. + +A human should fix and close this.`; + +const FLAKY_AGAIN_MESSAGE = `Oops! Looks like this issue is still flaky. :grimace: + +I reopened the issue, but a human will need to close it again.`; + interface TestCase { package?: string; testCase?: string; @@ -97,12 +117,12 @@ export function buildcop(app: Application) { owner, repo, per_page: 32, - labels: BUILDCOP_LABELS, + labels: ISSUE_LABEL, state: 'all', // Include open and closed issues. }) ).data; - // Open issues for failing tests. + // Open issues for failing tests (including flaky tests). await buildcop.openIssues( results.failures, issues, @@ -112,7 +132,7 @@ export function buildcop(app: Application) { buildID, buildURL ); - // Close issues for passing tests. + // Close issues for passing tests (unless they're flaky). await buildcop.closeIssues( results, issues, @@ -158,6 +178,8 @@ buildcop.openIssues = async ( `[${owner}/${repo}] existing issue #${existingIssue.number}: state: ${existingIssue.state}` ); if (existingIssue.state === 'closed') { + // If the issue is closed, we know the bot opened and closed it in the + // past. So, this is probably a flaky test. A human should close it. context.log.info( `[${owner}/${repo}] reopening issue #${existingIssue.number}` ); @@ -165,17 +187,36 @@ buildcop.openIssues = async ( owner, repo, issue_number: existingIssue.number, + labels: LABELS_FOR_FLAKY_ISSUE, state: 'open', }); + let body = FLAKY_MESSAGE; + // If the issue was flaky and we reopen it (again), say so. + if (buildcop.isFlaky(existingIssue)) { + body = FLAKY_AGAIN_MESSAGE; + } + body = body + '\n\n' + buildcop.formatBody(failure, buildID, buildURL); + await context.github.issues.createComment({ + owner, + repo, + issue_number: existingIssue.number, + body, + }); + } else { + // TODO: this can be spammy (https://github.com/googleapis/repo-automation-bots/issues/282). + + // Don't comment if it's flaky. + if (buildcop.isFlaky(existingIssue)) { + return; + } + + await context.github.issues.createComment({ + owner, + repo, + issue_number: existingIssue.number, + body: buildcop.formatBody(failure, buildID, buildURL), + }); } - // TODO: Make this comment say something nice about reopening the - // issue? - await context.github.issues.createComment({ - owner, - repo, - issue_number: existingIssue.number, - body: buildcop.formatBody(failure, buildID, buildURL), - }); } else { const newIssue = ( await context.github.issues.create({ @@ -183,7 +224,7 @@ buildcop.openIssues = async ( repo, title: buildcop.formatTestCase(failure), body: buildcop.formatBody(failure, buildID, buildURL), - labels: LABELS.split(',').concat(BUILDCOP_LABELS.split(',')), + labels: LABELS_FOR_NEW_ISSUE, }) ).data; context.log.info(`[${owner}/${repo}]: created issue #${newIssue.number}`); @@ -191,8 +232,8 @@ buildcop.openIssues = async ( } }; -// For every buildcop issue, if it passed and it didn't previously fail in the -// same build, close it. +// For every buildcop issue, if it's not flaky and it passed and it didn't +// previously fail in the same build, close it. buildcop.closeIssues = async ( results: TestResults, issues: Octokit.IssuesListForRepoResponseItem[], @@ -206,6 +247,12 @@ buildcop.closeIssues = async ( if (issue.state === 'closed') { continue; } + + // Don't close flaky issues. + if (buildcop.isFlaky(issue)) { + continue; + } + const failure = results.failures.find(failure => { return issue.title === buildcop.formatTestCase(failure); }); @@ -264,6 +311,18 @@ buildcop.closeIssues = async ( } }; +buildcop.isFlaky = (issue: Octokit.IssuesListForRepoResponseItem): boolean => { + if (issue.labels === undefined) { + return false; + } + for (const label of issue.labels) { + if (label.toString() === FLAKY_LABEL) { + return true; + } + } + return false; +}; + buildcop.formatBody = ( failure: TestCase, buildID: string, diff --git a/packages/buildcop/test/buildcop.ts b/packages/buildcop/test/buildcop.ts index 994d186a925..93900a37b75 100644 --- a/packages/buildcop/test/buildcop.ts +++ b/packages/buildcop/test/buildcop.ts @@ -359,6 +359,43 @@ describe('buildcop', () => { requests.done(); }); + it('does not comment about failure on existing flaky issue', async () => { + const input = fs.readFileSync( + resolve(fixturesPath, 'testdata', 'one_failed.xml'), + 'utf8' + ); + const payload = formatPayload({ + repo: 'tbpg/golang-samples', + organization: { login: 'tbpg' }, + repository: { name: 'golang-samples' }, + buildID: '123', + buildURL: 'http://example.com', + xunitXML: input, + }); + + const requests = nock('https://api.github.com') + .get( + '/repos/tbpg/golang-samples/issues?per_page=32&labels=buildcop%3Aissue&state=all' + ) + .reply(200, [ + { + title: formatTestCase({ + package: + 'github.com/GoogleCloudPlatform/golang-samples/spanner/spanner_snippets', + testCase: 'TestSample', + }), + number: 16, + body: `Failure!`, + labels: ['buildcop: flaky'], + state: 'open', + }, + ]); + + await probot.receive({ name: 'pubsub.message', payload, id: 'abc123' }); + + requests.done(); + }); + it('handles a testsuite with no test cases', async () => { const input = fs.readFileSync( resolve(fixturesPath, 'testdata', 'no_tests.xml'), @@ -411,6 +448,7 @@ describe('buildcop', () => { }), number: 16, body: 'Failure!', + labels: ['buildcop: flaky'], state: 'closed', }, ]) @@ -632,6 +670,42 @@ describe('buildcop', () => { requests.done(); }); + it('keeps an issue open for a passing flaky test', async () => { + const input = fs.readFileSync( + resolve(fixturesPath, 'testdata', 'passed.xml'), + 'utf8' + ); + const payload = formatPayload({ + repo: 'tbpg/golang-samples', + organization: { login: 'tbpg' }, + repository: { name: 'golang-samples' }, + buildID: '123', + buildURL: 'http://example.com', + xunitXML: input, + }); + + const requests = nock('https://api.github.com') + .get( + '/repos/tbpg/golang-samples/issues?per_page=32&labels=buildcop%3Aissue&state=all' + ) + .reply(200, [ + { + title: formatTestCase({ + package: + 'github.com/GoogleCloudPlatform/golang-samples/spanner/spanner_snippets', + testCase: 'TestSample', + }), + number: 16, + body: `Failure!`, + labels: ['buildcop: flaky'], + }, + ]); + + await probot.receive({ name: 'pubsub.message', payload, id: 'abc123' }); + + requests.done(); + }); + it('opens multiple issues for multiple failures', async () => { const input = fs.readFileSync( resolve(fixturesPath, 'testdata', 'many_failed_same_pkg.xml'),