Repository: intuit/auto
Pull request #746 (2019-12-02 20:54:36)

move git clean check to core
# What Changed

see title

# Why

seemed like this should be something any plugin benefits from


<!-- GITHUB_RELEASE PR BODY: canary-version -->
Published PR with canary version: `8.0.0-canary.746.9802.0`
<!-- GITHUB_RELEASE PR BODY: canary-version -->



Patch:
diff --git a/packages/core/src/auto.ts b/packages/core/src/auto.ts
index 09910db1..55d906a4 100644
--- a/packages/core/src/auto.ts
+++ b/packages/core/src/auto.ts
@@ -609,6 +609,8 @@ export default class Auto {
       process.exit(1);
     }
 
+    await this.checkClean();
+
     // SailEnv falls back to commit SHA
     let pr: string | undefined;
     let build: string | undefined;
@@ -713,6 +715,7 @@ export default class Auto {
       process.exit(1);
     }
 
+    await this.checkClean();
     await this.setGitUser();
 
     const lastTag = await this.git.getLatestTagInBranch();
@@ -862,6 +865,7 @@ export default class Auto {
     await this.makeChangelog(options);
 
     if (!options.dryRun) {
+      await this.checkClean();
       this.logger.verbose.info('Calling version hook');
       await this.hooks.version.promise(version);
       this.logger.verbose.info('Calling after version hook');
@@ -1079,6 +1083,22 @@ export default class Auto {
     return newVersion;
   }
 
+
+  /** Check if `git status` is clean. */
+  readonly checkClean = async () => {
+    const status = await execPromise('git', ['status', '--porcelain']);
+
+    if (!status) {
+      return;
+    }
+
+    this.logger.log.error('Changed Files:\n', status);
+
+    throw new Error(
+      'Working direction is not clean, make sure all files are commited'
+    );
+  }
+
   /** Prefix a version with a "v" if needed */
   readonly prefixRelease = (release: string) => {
     if (!this.release) {
diff --git a/plugins/npm/src/index.ts b/plugins/npm/src/index.ts
index bfaeb255..44f11696 100644
--- a/plugins/npm/src/index.ts
+++ b/plugins/npm/src/index.ts
@@ -186,23 +186,6 @@ interface INpmConfig {
 /** Parse the lerna.json file. */
 const getLernaJson = () => JSON.parse(fs.readFileSync('lerna.json', 'utf8'));
 
-/**
- * Check if `git status` is clean. We try to ensure that no
- * changes haven't been staged to match the behavior of NPM.
- */
-const checkClean = async (auto: Auto) => {
-  const status = await execPromise('git', ['status', '--porcelain']);
-
-  if (!status) {
-    return;
-  }
-
-  auto.logger.log.error('Changed Files:\n', status);
-  throw new Error(
-    'Working direction is not clean, make sure all files are commited'
-  );
-};
-
 /** Render a list of string in markdown */
 const markdownList = (lines: string[]) =>
   lines.map(line => `- \`${line}\``).join('\n');
@@ -450,8 +433,6 @@ export default class NPMPlugin implements IPlugin {
     );
 
     auto.hooks.version.tapPromise(this.name, async version => {
-      await checkClean(auto);
-
       if (isMonorepo()) {
         auto.logger.verbose.info('Detected monorepo, using lerna');
         const isIndependent = getLernaJson().version === 'independent';
@@ -488,8 +469,6 @@ export default class NPMPlugin implements IPlugin {
     });
 
     auto.hooks.canary.tapPromise(this.name, async (version, postFix) => {
-      await checkClean(auto);
-
       if (this.setRcToken) {
         await setTokenOnCI(auto.logger);
         auto.logger.verbose.info('Set CI NPM_TOKEN');
@@ -578,8 +557,6 @@ export default class NPMPlugin implements IPlugin {
     });
 
     auto.hooks.next.tapPromise(this.name, async (preReleaseVersions, bump) => {
-      await checkClean(auto);
-
       if (this.setRcToken) {
         await setTokenOnCI(auto.logger);
         auto.logger.verbose.info('Set CI NPM_TOKEN');


Test patch:
diff --git a/packages/core/src/__tests__/auto-canary-in-pr-ci.test.ts b/packages/core/src/__tests__/auto-canary-in-pr-ci.test.ts
index 05b1c9c5..a614bc36 100644
--- a/packages/core/src/__tests__/auto-canary-in-pr-ci.test.ts
+++ b/packages/core/src/__tests__/auto-canary-in-pr-ci.test.ts
@@ -20,6 +20,9 @@ const defaults = {
 describe('canary in ci', () => {
   test('calls the canary hook with the canary version', async () => {
     const auto = new Auto({ ...defaults, plugins: [] });
+    // @ts-ignore
+    auto.checkClean = () => Promise.resolve(true);
+  
     auto.logger = dummyLog();
     await auto.loadConfig();
     auto.release!.getCommitsInRelease = () =>
@@ -36,6 +39,9 @@ describe('canary in ci', () => {
 
   test('comments on PR in CI', async () => {
     const auto = new Auto({ ...defaults, plugins: [] });
+    // @ts-ignore
+    auto.checkClean = () => Promise.resolve(true);
+  
     auto.logger = dummyLog();
     await auto.loadConfig();
     auto.git!.getLatestRelease = () => Promise.resolve('1.2.3');
@@ -53,6 +59,9 @@ describe('canary in ci', () => {
 
   test('should fail when canaries not implemented', async () => {
     const auto = new Auto({ ...defaults, plugins: [] });
+    // @ts-ignore
+    auto.checkClean = () => Promise.resolve(true);
+  
 
     // @ts-ignore
     jest.spyOn(process, 'exit').mockImplementationOnce(() => {});
@@ -71,6 +80,9 @@ describe('canary in ci', () => {
 
   test('should not comment when passed "false"', async () => {
     const auto = new Auto({ ...defaults, plugins: [] });
+    // @ts-ignore
+    auto.checkClean = () => Promise.resolve(true);
+  
     auto.logger = dummyLog();
     await auto.loadConfig();
     auto.git!.getLatestRelease = () => Promise.resolve('1.2.3');
@@ -87,6 +99,9 @@ describe('canary in ci', () => {
 
   test('can override pr and build', async () => {
     const auto = new Auto({ ...defaults, plugins: [] });
+    // @ts-ignore
+    auto.checkClean = () => Promise.resolve(true);
+  
     auto.logger = dummyLog();
     await auto.loadConfig();
     auto.release!.getCommitsInRelease = () =>
@@ -104,6 +119,9 @@ describe('canary in ci', () => {
 describe('shipit in ci', () => {
   test('should publish canary in PR', async () => {
     const auto = new Auto({ ...defaults, plugins: [] });
+    // @ts-ignore
+    auto.checkClean = () => Promise.resolve(true);
+  
     auto.logger = dummyLog();
     await auto.loadConfig();
 
diff --git a/packages/core/src/__tests__/auto-canary-local.test.ts b/packages/core/src/__tests__/auto-canary-local.test.ts
index c6f6de11..1d2c15b5 100644
--- a/packages/core/src/__tests__/auto-canary-local.test.ts
+++ b/packages/core/src/__tests__/auto-canary-local.test.ts
@@ -15,6 +15,8 @@ const defaults = {
 test('shipit should publish canary in locally when not on master', async () => {
   const auto = new Auto({ ...defaults, plugins: [] });
   auto.logger = dummyLog();
+  // @ts-ignore
+  auto.checkClean = () => Promise.resolve(true);
   await auto.loadConfig();
 
   auto.git!.getLatestRelease = () => Promise.resolve('1.2.3');
diff --git a/packages/core/src/__tests__/auto.test.ts b/packages/core/src/__tests__/auto.test.ts
index 9924495e..08610f68 100644
--- a/packages/core/src/__tests__/auto.test.ts
+++ b/packages/core/src/__tests__/auto.test.ts
@@ -924,6 +924,8 @@ describe('Auto', () => {
   describe('canary', () => {
     test('should throw when not initialized', async () => {
       const auto = new Auto(defaults);
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
 
       await expect(auto.canary()).rejects.not.toBeUndefined();
@@ -931,6 +933,9 @@ describe('Auto', () => {
 
     test('does not call canary hook in dry-run', async () => {
       const auto = new Auto(defaults);
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
+   
       auto.logger = dummyLog();
       await auto.loadConfig();
       auto.git!.getLatestRelease = () => Promise.resolve('1.2.3');
@@ -946,6 +951,9 @@ describe('Auto', () => {
 
     test('calls the canary hook with the pr info', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
+   
       auto.logger = dummyLog();
       await auto.loadConfig();
       auto.git!.getLatestRelease = () => Promise.resolve('1.2.3');
@@ -964,6 +972,8 @@ describe('Auto', () => {
 
     test('falls back to first commit', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
       auto.git!.getLatestTagInBranch = () => {
@@ -989,6 +999,8 @@ describe('Auto', () => {
 
     test('adds sha if no pr or build number is found', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
       auto.git!.getLatestRelease = () => Promise.resolve('1.2.3');
@@ -1006,6 +1018,8 @@ describe('Auto', () => {
 
     test("doesn't comment if there is an error", async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
       jest.spyOn(auto, 'prBody').mockImplementation();
@@ -1025,6 +1039,8 @@ describe('Auto', () => {
 
     test('defaults to sha when run locally', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
 
@@ -1041,6 +1057,8 @@ describe('Auto', () => {
 
     test('works when PR has "skip-release" label', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
 
@@ -1063,6 +1081,8 @@ describe('Auto', () => {
   describe('shipit', () => {
     test('should throw when not initialized', async () => {
       const auto = new Auto(defaults);
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
 
       await expect(auto.shipit()).rejects.not.toBeUndefined();
@@ -1070,6 +1090,8 @@ describe('Auto', () => {
 
     test('should not publish when no latest version found', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
 
@@ -1085,6 +1107,8 @@ describe('Auto', () => {
 
     test('should publish to latest on base branch', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
 
@@ -1104,6 +1128,8 @@ describe('Auto', () => {
 
     test('should skip publish in dry run', async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
+      // @ts-ignore
+      auto.checkClean = () => Promise.resolve(true);
       auto.logger = dummyLog();
       await auto.loadConfig();
 
@@ -1118,6 +1144,8 @@ describe('Auto', () => {
       await auto.shipit({ dryRun: true });
       expect(version).not.toHaveBeenCalled();
     });
+
+
   });
 });
 
diff --git a/plugins/npm/__tests__/npm.test.ts b/plugins/npm/__tests__/npm.test.ts
index fefd0775..df2e4480 100644
--- a/plugins/npm/__tests__/npm.test.ts
+++ b/plugins/npm/__tests__/npm.test.ts
@@ -333,30 +333,7 @@ describe('publish', () => {
     exec.mockClear();
   });
 
-  test('throws when working dir not clean', async () => {
-    const plugin = new NPMPlugin();
-    const hooks = makeHooks();
 
-    plugin.apply(({
-      config: { prereleaseBranches: ['next'] },
-      hooks,
-      logger: dummyLog(),
-      getCurrentVersion: () => '1.2.3',
-      git: { getLatestRelease: () => '1.2.3' }
-    } as unknown) as Auto.Auto);
-
-    readResult = `
-      {
-        "name": "test"
-      }
-    `;
-
-    exec.mockReturnValueOnce('m Somefile.txt');
-
-    await expect(
-      hooks.version.promise(Auto.SEMVER.patch)
-    ).rejects.toBeInstanceOf(Error);
-  });
 
   test('should use silly logging in verbose mode', async () => {
     const plugin = new NPMPlugin();
@@ -509,7 +486,6 @@ describe('publish', () => {
       logger: dummyLog()
     } as Auto.Auto);
 
-    exec.mockReturnValueOnce('');
     exec.mockReturnValueOnce('1.0.0');
 
     readResult = `
@@ -540,7 +516,6 @@ describe('publish', () => {
 
     existsSync.mockReturnValueOnce(true);
     monorepoPackages.mockReturnValueOnce(monorepoPackagesResult);
-    exec.mockReturnValueOnce('');
     exec.mockReturnValueOnce('1.0.0');
 
     readResult = `
@@ -550,7 +525,7 @@ describe('publish', () => {
     `;
 
     await hooks.version.promise(Auto.SEMVER.patch);
-    expect(exec).toHaveBeenNthCalledWith(3, 'npx', [
+    expect(exec).toHaveBeenNthCalledWith(2, 'npx', [
       'lerna',
       'version',
       '1.0.1',
@@ -610,31 +585,6 @@ describe('canary', () => {
     exec.mockClear();
   });
 
-  test('throws when working dir not clean', async () => {
-    const plugin = new NPMPlugin();
-    const hooks = makeHooks();
-
-    plugin.apply(({
-      config: { prereleaseBranches: ['next'] },
-      hooks,
-      logger: dummyLog(),
-      getCurrentVersion: () => '1.2.3',
-      git: { getLatestRelease: () => '1.2.3' }
-    } as unknown) as Auto.Auto);
-
-    readResult = `
-      {
-        "name": "test"
-      }
-    `;
-
-    exec.mockReturnValueOnce('m Somefile.txt');
-
-    await expect(
-      hooks.canary.promise(Auto.SEMVER.patch, '.123.1')
-    ).rejects.toBeInstanceOf(Error);
-  });
-
   test('use npm for normal package', async () => {
     const plugin = new NPMPlugin();
     const hooks = makeHooks();
@@ -654,8 +604,8 @@ describe('canary', () => {
     `;
 
     await hooks.canary.promise(Auto.SEMVER.patch, '.123.1');
-    expect(exec.mock.calls[1]).toContain('npm');
-    expect(exec.mock.calls[1][1]).toContain('1.2.4-canary.123.1');
+    expect(exec.mock.calls[0]).toContain('npm');
+    expect(exec.mock.calls[0][1]).toContain('1.2.4-canary.123.1');
   });
 
   test('publishes to public for scoped packages', async () => {
@@ -677,7 +627,7 @@ describe('canary', () => {
     `;
 
     await hooks.canary.promise(Auto.SEMVER.patch, '');
-    expect(exec.mock.calls[2][1]).toContain('public');
+    expect(exec.mock.calls[1][1]).toContain('public');
   });
 
   test('use lerna for monorepo package', async () => {
@@ -697,7 +647,7 @@ describe('canary', () => {
         "name": "test"
       }
     `;
-    exec.mockReturnValueOnce('');
+
     exec.mockReturnValue(
       Promise.resolve(
         `path/to/package:@foo/app:1.2.3-canary.0+abcd\npath/to/package:@foo/lib:1.2.3-canary.0+abcd`
@@ -726,7 +676,6 @@ describe('canary', () => {
         "name": "test"
       }
     `;
-    exec.mockReturnValueOnce('');
     exec.mockReturnValue(
       Promise.resolve(
         `path/to/package:@foo/app:1.2.3\npath/to/package:@foo/lib:1.2.3`
@@ -756,7 +705,6 @@ describe('canary', () => {
         "name": "test"
       }
     `;
-    exec.mockReturnValueOnce('');
     exec.mockReturnValue(
       Promise.resolve(
         'path/to/package:@foo/app:1.2.4-canary.0\npath/to/package:@foo/lib:1.1.0-canary.0'
@@ -764,7 +712,7 @@ describe('canary', () => {
     );
 
     await hooks.version.promise(Auto.SEMVER.patch);
-    expect(exec).toHaveBeenNthCalledWith(2, 'npx', [
+    expect(exec).toHaveBeenNthCalledWith(1, 'npx', [
       'lerna',
       'version',
       'patch',
@@ -795,7 +743,6 @@ describe('canary', () => {
         "name": "test"
       }
     `;
-    exec.mockReturnValueOnce('');
     exec.mockReturnValue(
       Promise.resolve(
         'path/to/package:@foo/app:1.2.4\npath/to/package:@foo/lib:1.1.0'