Add more tests and improve publish logging (#1201)

Author: ecraig12345

.vscode/launch.json

@@ -11,7 +11,7 @@
       "runtimeExecutable": "npm",
       "cwd": "${workspaceFolder}",
       "runtimeArgs": ["run-script", "test"],
-      "args": ["--", "--runInBand", "--watch", "--testTimeout=1000000", "${fileBasenameNoExtension}"],
+      "args": ["--", "--runInBand", "--watch", "--testTimeout=1000000", "${file}"],
       "sourceMaps": true,
       "outputCapture": "std",
       "console": "integratedTerminal",

change/beachball-a829dbf8-bbe0-44f3-8b0a-dc845bba6f17.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Refine publish logging",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

src/__e2e__/publishE2E.test.ts

@@ -20,7 +20,7 @@ import { validate } from '../validation/validate';
 // These tests are slow, so they should only cover E2E publishing scenarios that can't be fully
 // covered by lower-level tests (such as publishToRegistry or bumping functional tests), and a
 // few all-up scenarios as sanity checks. Tests specific to git or npm scenarios should
-// potentially go in publishGit.test.ts or publishRegistry.test.ts instead.
+// potentially go in publishGit.test.ts or publishNpm.test.ts instead.
 //
 // 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).
@@ -88,9 +88,9 @@ describe('publish command (e2e)', () => {
     generateChangeFiles(['foo'], options);
     repo.push();
 
-    let fetchCount = 0;
     addGitObserver(args => {
-      args[0] === 'fetch' && fetchCount++;
+      // no fetch when flag set to false
+      expect(args[0]).not.toBe('fetch');
     });
 
     await publishWrapper(parsedOptions);
@@ -101,9 +101,6 @@ describe('publish command (e2e)', () => {
       'dist-tags': { latest: '1.1.0' },
     });
 
-    // no fetch when flag set to false
-    expect(fetchCount).toBe(0);
-
     repo.checkout(defaultBranchName);
     repo.pull();
     expect(repo.getCurrentTags()).toEqual(['foo_v1.1.0']);

src/__e2e__/publishGit.test.ts

@@ -16,7 +16,6 @@ import { getParsedOptions } from '../options/getOptions';
 import { readJson } from '../object/readJson';
 import { createCommandContext } from '../monorepo/createCommandContext';
 import type { RepoOptions } from '../types/BeachballOptions';
-import { addGitObserver, clearGitObservers } from 'workspace-tools';
 
 describe('publish command (git)', () => {
   let repositoryFactory: RepositoryFactory | undefined;
@@ -44,7 +43,6 @@ describe('publish command (git)', () => {
   }
 
   afterEach(() => {
-    clearGitObservers();
     repositoryFactory?.cleanUp();
     repositoryFactory = undefined;
     repo = undefined;
@@ -129,26 +127,4 @@ describe('publish command (git)', () => {
     // changes from publish process were committed
     expect(fs.existsSync(txtPath)).toBe(true);
   });
-
-  it('specifies fetch depth when depth param is defined', async () => {
-    repositoryFactory = new RepositoryFactory('single');
-    repo = repositoryFactory.cloneRepository();
-
-    const { options, parsedOptions } = getOptions({
-      depth: 10,
-    });
-
-    generateChangeFiles(['foo'], options);
-    repo.push();
-
-    const gitObserver = jest.fn((args: string[]) => {
-      if (args[0] === 'fetch') {
-        expect(args).toContain('--depth=10');
-      }
-    });
-    addGitObserver(gitObserver);
-
-    await publish(options, createCommandContext(parsedOptions));
-    expect(gitObserver).toHaveBeenCalled();
-  });
 });

src/__e2e__/publishRegistry.test.ts src/__e2e__/publishNpm.test.tssrc/__e2e__/publishRegistry.test.ts renamed to src/__e2e__/publishNpm.test.ts

@@ -25,7 +25,7 @@ import { validate } from '../validation/validate';
 jest.mock('../packageManager/npm');
 // jest.mock('npm-registry-fetch');
 
-describe('publish command (registry)', () => {
+describe('publish command (npm)', () => {
   const npmMock = initNpmMock();
 
   let repositoryFactory: RepositoryFactory | undefined;
@@ -110,7 +110,7 @@ describe('publish command (registry)', () => {
 
     // 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.log).toHaveBeenCalledWith(expect.stringContaining('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();
@@ -132,10 +132,9 @@ describe('publish command (registry)', () => {
       [log]   OK!
 
       [log]
-      [log]
-      Preparing to publish
-      [log]
-      Publishing with the following configuration:
+      [log] Preparing to publish
+
+      [log] Publishing with the following configuration:
 
         registry: fake
 
@@ -150,8 +149,8 @@ describe('publish command (registry)', () => {
 
 
       [log] Creating temporary publish branch publish_<timestamp>
-      [log]
-      Bumping versions and publishing packages to npm registry
+
+      [log] Bumping versions and publishing packages to npm registry
 
       [log] Removing change files:
       [log] - publicpkg-<guid>.json
@@ -171,8 +170,9 @@ describe('publish command (registry)', () => {
 
       [log]
       [log] Skipping git push and tagging
-      [log]
-      Cleaning up
+
+      [log] Cleaning up
+
       [log] git checkout master
       [log] deleting temporary publish branch publish_<timestamp>"
     `);
@@ -210,7 +210,7 @@ describe('publish command (registry)', () => {
     // initial validate() isn't relevant here
     await publish(options, createCommandContext(parsedOptions));
 
-    expect(logs.mocks.log).toHaveBeenCalledWith('Nothing to bump, skipping publish!');
+    expect(logs.mocks.log).toHaveBeenCalledWith(expect.stringContaining('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')

src/__fixtures__/mockLogs.ts

@@ -1,4 +1,4 @@
-import { jest, afterEach, beforeAll, afterAll } from '@jest/globals';
+import { jest, afterEach, beforeAll, afterAll, beforeEach } from '@jest/globals';
 
 /** Methods that will be mocked. More could be added later if needed. */
 type MockLogMethod = 'log' | 'warn' | 'error';
@@ -10,6 +10,12 @@ type MockLogsOptions = {
    * All logging can be enabled by setting the VERBOSE env var.
    */
   alsoLog?: boolean | MockLogMethod[];
+
+  /**
+   * Instead of setting up mocks in `beforeAll`, do it in `beforeEach`.
+   * This allows calling `jest.resetAllMocks()` in tests without breaking the logging mocks.
+   */
+  mockBeforeEach?: boolean;
 };
 
 export type MockLogs = {
@@ -49,7 +55,7 @@ export type MockLogs = {
  * of any lifecycle hooks or tests because it calls lifecycle hooks internally for setup and teardown.
  */
 export function initMockLogs(options: MockLogsOptions = {}): MockLogs {
-  const { alsoLog } = options;
+  const { alsoLog, mockBeforeEach } = options;
   let allLines: unknown[][] = [];
   let overrideOptions: MockLogsOptions | undefined;
   const jestConsole = { ...console };
@@ -102,7 +108,7 @@ export function initMockLogs(options: MockLogsOptions = {}): MockLogs {
     },
   };
 
-  beforeAll(() => {
+  (mockBeforeEach ? beforeEach : beforeAll)(() => {
     for (const method of mockedMethods) {
       const mainShouldLog = shouldLog(method, alsoLog);
 
@@ -121,7 +127,7 @@ export function initMockLogs(options: MockLogsOptions = {}): MockLogs {
     logs.clear();
   });
 
-  afterAll(() => {
+  (mockBeforeEach ? afterEach : afterAll)(() => {
     Object.values(logs.mocks).forEach(mock => mock.mockRestore());
   });
 

src/__fixtures__/registry.ts

@@ -17,7 +17,7 @@ const portRange = 1000;
  * Lists of tests known to use `Registry`. This is used to make each test try a different
  * port range to avoid collisions caused by race conditions with grabbing free ports.
  */
-const knownTests = ['packagePublish', 'publishE2E', 'publishRegistry', 'syncE2E'];
+const knownTests = ['packagePublish', 'publishE2E', 'publishNpm', 'syncE2E'];
 
 // NOTE: If you are getting timeouts and port collisions, set jest.setTimeout to a higher value.
 //       The default value of 5 seconds may not be enough in situations with port collisions.

src/__functional__/commands/__snapshots__/publish.test.ts.snap

@@ -0,0 +1,39 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`publish command logs expected output for a standard publish flow 1`] = `
+"[log]
+Validating options and change files...
+[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]
+[log] Skipping git push and tagging
+
+[log] Cleaning up
+
+[log] git checkout master
+[log] deleting temporary publish branch publish_<timestamp>"
+`;