Centralize error handling (#1172)

Author: ecraig12345

change/beachball-4c8bb372-aff5-438c-9706-ca20aaac11bb.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Centralize error handling instead of calling process.exit() throughout the code",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

scripts/jestSetup.js

@@ -1,6 +1,10 @@
 // @ts-check
 const { jest, afterAll } = require('@jest/globals');
 
+// Safety net: production code should never call process.exit() directly
+// (errors should be thrown and caught at the top level in cli.ts).
+// This mock ensures any accidental process.exit() call in tests immediately
+// throws rather than silently exiting the test runner.
 jest.spyOn(process, 'exit').mockImplementation(code => {
   throw new Error(`process.exit called with code ${code}`);
 });

src/__e2e__/validate.test.ts

@@ -5,13 +5,12 @@ import { initMockLogs } from '../__fixtures__/mockLogs';
 import { validate, type ValidateOptions } from '../validation/validate';
 import type { Repository } from '../__fixtures__/repository';
 import { getParsedOptions } from '../options/getOptions';
-import { mockProcessExit } from '../__fixtures__/mockProcessExit';
+import { BeachballError } from '../types/BeachballError';
 
 describe('validate', () => {
   let repositoryFactory: RepositoryFactory;
   let repo: Repository | undefined;
   const logs = initMockLogs();
-  const processExit = mockProcessExit(logs);
 
   function validateWrapper(validateOptions?: ValidateOptions) {
     const parsedOptions = getParsedOptions({
@@ -30,7 +29,6 @@ describe('validate', () => {
   });
 
   afterEach(() => {
-    processExit.mockClear();
     repo = undefined;
   });
 
@@ -54,8 +52,7 @@ describe('validate', () => {
     repo.checkout('-b', 'test');
     repo.stageChange('packages/foo/test.js');
 
-    expect(() => validateWrapper({ checkChangeNeeded: true })).toThrowError(/process\.exit/);
-    expect(processExit).toHaveBeenCalledWith(1);
+    expect(() => validateWrapper({ checkChangeNeeded: true })).toThrow(BeachballError);
     expect(logs.mocks.error).toHaveBeenCalledWith('ERROR: Change files are needed!');
   });
 

src/__fixtures__/mockNpm.test.ts

@@ -2,7 +2,7 @@
 // But this added complexity greatly speeds up the other npm-related tests by removing the
 // dependency on actual npm CLI calls and a fake registry (which are very slow).
 
-import { afterAll, afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals';
+import { afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals';
 import fs from 'fs';
 // import fetch from 'npm-registry-fetch';
 import { type NpmResult, npm } from '../packageManager/npm';
@@ -208,10 +208,6 @@ describe('_mockNpmPublish', () => {
     packageJson = undefined;
   });
 
-  afterAll(() => {
-    jest.restoreAllMocks();
-  });
-
   it('throws if cwd is not specified', async () => {
     await expect(() => _mockNpmPublish({}, [], { cwd: undefined })).rejects.toThrow(
       'cwd is required for mock npm publish'
@@ -311,10 +307,6 @@ describe('_mockNpmPack', () => {
     writtenFiles = [];
   });
 
-  afterAll(() => {
-    jest.restoreAllMocks();
-  });
-
   it('throws if cwd is not specified', async () => {
     await expect(() => _mockNpmPack({}, [], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm pack');
   });
@@ -370,10 +362,6 @@ describe('mockNpm', () => {
     packageJson = undefined;
   });
 
-  afterAll(() => {
-    jest.restoreAllMocks();
-  });
-
   // describe('mockFetchJson', () => {
   //   it('mocks registry fetch', async () => {
   //     npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });

src/__fixtures__/mockProcessExit.ts

@@ -1,4 +1,4 @@
-import { afterAll, afterEach, describe, expect, it, jest } from '@jest/globals';
+import { afterEach, describe, expect, it, jest } from '@jest/globals';
 import { getCliOptions } from '../../options/getCliOptions';
 import { findProjectRoot, getDefaultRemoteBranch } from 'workspace-tools';
 
@@ -34,10 +34,6 @@ describe('getCliOptions', () => {
     jest.clearAllMocks();
   });
 
-  afterAll(() => {
-    jest.restoreAllMocks();
-  });
-
   // start by making sure nothing went wrong with the mock
   it('uses fake project root', () => {
     expect(findProjectRoot(process.cwd())).toEqual(projectRoot);

src/__functional__/options/getCliOptions.test.ts

@@ -30,7 +30,6 @@ describe('getOptions (deprecated)', () => {
 
   afterAll(() => {
     repositoryFactory.cleanUp();
-    jest.restoreAllMocks();
   });
 
   it('uses the branch name defined in beachball.config.js', () => {
@@ -115,7 +114,6 @@ describe('getParsedOptions', () => {
 
   afterAll(() => {
     repositoryFactory.cleanUp();
-    jest.restoreAllMocks();
   });
 
   it('uses the branch name defined in beachball.config.js', () => {

src/__functional__/options/getOptions.test.ts

@@ -1,4 +1,4 @@
-import { describe, expect, it, afterAll, beforeEach, jest } from '@jest/globals';
+import { describe, expect, it, beforeEach, jest } from '@jest/globals';
 // import fetch from 'npm-registry-fetch';
 import {
   _npmShowProperties,
@@ -24,10 +24,6 @@ describe('getNpmPackageInfo', () => {
     // fetchJsonSpy.mockClear();
   });
 
-  afterAll(() => {
-    jest.restoreAllMocks();
-  });
-
   it.each<{ desc: string; name: string; knownVersion: string }>([
     { desc: 'unscoped', name: 'beachball', knownVersion: '2.60.1' },
     { desc: 'scoped', name: '@lage-run/cli', knownVersion: '0.33.0' },

src/__functional__/packageManager/getNpmPackageInfo.test.ts

@@ -158,8 +158,7 @@ describe('publishToRegistry', () => {
     // Pre-populate registry so version 1.0.0 already exists
     npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
 
-    // process.exit is mocked in jestSetup.js to throw
-    await expect(publishToRegistry(bumpInfo, defaultOptions)).rejects.toThrow('process.exit called with code 1');
+    await expect(publishToRegistry(bumpInfo, defaultOptions)).rejects.toThrow('Pre-publish validation failed');
 
     expect(logs.getMockLines('all')).toMatchInlineSnapshot(`
       "[log] Validating new package versions...

src/__functional__/publish/publishToRegistry.test.ts

@@ -1,8 +1,7 @@
-import { afterAll, beforeAll, describe, expect, it, jest } from '@jest/globals';
+import { describe, expect, it } from '@jest/globals';
 import path from 'path';
 import { generateChanges, type PartialChangeFile } from '../../__fixtures__/changeFiles';
 import { initMockLogs } from '../../__fixtures__/mockLogs';
-import { mockProcessExit } from '../../__fixtures__/mockProcessExit';
 import { makePackageInfosByFolder, type PartialPackageInfo } from '../../__fixtures__/packageInfos';
 import { bumpInMemory } from '../../bump/bumpInMemory';
 import { getParsedOptions } from '../../options/getOptions';
@@ -12,7 +11,7 @@ import { getScopedPackages } from '../../monorepo/getScopedPackages';
 import { getPackageGroups } from '../../monorepo/getPackageGroups';
 
 describe('bumpInMemory', () => {
-  const logs = initMockLogs();
+  initMockLogs();
   const cwd = path.resolve('/fake-root');
 
   function gatherBumpInfoWrapper(params: {
@@ -42,15 +41,6 @@ describe('bumpInMemory', () => {
     return { bumpInfo, options, originalPackageInfos };
   }
 
-  beforeAll(() => {
-    // getPackageGroups currently can call process.exit
-    mockProcessExit(logs);
-  });
-
-  afterAll(() => {
-    jest.restoreAllMocks();
-  });
-
   it('bumps only packages with change files with bumpDeps: false', () => {
     const { bumpInfo, originalPackageInfos } = gatherBumpInfoWrapper({
       packageFolders: {