fix(build): ensure to check spawn result and assume files unchanged M…

…ONGOSH-521 (#599)

* ensure to check spawn result and assume files unchanged

- uses a helper function to ensure spawn.sync calls throw if not successful
- before lerna publish marks known changed files as assume-unchanged and reverts after publish

Signed-off-by: Michael Rose <michael_rose@gmx.de>

* increase test coverage

* fixup: increase test coverage

packages/build/src/npm-packages.spec.ts

@@ -1,13 +1,203 @@
 import { expect } from 'chai';
-import { listNpmPackages } from './npm-packages';
-
-describe('listNpmPackages', () => {
-  it('lists packages', () => {
-    const packages = listNpmPackages();
-    expect(packages.length).to.be.greaterThan(1);
-    for (const { name, version } of packages) {
-      expect(name).to.be.a('string');
-      expect(version).to.be.a('string');
-    }
+import path from 'path';
+import sinon, { SinonStub } from 'sinon';
+import {
+  bumpNpmPackages,
+  listNpmPackages,
+  markBumpedFilesAsAssumeUnchanged,
+  publishNpmPackages,
+  spawnSync
+} from './npm-packages';
+
+
+describe('npm-packages', () => {
+  describe('spawnSync', () => {
+    it('works for a valid command', () => {
+      const result = spawnSync('bash', ['-c', 'echo -n works'], { encoding: 'utf8' });
+      expect(result.status).to.equal(0);
+      expect(result.stdout).to.equal('works');
+    });
+
+    it('throws on ENOENT error', () => {
+      try {
+        spawnSync('notaprogram', [], { encoding: 'utf8' });
+      } catch (e) {
+        return expect(e).to.not.be.undefined;
+      }
+      expect.fail('Expected error');
+    });
+
+    it('throws on non-zero exit code', () => {
+      try {
+        spawnSync('bash', ['-c', 'exit 1'], { encoding: 'utf8' });
+      } catch (e) {
+        return expect(e).to.not.be.undefined;
+      }
+      expect.fail('Expected error');
+    });
+  });
+
+  describe('bumpNpmPackages', () => {
+    let spawnSync: SinonStub;
+
+    beforeEach(() => {
+      spawnSync = sinon.stub();
+    });
+
+    it('does not do anything if no version or placeholder version is specified', () => {
+      bumpNpmPackages('', spawnSync);
+      bumpNpmPackages('0.0.0-dev.0', spawnSync);
+      expect(spawnSync).to.not.have.been.called;
+    });
+
+    it('calls lerna to bump package version', () => {
+      const lernaBin = path.resolve(__dirname, '..', '..', '..', 'node_modules', '.bin', 'lerna');
+      bumpNpmPackages('0.7.0', spawnSync);
+      expect(spawnSync).to.have.been.calledWith(
+        lernaBin,
+        ['version', '0.7.0', '--no-changelog', '--no-push', '--exact', '--no-git-tag-version', '--force-publish', '--yes'],
+        sinon.match.any
+      );
+    });
+  });
+
+  describe('publishNpmPackages', () => {
+    let listNpmPackages: SinonStub;
+    let markBumpedFilesAsAssumeUnchanged: SinonStub;
+    let spawnSync: SinonStub;
+
+    beforeEach(() => {
+      listNpmPackages = sinon.stub();
+      markBumpedFilesAsAssumeUnchanged = sinon.stub();
+      spawnSync = sinon.stub();
+    });
+
+    it('fails if packages have different versions', () => {
+      listNpmPackages.returns([
+        { name: 'packageA', version: '0.0.1' },
+        { name: 'packageB', version: '0.0.2' }
+      ]);
+      try {
+        publishNpmPackages(
+          listNpmPackages,
+          markBumpedFilesAsAssumeUnchanged,
+          spawnSync
+        );
+      } catch (e) {
+        expect(markBumpedFilesAsAssumeUnchanged).to.not.have.been.called;
+        expect(spawnSync).to.not.have.been.called;
+        return;
+      }
+      expect.fail('Expected error');
+    });
+
+    it('fails if packages have placeholder versions', () => {
+      listNpmPackages.returns([
+        { name: 'packageA', version: '0.0.0-dev.0' },
+        { name: 'packageB', version: '0.0.0-dev.0' }
+      ]);
+      try {
+        publishNpmPackages(
+          listNpmPackages,
+          markBumpedFilesAsAssumeUnchanged,
+          spawnSync
+        );
+      } catch (e) {
+        expect(markBumpedFilesAsAssumeUnchanged).to.not.have.been.called;
+        expect(spawnSync).to.not.have.been.called;
+        return;
+      }
+      expect.fail('Expected error');
+    });
+
+    it('calls lerna to publish packages for a real version', () => {
+      const lernaBin = path.resolve(__dirname, '..', '..', '..', 'node_modules', '.bin', 'lerna');
+      const packages = [
+        { name: 'packageA', version: '0.7.0' }
+      ];
+      listNpmPackages.returns(packages);
+
+      publishNpmPackages(
+        listNpmPackages,
+        markBumpedFilesAsAssumeUnchanged,
+        spawnSync
+      );
+
+      expect(markBumpedFilesAsAssumeUnchanged).to.have.been.calledWith(packages, true);
+      expect(spawnSync).to.have.been.calledWith(
+        lernaBin,
+        ['publish', 'from-package', '--no-changelog', '--no-push', '--exact', '--no-git-tag-version', '--force-publish', '--yes'],
+        sinon.match.any
+      );
+      expect(markBumpedFilesAsAssumeUnchanged).to.have.been.calledWith(packages, false);
+    });
+
+    it('reverts the assume unchanged even on spawn failure', () => {
+      const packages = [
+        { name: 'packageA', version: '0.7.0' }
+      ];
+      listNpmPackages.returns(packages);
+      spawnSync.throws(new Error('meeep'));
+
+      try {
+        publishNpmPackages(
+          listNpmPackages,
+          markBumpedFilesAsAssumeUnchanged,
+          spawnSync
+        );
+      } catch (e) {
+        expect(markBumpedFilesAsAssumeUnchanged).to.have.been.calledWith(packages, true);
+        expect(spawnSync).to.have.been.called;
+        expect(markBumpedFilesAsAssumeUnchanged).to.have.been.calledWith(packages, false);
+        return;
+      }
+      expect.fail('Expected error');
+    });
+  });
+
+  describe('listNpmPackages', () => {
+    it('lists packages', () => {
+      const packages = listNpmPackages();
+      expect(packages.length).to.be.greaterThan(1);
+      for (const { name, version } of packages) {
+        expect(name).to.be.a('string');
+        expect(version).to.be.a('string');
+      }
+    });
+  });
+
+  describe('markBumpedFilesAsAssumeUnchanged', () => {
+    let packages: { name: string; version: string }[];
+    let expectedFiles: string[];
+    let spawnSync: SinonStub;
+
+    beforeEach(() => {
+      expectedFiles = ['.npmrc', 'lerna.json'];
+      packages = listNpmPackages();
+      packages.forEach(({ name }) => {
+        expectedFiles.push(`packages/${name}/package.json`);
+        expectedFiles.push(`packages/${name}/package-lock.json`);
+      });
+
+      spawnSync = sinon.stub();
+    });
+
+    it('marks files with --assume-unchanged', () => {
+      markBumpedFilesAsAssumeUnchanged(packages, true, spawnSync);
+      expectedFiles.forEach(f => {
+        expect(spawnSync).to.have.been.calledWith(
+          'git', ['update-index', '--assume-unchanged', f], sinon.match.any
+        );
+      });
+    });
+
+    it('marks files with --no-assume-unchanged', () => {
+      markBumpedFilesAsAssumeUnchanged(packages, false, spawnSync);
+      expectedFiles.forEach(f => {
+        expect(spawnSync).to.have.been.calledWith(
+          'git', ['update-index', '--no-assume-unchanged', f], sinon.match.any
+        );
+      });
+    });
   });
 });

packages/build/src/npm-packages.ts

@@ -1,18 +1,38 @@
+import { SpawnSyncOptionsWithStringEncoding, SpawnSyncReturns } from 'child_process';
 import * as spawn from 'cross-spawn';
 import path from 'path';
 
 const PLACEHOLDER_VERSION = '0.0.0-dev.0';
 const PROJECT_ROOT = path.resolve(__dirname, '..', '..', '..');
 const LERNA_BIN = path.resolve(PROJECT_ROOT, 'node_modules', '.bin', 'lerna');
 
-export function bumpNpmPackages(version: string): void {
+export function spawnSync(command: string, args: string[], options: SpawnSyncOptionsWithStringEncoding): SpawnSyncReturns<string> {
+  const result = spawn.sync(command, args, options);
+  if (result.error) {
+    console.error('spawn.sync returned error', result.error);
+    console.error(result.stdout);
+    console.error(result.stderr);
+    throw new Error(`Failed to spawn ${command}, args: ${args.join(',')}: ${result.error}`);
+  } else if (result.status !== 0) {
+    console.error('spawn.sync exited with non-zero', result.status);
+    console.error(result.stdout);
+    console.error(result.stderr);
+    throw new Error(`Spawn exited non-zero for ${command}, args: ${args.join(',')}: ${result.status}`);
+  }
+  return result;
+}
+
+export function bumpNpmPackages(
+  version: string,
+  spawnSyncFn: typeof spawnSync = spawnSync
+): void {
   if (!version || version === PLACEHOLDER_VERSION) {
     console.info('mongosh: Not bumping package version, keeping at placeholder');
     return;
   }
 
   console.info(`mongosh: Bumping package versions to ${version}`);
-  spawn.sync(LERNA_BIN, [
+  spawnSyncFn(LERNA_BIN, [
     'version',
     version,
     '--no-changelog',
@@ -23,12 +43,17 @@ export function bumpNpmPackages(version: string): void {
     '--yes'
   ], {
     stdio: 'inherit',
-    cwd: PROJECT_ROOT
+    cwd: PROJECT_ROOT,
+    encoding: 'utf8'
   });
 }
 
-export function publishNpmPackages(): void {
-  const packages = listNpmPackages();
+export function publishNpmPackages(
+  listNpmPackagesFn: typeof listNpmPackages = listNpmPackages,
+  markBumpedFilesAsAssumeUnchangedFn: typeof markBumpedFilesAsAssumeUnchanged = markBumpedFilesAsAssumeUnchanged,
+  spawnSyncFn: typeof spawnSync = spawnSync
+): void {
+  const packages = listNpmPackagesFn();
 
   const versions = Array.from(new Set(packages.map(({ version }) => version)));
 
@@ -40,23 +65,31 @@ export function publishNpmPackages(): void {
     throw new Error('Refusing to publish packages with placeholder version');
   }
 
-  spawn.sync(LERNA_BIN, [
-    'publish',
-    'from-package',
-    '--no-changelog',
-    '--no-push',
-    '--exact',
-    '--no-git-tag-version',
-    '--force-publish',
-    '--yes'
-  ], {
-    stdio: 'inherit',
-    cwd: PROJECT_ROOT
-  });
+  // Lerna requires a clean repository for a publish from-package (--force-publish does not have any effect here)
+  // we use git update-index --assume-unchanged on files we know have been bumped
+  markBumpedFilesAsAssumeUnchangedFn(packages, true);
+  try {
+    spawnSyncFn(LERNA_BIN, [
+      'publish',
+      'from-package',
+      '--no-changelog',
+      '--no-push',
+      '--exact',
+      '--no-git-tag-version',
+      '--force-publish',
+      '--yes'
+    ], {
+      stdio: 'inherit',
+      cwd: PROJECT_ROOT,
+      encoding: 'utf8'
+    });
+  } finally {
+    markBumpedFilesAsAssumeUnchangedFn(packages, false);
+  }
 }
 
-export function listNpmPackages(): {name: string; version: string}[] {
-  const lernaListOutput = spawn.sync(
+export function listNpmPackages(): { name: string; version: string }[] {
+  const lernaListOutput = spawnSync(
     LERNA_BIN, [
       'list',
       '--json',
@@ -69,3 +102,30 @@ export function listNpmPackages(): {name: string; version: string}[] {
 
   return JSON.parse(lernaListOutput.stdout);
 }
+
+export function markBumpedFilesAsAssumeUnchanged(
+  packages: { name: string }[], assumeUnchanged: boolean,
+  spawnSyncFn: typeof spawnSync = spawnSync
+): void {
+  const filesToAssume = [
+    '.npmrc',
+    'lerna.json'
+  ];
+  packages.forEach(({ name }) => {
+    filesToAssume.push(`packages/${name}/package.json`);
+    filesToAssume.push(`packages/${name}/package-lock.json`);
+  });
+
+  filesToAssume.forEach(f => {
+    spawnSyncFn('git', [
+      'update-index',
+      assumeUnchanged ? '--assume-unchanged' : '--no-assume-unchanged',
+      f
+    ], {
+      stdio: 'inherit',
+      cwd: PROJECT_ROOT,
+      encoding: 'utf8'
+    });
+    console.info(`File ${f} is now ${assumeUnchanged ? '' : 'NOT '}assumed to be unchanged`);
+  });
+}