Improve autofixing (#844)

1. Skip when ESLint reports no fixable issues
2. Detect ESLint warnings instead of just errors

.changeset/brave-owls-join.md

@@ -1,5 +1,5 @@
 ---
-"skuba": patch
+'skuba': patch
 ---
 
 template/private-npm-package: Use `npm2` build agent queue

.changeset/six-worms-jump.md

@@ -0,0 +1,5 @@
+---
+'skuba': patch
+---
+
+lint: Detect and autofix ESLint warnings

.changeset/tasty-steaks-count.md

@@ -0,0 +1,5 @@
+---
+'skuba': patch
+---
+
+lint: Skip autofixing when ESLint reports no fixable issues

src/cli/adapter/eslint.ts

@@ -22,9 +22,10 @@ export interface ESLintResult {
 
 export interface ESLintOutput {
   errors: ESLintResult[];
-  warnings: ESLintResult[];
+  fixable: boolean;
   ok: boolean;
   output: string;
+  warnings: ESLintResult[];
 }
 
 export const runESLint = async (
@@ -81,9 +82,14 @@ export const runESLint = async (
 
   const errors: ESLintResult[] = [];
   const warnings: ESLintResult[] = [];
+  let fixable = false;
 
   for (const result of results) {
     const relativePath = path.relative(cwd, result.filePath);
+    if (result.fixableErrorCount + result.fixableWarningCount) {
+      fixable = true;
+    }
+
     if (result.errorCount) {
       errors.push({
         filePath: relativePath,
@@ -111,5 +117,5 @@ export const runESLint = async (
     logger.plain(output);
   }
 
-  return { ok, output, errors, warnings };
+  return { errors, fixable, ok, output, warnings };
 };

src/cli/lint/annotate/github/eslint.test.ts

@@ -24,6 +24,7 @@ it('should create failure annotations for ESLint errors', () => {
         ],
       },
     ],
+    fixable: false,
     ok: false,
     output: '',
     warnings: [],
@@ -49,6 +50,7 @@ it('should create failure annotations for ESLint errors', () => {
 it('should create warning annotations for ESLint warnings', () => {
   const eslintOutput: ESLintOutput = {
     errors: [],
+    fixable: false,
     ok: true,
     output: '',
     warnings: [
@@ -110,6 +112,7 @@ it('should create both failure and warning annotations for ESLint errors and war
         ],
       },
     ],
+    fixable: false,
     ok: false,
     output: '',
     warnings: [
@@ -181,6 +184,7 @@ it('should not specify columns when an annotation spans multiple lines', () => {
         ],
       },
     ],
+    fixable: false,
     ok: false,
     output: '',
     warnings: [],

src/cli/lint/annotate/github/index.test.ts

@@ -36,6 +36,7 @@ const eslintOutput: ESLintOutput = {
       ],
     },
   ],
+  fixable: false,
   ok: false,
   output: '',
   warnings: [],

src/cli/lint/autofix.test.ts

@@ -23,8 +23,10 @@ const stdout = () => {
   return `\n${result}`;
 };
 
-const expectAutofixCommit = () => {
-  expect(runESLint).toHaveBeenCalledTimes(1);
+const expectAutofixCommit = (
+  { eslint }: { eslint: boolean } = { eslint: true },
+) => {
+  expect(runESLint).toHaveBeenCalledTimes(eslint ? 1 : 0);
   expect(runPrettier).toHaveBeenCalledTimes(1);
   expect(Git.commitAllChanges).toHaveBeenCalledTimes(1);
 };
@@ -51,26 +53,28 @@ beforeEach(() => {
 afterEach(jest.resetAllMocks);
 
 describe('autofix', () => {
+  const params = { debug: false, eslint: true, prettier: true };
+
   it('bails on a non-CI environment', async () => {
     delete process.env.CI;
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectNoAutofix();
   });
 
   it('bails on the master branch', async () => {
     jest.mocked(Git.currentBranch).mockResolvedValue('master');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectNoAutofix();
   });
 
   it('bails on the main branch', async () => {
     jest.mocked(Git.currentBranch).mockResolvedValue('main');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectNoAutofix();
   });
@@ -80,7 +84,7 @@ describe('autofix', () => {
 
     jest.mocked(Git.currentBranch).mockResolvedValue('devel');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectNoAutofix();
   });
@@ -90,7 +94,7 @@ describe('autofix', () => {
 
     jest.mocked(Git.currentBranch).mockResolvedValue('beta');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectNoAutofix();
   });
@@ -101,15 +105,23 @@ describe('autofix', () => {
       .mocked(Git.getHeadCommitMessage)
       .mockResolvedValue('Run `skuba format`');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
+
+    expectNoAutofix();
+  });
+
+  it('bails on no fixable issues', async () => {
+    await expect(
+      autofix({ ...params, eslint: false, prettier: false }),
+    ).resolves.toBeUndefined();
 
     expectNoAutofix();
   });
 
   it('skips push on empty commit', async () => {
     jest.mocked(Git.commitAllChanges).mockResolvedValue(undefined);
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectAutofixCommit();
     expect(Git.push).not.toHaveBeenCalled();
@@ -131,7 +143,7 @@ describe('autofix', () => {
 
     jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectAutofixCommit();
 
@@ -151,7 +163,7 @@ describe('autofix', () => {
     jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
     jest.mocked(Git.currentBranch).mockResolvedValue('dev');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectAutofixCommit();
     expect(Git.push).toHaveBeenNthCalledWith(1, {
@@ -170,6 +182,58 @@ describe('autofix', () => {
     `);
   });
 
+  it('handles fixable issues from ESLint only', async () => {
+    jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
+    jest.mocked(Git.currentBranch).mockResolvedValue('dev');
+
+    await expect(
+      autofix({ ...params, eslint: true, prettier: false }),
+    ).resolves.toBeUndefined();
+
+    expectAutofixCommit();
+    expect(Git.push).toHaveBeenNthCalledWith(1, {
+      auth: { type: 'gitHubApp' },
+      dir: expect.any(String),
+      ref: 'commit-sha',
+      remoteRef: 'dev',
+    });
+
+    // We should run both ESLint and Prettier
+    expect(stdout()).toMatchInlineSnapshot(`
+      "
+
+      Trying to autofix with ESLint and Prettier...
+      Pushed fix commit commit-sha.
+      "
+    `);
+  });
+
+  it('handles fixable issues from Prettier only', async () => {
+    jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
+    jest.mocked(Git.currentBranch).mockResolvedValue('dev');
+
+    await expect(
+      autofix({ ...params, eslint: false, prettier: true }),
+    ).resolves.toBeUndefined();
+
+    expectAutofixCommit({ eslint: false });
+    expect(Git.push).toHaveBeenNthCalledWith(1, {
+      auth: { type: 'gitHubApp' },
+      dir: expect.any(String),
+      ref: 'commit-sha',
+      remoteRef: 'dev',
+    });
+
+    // We should only run Prettier
+    expect(stdout()).toMatchInlineSnapshot(`
+      "
+
+      Trying to autofix with Prettier...
+      Pushed fix commit commit-sha.
+      "
+    `);
+  });
+
   it('tolerates guard errors', async () => {
     const ERROR = new Error('badness!');
 
@@ -178,7 +242,7 @@ describe('autofix', () => {
 
     jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectAutofixCommit();
     expect(Git.push).toHaveBeenNthCalledWith(1, {
@@ -199,7 +263,7 @@ describe('autofix', () => {
   it('bails on commit error', async () => {
     jest.mocked(Git.commitAllChanges).mockRejectedValue(MOCK_ERROR);
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectAutofixCommit();
     expect(Git.push).not.toHaveBeenCalled();
@@ -219,7 +283,7 @@ describe('autofix', () => {
     jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
     jest.mocked(Git.push).mockRejectedValue(MOCK_ERROR);
 
-    await expect(autofix({ debug: false })).resolves.toBeUndefined();
+    await expect(autofix(params)).resolves.toBeUndefined();
 
     expectAutofixCommit();
     expect(Git.push).toHaveBeenNthCalledWith(1, {

src/cli/lint/autofix.ts

@@ -56,7 +56,18 @@ const shouldPush = async ({
   return true;
 };
 
-export const autofix = async (input: Pick<Input, 'debug'>): Promise<void> => {
+interface AutofixParameters {
+  debug: Input['debug'];
+
+  eslint: boolean;
+  prettier: boolean;
+}
+
+export const autofix = async (params: AutofixParameters): Promise<void> => {
+  if (!params.eslint && !params.prettier) {
+    return;
+  }
+
   const dir = process.cwd();
 
   let currentBranch;
@@ -68,14 +79,19 @@ export const autofix = async (input: Pick<Input, 'debug'>): Promise<void> => {
     return;
   }
 
-  // Naively try to autofix issues as we can't tell from ESLint output.
   try {
     log.newline();
-    log.warn(`Trying to autofix with ESLint and Prettier...`);
+    log.warn(
+      `Trying to autofix with ${params.eslint ? 'ESLint and ' : ''}Prettier...`,
+    );
 
-    const logger = createLogger(input.debug);
+    const logger = createLogger(params.debug);
 
-    await runESLint('format', logger);
+    if (params.eslint) {
+      await runESLint('format', logger);
+    }
+    // Unconditionally re-run Prettier; reaching here means we have pre-existing
+    // format violations or may have created new ones through ESLint fixes.
     await runPrettier('format', logger);
 
     const ref = await Git.commitAllChanges({

src/cli/lint/external.ts

@@ -96,25 +96,22 @@ export const externalLint = async (input: Input) => {
     log.subtle(inspect(err));
   }
 
-  if (eslint.ok && prettier.ok && tscOk) {
-    return;
-  }
-
-  const tools = [
-    ...(eslint.ok ? [] : ['ESLint']),
-    ...(prettier.ok ? [] : ['Prettier']),
-    ...(tscOk ? [] : ['tsc']),
-  ];
-
-  log.newline();
-  log.err(`${tools.join(', ')} found issues that require triage.`);
+  if (!eslint.ok || !prettier.ok || !tscOk) {
+    const tools = [
+      ...(eslint.ok ? [] : ['ESLint']),
+      ...(prettier.ok ? [] : ['Prettier']),
+      ...(tscOk ? [] : ['tsc']),
+    ];
 
-  process.exitCode = 1;
+    log.newline();
+    log.err(`${tools.join(', ')} found issues that require triage.`);
 
-  if (eslint.ok && prettier.ok) {
-    // If these are fine then the issue lies with tsc, which we can't autofix.
-    return;
+    process.exitCode = 1;
   }
 
-  await autofix(input);
+  await autofix({
+    debug: input.debug,
+    eslint: eslint.fixable,
+    prettier: !prettier.ok,
+  });
 };