Add more tests for bump and publish (#1127)

Author: ecraig12345

.vscode/launch.json

@@ -26,6 +26,28 @@
       "processId": "${command:PickProcess}",
       "sourceMaps": true,
       "skipFiles": ["<node_internals>/**"]
+    },
+    {
+      // Debug configuration for Jest extension
+      "name": "vscode-jest-tests.v2",
+      "type": "node",
+      "request": "launch",
+      "runtimeExecutable": "npm",
+      "cwd": "${workspaceFolder}",
+      "runtimeArgs": ["run-script", "test"],
+      "args": [
+        "--",
+        "--runInBand",
+        "--watch",
+        "--testTimeout=1000000",
+        "--no-coverage",
+        "--testNamePattern",
+        "${jest.testNamePattern}",
+        "--runTestsByPath",
+        "${jest.testFile}"
+      ],
+      "console": "integratedTerminal",
+      "runtimeVersion": "14"
     }
   ]
 }

change/beachball-16f366fb-0faa-4ab5-8ff0-7528df9476b7.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Add validation of package names in callHook. Update internal handling of dependent modified packages (behavior is the same).",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

jest.config.js

@@ -18,6 +18,14 @@ const commonOptions = {
 const config = {
   reporters: ['default', 'github-actions'],
   testTimeout: 60000,
+  // Enable to locally test with coverage info (will only work properly with `yarn test`, not
+  // `test:all` or individual projects). This would be tricky to enable in CI due to the multiple
+  // test projects that run in sequence. Coverage also doesn't alone capture tricky scenarios.
+  // collectCoverage: true,
+  coveragePathIgnorePatterns: ['/node_modules/', '__fixtures__'],
+  coverageThreshold: {
+    global: { branches: 80, functions: 100, lines: 90, statements: 90 },
+  },
   projects: [
     {
       displayName: 'unit',

src/__e2e__/bump.test.ts

@@ -215,7 +215,7 @@ describe('getChangedPackages', () => {
 
     // foo is not included in changed packages
     let changedPackages = getChangedPackages(options, packageInfos, scopedPackages);
-    const logLines = logs.getMockLines('all', { guids: true });
+    const logLines = logs.getMockLines('all', { sanitize: true });
     expect(logLines).toMatch(/Your local repository already has change files for these packages:\s+• foo/);
     expect(logLines).toMatchInlineSnapshot(`
       "[log] Checking for changes against "origin/master"
@@ -232,7 +232,7 @@ describe('getChangedPackages', () => {
     // change bar => bar is the only changed package returned
     repo.stageChange('packages/bar/test.js');
     changedPackages = getChangedPackages(options, packageInfos, scopedPackages);
-    expect(logs.getMockLines('all', { guids: true })).toMatchInlineSnapshot(`
+    expect(logs.getMockLines('all', { sanitize: true })).toMatchInlineSnapshot(`
       "[log] Checking for changes against "origin/master"
       [log] Found 3 changed files in current branch (before filtering)
       [log]   - ~~change/foo-<guid>.json~~ (ignored by pattern "change/*.json")
@@ -266,7 +266,7 @@ describe('getChangedPackages', () => {
 
     const changedPackages = getChangedPackages(options, packageInfos, scopedPackages);
     expect(changedPackages).toStrictEqual(['foo']);
-    expect(logs.getMockLines('all', { guids: true })).toMatchInlineSnapshot(`
+    expect(logs.getMockLines('all', { sanitize: true })).toMatchInlineSnapshot(`
       "[log] Checking for changes against "origin/master"
       [log] Found 2 changed files in current branch (before filtering)
       [log]   - test.js

src/__e2e__/getChangedPackages.test.ts

@@ -6,11 +6,13 @@ import { initMockLogs } from '../__fixtures__/mockLogs';
 import type { Repository } from '../__fixtures__/repository';
 import { RepositoryFactory } from '../__fixtures__/repositoryFactory';
 import { publish } from '../commands/publish';
-import type { RepoOptions } from '../types/BeachballOptions';
+import type { ParsedOptions, RepoOptions } from '../types/BeachballOptions';
 import { initNpmMock } from '../__fixtures__/mockNpm';
 import { removeTempDir, tmpdir } from '../__fixtures__/tmpdir';
 import { getParsedOptions } from '../options/getOptions';
 import { createCommandContext } from '../monorepo/createCommandContext';
+import { validate } from '../validation/validate';
+import { deepFreezeProperties } from '../__fixtures__/object';
 
 // Spawning actual npm to run commands against a fake registry is extremely slow, so mock it for
 // this test (packagePublish covers the more complete npm registry scenario).
@@ -27,9 +29,11 @@ describe('publish command (registry)', () => {
   let repo: Repository | undefined;
   let packToPath: string | undefined;
 
-  // show error logs for these tests
-  const logs = initMockLogs({ alsoLog: ['error'] });
+  const logs = initMockLogs();
 
+  /**
+   * Get options with defaults including skipping git stuff
+   */
   function getOptions(repoOptions?: Partial<RepoOptions>) {
     const parsedOptions = getParsedOptions({
       cwd: repo!.rootPath,
@@ -38,6 +42,7 @@ describe('publish command (registry)', () => {
         branch: defaultRemoteBranchName,
         registry: 'fake',
         message: 'apply package updates',
+        fetch: false,
         bumpDeps: false,
         push: false,
         gitTags: false,
@@ -49,6 +54,20 @@ describe('publish command (registry)', () => {
     return { options: parsedOptions.options, parsedOptions };
   }
 
+  /**
+   * For more realistic testing, call `validate()` like the CLI command does, then call `publish()`.
+   * This helps catch any new issues with double bumps or context mutation.
+   */
+  async function publishWrapper(parsedOptions: ParsedOptions) {
+    logs.clear();
+    // This does an initial bump
+    const { context } = validate(parsedOptions, { checkDependencies: true });
+    // Ensure the later bump process does not modify the context
+    deepFreezeProperties(context.bumpInfo);
+    deepFreezeProperties(context.originalPackageInfos);
+    await publish(parsedOptions.options, context);
+  }
+
   afterEach(() => {
     repositoryFactory?.cleanUp();
     repositoryFactory = undefined;
@@ -64,15 +83,14 @@ describe('publish command (registry)', () => {
 
     const { options, parsedOptions } = getOptions({ packToPath });
     generateChangeFiles(['foo'], options);
-    repo.push();
-
-    await publish(options, createCommandContext(parsedOptions));
+    await publishWrapper(parsedOptions);
 
     expect(fs.readdirSync(packToPath)).toEqual(['1-foo-1.1.0.tgz']);
     expect(npmMock.getPublishedVersions('foo')).toBeUndefined();
+    expect(logs.mocks.error).not.toHaveBeenCalled();
   });
 
-  it('publishes in monorepo with mixed public and private packages', async () => {
+  it('skips publishing private package with change file', async () => {
     repositoryFactory = new RepositoryFactory({
       folders: {
         packages: {
@@ -86,36 +104,92 @@ describe('publish command (registry)', () => {
     const { options, parsedOptions } = getOptions();
     generateChangeFiles(['foopkg'], options);
 
-    repo.push();
-
-    await publish(options, createCommandContext(parsedOptions));
-
+    // If there's only the private package with a change file, nothing happens
+    await publishWrapper(parsedOptions);
     expect(logs.mocks.log).toHaveBeenCalledWith('Nothing to bump, skipping publish!');
     expect(logs.mocks.warn).toHaveBeenCalledWith(expect.stringContaining('Change detected for private package foopkg'));
-
+    expect(logs.mocks.error).not.toHaveBeenCalled();
     expect(npmMock.getPublishedVersions('foopkg')).toBeUndefined();
+
+    // Now try with a public package change file
+    logs.clear();
+    generateChangeFiles(['publicpkg'], options);
+    await publishWrapper(parsedOptions);
+    // Should be published despite private package change also existing
+    expect(npmMock.getPublishedPackage('publicpkg')!.version).toEqual('1.1.0');
+    // This is also a good case to get a "visual regression" test of the logs
+    expect(logs.getMockLines('all', { root: repo.rootPath, sanitize: true })).toMatchInlineSnapshot(`
+      "[log]
+      Validating options and change files...
+      [warn] Change detected for private package foopkg; delete this file: <root>/change/foopkg-<guid>.json
+      [log]
+      Validating package dependencies...
+      [log] Validating no private package among package dependencies
+      [log]   OK!
+
+      [log]
+      [log]
+      Preparing to publish
+      [log]
+      Publishing with the following configuration:
+
+        registry: fake
+
+        current branch: master
+        current hash: <commit>
+        target branch: origin/master
+        npm dist-tag: latest
+
+        bumps versions before publishing: yes
+        publishes to npm registry: yes
+        pushes bumps and changelogs to remote git repo: no
+
+
+      [log] Creating temporary publish branch publish_<timestamp>
+      [log]
+      Bumping versions and publishing packages to npm registry
+
+      [log] Removing change files:
+      [log] - publicpkg-<guid>.json
+      [log]
+      Validating new package versions...
+      [log]
+      Package versions are OK to publish:
+        • publicpkg@1.1.0
+      [log] Validating no private package among package dependencies
+      [log]   OK!
+
+      [log]
+      Publishing - publicpkg@1.1.0 with tag latest
+      [log]   publish command: publish --registry fake --tag latest --loglevel warn
+      [log]   (cwd: <root>/packages/publicpkg)
+      [log] Published! - publicpkg@1.1.0
+      [log]
+      [log] Skipping git push and tagging
+      [log]
+      Cleaning up
+      [log] git checkout master
+      [log] deleting temporary publish branch publish_<timestamp>"
+    `);
   });
 
   it('publishes multiple changed packages', async () => {
-    repositoryFactory = new RepositoryFactory({
-      folders: {
-        packages: {
-          foopkg: { version: '1.0.0', dependencies: { barpkg: '^1.0.0' } },
-          barpkg: { version: '1.0.0' },
-        },
-      },
-    });
+    repositoryFactory = new RepositoryFactory('monorepo');
     repo = repositoryFactory.cloneRepository();
+    // Simulate the current package versions already existing to test validatePackageVersions
+    npmMock.publishPackage(repositoryFactory.fixture.folders.packages.foo);
+    npmMock.publishPackage(repositoryFactory.fixture.folders.packages.bar);
+    npmMock.publishPackage(repositoryFactory.fixture.folders.packages.baz);
 
     const { options, parsedOptions } = getOptions();
-    generateChangeFiles(['foopkg', 'barpkg'], options);
+    generateChangeFiles(['foo', 'bar'], options);
 
-    repo.push();
+    await publishWrapper(parsedOptions);
 
-    await publish(options, createCommandContext(parsedOptions));
-
-    expect(npmMock.getPublishedPackage('foopkg')!.version).toEqual('1.1.0');
-    expect(npmMock.getPublishedPackage('barpkg')!.version).toEqual('1.1.0');
+    expect(npmMock.getPublishedPackage('foo')!.version).toEqual('1.1.0');
+    expect(npmMock.getPublishedPackage('bar')!.version).toEqual('1.4.0');
+    expect(npmMock.mock).toHaveBeenCalledTimes(2);
+    expect(logs.mocks.error).not.toHaveBeenCalled();
   });
 
   it('packs many packages', async () => {
@@ -136,14 +210,14 @@ describe('publish command (registry)', () => {
 
     const { options, parsedOptions } = getOptions({ packToPath, groupChanges: true });
     generateChangeFiles(packageNames, options);
-    repo.push();
-
+    // initial validate() isn't relevant here
     await publish(options, createCommandContext(parsedOptions));
 
     expect(fs.readdirSync(packToPath).sort()).toEqual(
       [...packageNames].reverse().map((name, i) => `${String(i + 1).padStart(2, '0')}-${name}-1.1.0.tgz`)
     );
     expect(npmMock.getPublishedVersions('pkg-1')).toBeUndefined();
+    expect(logs.mocks.error).not.toHaveBeenCalled();
   });
 
   it('exits publishing early if only invalid change files exist', async () => {
@@ -155,16 +229,43 @@ describe('publish command (registry)', () => {
     const { options, parsedOptions } = getOptions();
     generateChangeFiles(['bar', 'fake'], options);
 
-    repo.push();
-
+    // initial validate() isn't relevant here
     await publish(options, createCommandContext(parsedOptions));
 
     expect(logs.mocks.log).toHaveBeenCalledWith('Nothing to bump, skipping publish!');
     expect(logs.mocks.warn).toHaveBeenCalledWith(expect.stringContaining('Change detected for private package bar'));
     expect(logs.mocks.warn).toHaveBeenCalledWith(
       expect.stringContaining('Change detected for nonexistent package fake')
     );
+    expect(logs.mocks.error).not.toHaveBeenCalled();
 
     expect(npmMock.getPublishedVersions('foo')).toBeUndefined();
   });
+
+  it('errors if version already exists in registry', async () => {
+    repositoryFactory = new RepositoryFactory('monorepo');
+    repo = repositoryFactory.cloneRepository();
+    // Simulate the current package versions already existing to test validatePackageVersions
+    npmMock.publishPackage(repositoryFactory.fixture.folders.packages.foo);
+    npmMock.publishPackage(repositoryFactory.fixture.folders.packages.bar);
+    npmMock.publishPackage(repositoryFactory.fixture.folders.packages.baz);
+    // also say the bumped version of foo and bar already exist (baz is fine)
+    npmMock.publishPackage({ ...repositoryFactory.fixture.folders.packages.foo, version: '1.1.0' });
+    npmMock.publishPackage({ ...repositoryFactory.fixture.folders.packages.bar, version: '1.4.0' });
+
+    const { options, parsedOptions } = getOptions();
+    generateChangeFiles(['foo', 'bar', 'baz'], options);
+    await expect(() => publishWrapper(parsedOptions)).rejects.toThrow('process.exit');
+
+    expect(logs.getMockLines('error')).toMatchInlineSnapshot(`
+      "ERROR: Attempting to publish package versions that already exist in the registry:
+        • bar@1.4.0
+        • foo@1.1.0
+      Something went wrong with publishing! Manually update these package and versions:
+        • bar@1.4.0
+        • baz@1.4.0
+        • foo@1.1.0
+      No packages were published due to validation errors (see above for details)."
+    `);
+  });
 });

src/__e2e__/publishRegistry.test.ts

@@ -37,21 +37,36 @@ export function getChange(
  * - `dependentChangeType: 'patch'`
  * - `comment: '<packageName> comment'`
  * - `email: 'test@test.com'`
+ *
+ * @param changes Array of package names or partial change files (which must include `packageName`).
+ */
+export function generateChanges(changes: (string | PartialChangeFile)[]): ChangeFileInfo[] {
+  return changes.map(change => {
+    change = typeof change === 'string' ? { packageName: change } : change;
+    return {
+      ...getChange(change.packageName, undefined, 'minor'),
+      ...change,
+    };
+  });
+}
+
+/**
+ * Generates and writes change files for the given packages.
+ * Also commits if `options.commit` is true.
+ *
+ * Default change info values:
+ * - `type: 'minor'`
+ * - `dependentChangeType: 'patch'`
+ * - `comment: '<packageName> comment'`
+ * - `email: 'test@test.com'`
+ *
+ * @param changes Array of package names or partial change files (which must include `packageName`).
  */
 export function generateChangeFiles(
   changes: (string | PartialChangeFile)[],
   options: Parameters<typeof writeChangeFiles>[1]
 ): void {
-  writeChangeFiles(
-    changes.map(change => {
-      change = typeof change === 'string' ? { packageName: change } : change;
-      return {
-        ...getChange(change.packageName, undefined, 'minor'),
-        ...change,
-      };
-    }),
-    options
-  );
+  writeChangeFiles(generateChanges(changes), options);
 }
 
 /** Get full paths to existing change files under `cwd` */