feat(Build Cop): handle flaky tests

If the bot reopens an issue, it marks it as flaky then stops commenting
on it and will not close it.

If a flaky test is closed (by a human) then fails again, it will be reopened.

Fixes #281.

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'] = {

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,41 +178,62 @@ 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}`
         );
         await context.github.issues.update({
           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({
           owner,
           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}`);
     }
   }
 };
 
-// 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,

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'),