feat(Build Cop): handle flaky tests (#285)

4dd6255
commit 4dd6255
Author: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
Date:   Fri Feb 14 19:37:55 2020 -0500

    feat(Build Cop): handle flaky tests (#285)

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

synth.metadata

@@ -1,5 +1,5 @@
 {
- "updateTime": "2020-04-01T13:36:07.404475Z",
+ "updateTime": "2020-04-01T13:36:09.541251Z",
  "sources": [
  {
  "git": {