Skip to content

Commit b07e4b5

Browse files
ecraig12345ecraig12345
authored
Improve validation when bumping semver (#1122)
1 parent 079e9f4 commit b07e4b5

5 files changed

Lines changed: 172 additions & 103 deletions

File tree

‎change/beachball-6d8d2d71-d22d-4e03-bbc8-ec0adedc5d84.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "Improve validation when bumping semver",
4+
"packageName": "beachball",
5+
"email": "elcraig@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

‎src/__tests__/bump/bumpPackageInfoVersion.test.ts‎

Lines changed: 130 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -1,64 +1,77 @@
1-
import { describe, it, expect, jest, afterEach, beforeAll, afterAll } from '@jest/globals';
1+
import { describe, it, expect } from '@jest/globals';
22
import { bumpPackageInfoVersion } from '../../bump/bumpPackageInfoVersion';
33
import { makePackageInfos } from '../../__fixtures__/packageInfos';
44
import type { ChangeType } from '../../types/ChangeInfo';
5+
import { initMockLogs } from '../../__fixtures__/mockLogs';
6+
import type { PackageInfo } from '../../types/PackageInfo';
7+
import type { BeachballOptions } from '../../types/BeachballOptions';
58

69
type PartialBumpInfo = Parameters<typeof bumpPackageInfoVersion>[1];
710

811
describe('bumpPackageInfoVersion', () => {
9-
let consoleLogSpy: jest.SpiedFunction<typeof console.log>;
12+
const logs = initMockLogs();
13+
/** Reused package name */
14+
const name = 'pkg';
15+
16+
/**
17+
* Bump `pkg` (default version `1.0.0`) with given params and return the `bumpInfo`.
18+
*/
19+
function bumpPackageInfoVersionWrapper(params: {
20+
/** Extra info (defaults to version 1.0.0), or null for nonexistent package */
21+
packageInfo?: Partial<PackageInfo> | null;
22+
changeType: ChangeType | undefined;
23+
options?: Parameters<typeof bumpPackageInfoVersion>[2];
24+
}) {
25+
const { changeType, packageInfo } = params;
26+
const bumpInfo: PartialBumpInfo = {
27+
calculatedChangeTypes: changeType ? { [name]: changeType } : {},
28+
packageInfos: packageInfo === null ? {} : makePackageInfos({ [name]: packageInfo || {} }),
29+
modifiedPackages: new Set(),
30+
};
1031

11-
beforeAll(() => {
12-
consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(() => undefined);
13-
});
32+
bumpPackageInfoVersion(name, bumpInfo, params.options || {});
1433

15-
afterEach(() => {
16-
jest.clearAllMocks();
17-
});
34+
return bumpInfo;
35+
}
1836

19-
afterAll(() => {
20-
jest.restoreAllMocks();
37+
it('warns and skips when package info is not found', () => {
38+
const bumpInfo = bumpPackageInfoVersionWrapper({
39+
packageInfo: null,
40+
changeType: 'patch',
41+
});
42+
expect(logs.mocks.warn).toHaveBeenCalledWith('Unknown package named "pkg" detected from change files, skipping!');
43+
expect(bumpInfo.modifiedPackages.size).toBe(0);
2144
});
2245

23-
it('logs and skips when package info is not found', () => {
24-
const bumpInfo: PartialBumpInfo = {
25-
calculatedChangeTypes: { 'pkg-a': 'patch' },
26-
packageInfos: {},
27-
modifiedPackages: new Set<string>(),
28-
};
29-
30-
bumpPackageInfoVersion('pkg-a', bumpInfo, {});
31-
32-
expect(consoleLogSpy).toHaveBeenCalledWith('Unknown package named "pkg-a" detected from change files, skipping!');
46+
it('warns and skips when no change type is found', () => {
47+
const bumpInfo = bumpPackageInfoVersionWrapper({
48+
changeType: undefined,
49+
});
50+
expect(logs.mocks.warn).toHaveBeenCalledWith(
51+
'No change type found when bumping "pkg" (this may be a beachball bug)'
52+
);
53+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.0');
3354
expect(bumpInfo.modifiedPackages.size).toBe(0);
3455
});
3556

3657
it('logs and skips when change type is "none"', () => {
37-
const bumpInfo: PartialBumpInfo = {
38-
calculatedChangeTypes: { 'pkg-a': 'none' },
39-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0' } }),
40-
modifiedPackages: new Set<string>(),
41-
};
42-
43-
// prereleasePrefix should be ignored here
44-
bumpPackageInfoVersion('pkg-a', bumpInfo, { prereleasePrefix: 'beta' });
45-
46-
expect(consoleLogSpy).toHaveBeenCalledWith('"pkg-a" has a "none" change type, no version bump is required.');
47-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe('1.0.0');
58+
const bumpInfo = bumpPackageInfoVersionWrapper({
59+
changeType: 'none',
60+
// prereleasePrefix should be ignored here
61+
options: { prereleasePrefix: 'beta' },
62+
});
63+
expect(logs.mocks.log).toHaveBeenCalledWith('"pkg" has a "none" change type, so no version bump is required.');
64+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.0');
4865
expect(bumpInfo.modifiedPackages.size).toBe(0);
4966
});
5067

51-
it('logs and skips when package is private', () => {
52-
const bumpInfo: PartialBumpInfo = {
53-
calculatedChangeTypes: { 'pkg-a': 'patch' },
54-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0', private: true } }),
55-
modifiedPackages: new Set<string>(),
56-
};
57-
58-
bumpPackageInfoVersion('pkg-a', bumpInfo, {});
59-
60-
expect(consoleLogSpy).toHaveBeenCalledWith('Skipping bumping private package "pkg-a"');
61-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe('1.0.0');
68+
it('warns and skips when package is private', () => {
69+
const bumpInfo = bumpPackageInfoVersionWrapper({
70+
changeType: 'patch',
71+
packageInfo: { private: true },
72+
});
73+
expect(logs.mocks.warn).toHaveBeenCalledWith('Skipping bumping private package "pkg"');
74+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.0');
6275
expect(bumpInfo.modifiedPackages.size).toBe(0);
6376
});
6477

@@ -71,33 +84,24 @@ describe('bumpPackageInfoVersion', () => {
7184
['preminor', '1.1.0-0'],
7285
['prepatch', '1.0.1-0'],
7386
])('bumps %s version', (changeType, expectedVersion) => {
74-
const bumpInfo: PartialBumpInfo = {
75-
calculatedChangeTypes: { 'pkg-a': changeType },
76-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0' } }),
77-
modifiedPackages: new Set<string>(),
78-
};
79-
80-
bumpPackageInfoVersion('pkg-a', bumpInfo, {});
81-
82-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe(expectedVersion);
83-
expect(bumpInfo.modifiedPackages).toContain('pkg-a');
87+
const bumpInfo = bumpPackageInfoVersionWrapper({
88+
changeType,
89+
});
90+
expect(bumpInfo.packageInfos[name].version).toBe(expectedVersion);
91+
expect(bumpInfo.modifiedPackages).toContain(name);
8492
});
8593

8694
// This should probably be changed, but documenting it for now
8795
// https://github.com/microsoft/beachball/issues/1098
8896
it.each<ChangeType>(['major', 'minor', 'patch'])(
8997
'bumps as prerelease when prereleasePrefix is set and changeType is %s',
9098
changeType => {
91-
const bumpInfo: PartialBumpInfo = {
92-
calculatedChangeTypes: { 'pkg-a': changeType },
93-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0' } }),
94-
modifiedPackages: new Set<string>(),
95-
};
96-
97-
bumpPackageInfoVersion('pkg-a', bumpInfo, { prereleasePrefix: 'beta' });
98-
99-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe('1.0.1-beta.0');
100-
expect(bumpInfo.modifiedPackages).toContain('pkg-a');
99+
const bumpInfo = bumpPackageInfoVersionWrapper({
100+
changeType,
101+
options: { prereleasePrefix: 'beta' },
102+
});
103+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.1-beta.0');
104+
expect(bumpInfo.modifiedPackages).toContain(name);
101105
}
102106
);
103107

@@ -107,41 +111,77 @@ describe('bumpPackageInfoVersion', () => {
107111
['preminor', '1.1.0-beta.0'],
108112
['prepatch', '1.0.1-beta.0'],
109113
])('uses prereleasePrefix for changeType %s', (changeType, expectedVersion) => {
110-
const bumpInfo: PartialBumpInfo = {
111-
calculatedChangeTypes: { 'pkg-a': changeType },
112-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0' } }),
113-
modifiedPackages: new Set<string>(),
114-
};
115-
116-
bumpPackageInfoVersion('pkg-a', bumpInfo, { prereleasePrefix: 'beta' });
117-
118-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe(expectedVersion);
119-
expect(bumpInfo.modifiedPackages).toContain('pkg-a');
114+
const bumpInfo = bumpPackageInfoVersionWrapper({
115+
changeType,
116+
options: { prereleasePrefix: 'beta' },
117+
});
118+
expect(bumpInfo.packageInfos[name].version).toBe(expectedVersion);
119+
expect(bumpInfo.modifiedPackages).toContain(name);
120120
});
121121

122-
it('uses identifierBase for prerelease when provided', () => {
123-
const bumpInfo: PartialBumpInfo = {
124-
calculatedChangeTypes: { 'pkg-a': 'prerelease' },
125-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0' } }),
126-
modifiedPackages: new Set<string>(),
127-
};
128-
129-
bumpPackageInfoVersion('pkg-a', bumpInfo, { identifierBase: '1' });
122+
it('bumps to subsequent prerelease version with existing prefix', () => {
123+
const bumpInfo = bumpPackageInfoVersionWrapper({
124+
changeType: 'prerelease',
125+
packageInfo: { version: '1.0.1-beta.2' },
126+
});
127+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.1-beta.3');
128+
expect(bumpInfo.modifiedPackages).toContain(name);
129+
});
130130

131-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe('1.0.1-1');
132-
expect(bumpInfo.modifiedPackages).toContain('pkg-a');
131+
it.each<[BeachballOptions['identifierBase'], string]>([
132+
[undefined, '1.0.1-beta.0'], // default
133+
['0', '1.0.1-beta.0'],
134+
['1', '1.0.1-beta.1'],
135+
[false, '1.0.1-beta'], // disable numeric identifier
136+
])('uses identifierBase %s for prerelease', (identifierBase, expectedVersion) => {
137+
const bumpInfo = bumpPackageInfoVersionWrapper({
138+
changeType: 'prerelease',
139+
options: { identifierBase, prereleasePrefix: 'beta' },
140+
});
141+
expect(bumpInfo.packageInfos[name].version).toBe(expectedVersion);
142+
expect(bumpInfo.modifiedPackages).toContain(name);
133143
});
134144

135145
it('uses both prereleasePrefix and identifierBase when provided', () => {
136-
const bumpInfo: PartialBumpInfo = {
137-
calculatedChangeTypes: { 'pkg-a': 'prerelease' },
138-
packageInfos: makePackageInfos({ 'pkg-a': { version: '1.0.0' } }),
139-
modifiedPackages: new Set<string>(),
140-
};
146+
const bumpInfo = bumpPackageInfoVersionWrapper({
147+
changeType: 'prerelease',
148+
options: { prereleasePrefix: 'beta', identifierBase: '1' },
149+
});
150+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.1-beta.1');
151+
expect(bumpInfo.modifiedPackages).toContain(name);
152+
});
141153

142-
bumpPackageInfoVersion('pkg-a', bumpInfo, { prereleasePrefix: 'beta', identifierBase: '1' });
154+
it('warns and skips if change type is not valid semver', () => {
155+
const bumpInfo = bumpPackageInfoVersionWrapper({
156+
changeType: 'invalid-type' as ChangeType,
157+
});
158+
expect(logs.mocks.warn).toHaveBeenCalledWith(
159+
'Invalid version bump requested for "pkg": from version "1.0.0", change type "invalid-type"'
160+
);
161+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.0');
162+
expect(bumpInfo.modifiedPackages.size).toBe(0);
163+
});
164+
165+
it('warns and skips if prereleasePrefix is invalid', () => {
166+
const bumpInfo = bumpPackageInfoVersionWrapper({
167+
changeType: 'prerelease',
168+
options: { prereleasePrefix: '!!!' },
169+
});
170+
expect(logs.mocks.warn).toHaveBeenCalledWith(
171+
'Invalid version bump requested for "pkg": from version "1.0.0", change type "prerelease", prerelease prefix "!!!"'
172+
);
173+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.0');
174+
expect(bumpInfo.modifiedPackages.size).toBe(0);
175+
});
143176

144-
expect(bumpInfo.packageInfos['pkg-a'].version).toBe('1.0.1-beta.1');
145-
expect(bumpInfo.modifiedPackages).toContain('pkg-a');
177+
// documenting semver package behavior
178+
it('ignores invalid identifierBase', () => {
179+
const bumpInfo = bumpPackageInfoVersionWrapper({
180+
changeType: 'prerelease',
181+
options: { prereleasePrefix: 'beta', identifierBase: 'nope' as BeachballOptions['identifierBase'] },
182+
});
183+
expect(logs.mocks.warn).not.toHaveBeenCalled();
184+
expect(bumpInfo.packageInfos[name].version).toBe('1.0.1-beta.0');
185+
expect(bumpInfo.modifiedPackages).toContain(name);
146186
});
147187
});

‎src/bump/bumpPackageInfoVersion.ts‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,24 +16,43 @@ export function bumpPackageInfoVersion(
1616
const changeType = calculatedChangeTypes[pkgName];
1717

1818
if (!info) {
19-
console.log(`Unknown package named "${pkgName}" detected from change files, skipping!`);
19+
console.warn(`Unknown package named "${pkgName}" detected from change files, skipping!`);
20+
} else if (!changeType) {
21+
console.warn(`No change type found when bumping "${pkgName}" (this may be a beachball bug)`);
2022
} else if (changeType === 'none') {
21-
console.log(`"${pkgName}" has a "none" change type, no version bump is required.`);
23+
console.log(`"${pkgName}" has a "none" change type, so no version bump is required.`);
2224
} else if (info.private) {
23-
console.log(`Skipping bumping private package "${pkgName}"`);
25+
console.warn(`Skipping bumping private package "${pkgName}"`);
2426
} else {
2527
// Ensure we can bump the correct versions
26-
const bumpAsPrerelease = !!options.prereleasePrefix && !['premajor', 'preminor', 'prepatch'].includes(changeType);
28+
const effectiveChangeType =
29+
options.prereleasePrefix && !['premajor', 'preminor', 'prepatch'].includes(changeType)
30+
? 'prerelease'
31+
: changeType;
2732

28-
// Version should be updated
29-
info.version = semver.inc(
33+
// Attempt to update the version
34+
const newVersion = semver.inc(
3035
info.version,
31-
bumpAsPrerelease ? 'prerelease' : changeType,
36+
effectiveChangeType,
3237
undefined,
3338
options.prereleasePrefix || undefined,
3439
options.identifierBase
35-
) as string;
40+
);
3641

37-
modifiedPackages.add(pkgName);
42+
if (newVersion) {
43+
info.version = newVersion;
44+
modifiedPackages.add(pkgName);
45+
} else {
46+
let message = `Invalid version bump requested for "${pkgName}": from version "${info.version}", change type "${effectiveChangeType}"`;
47+
if (effectiveChangeType.startsWith('pre')) {
48+
if (options.prereleasePrefix) {
49+
message += `, prerelease prefix "${options.prereleasePrefix}"`;
50+
}
51+
if (options.identifierBase) {
52+
message += `, identifier base "${options.identifierBase}"`;
53+
}
54+
}
55+
console.warn(message);
56+
}
3857
}
3958
}

‎src/changefile/changeTypes.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ const ChangeTypeWeights = Object.fromEntries(SortedChangeTypes.map((t, i) => [t,
2626

2727
/**
2828
* Get initial package change types based on the greatest change type set for each package in any
29-
* change file, accounting for any disallowed change types or nonexistent packages.
29+
* change file, accounting for any disallowed change types. (Nonexistent or private packages were
30+
* already filtered out by `readChangeFiles`.)
31+
*
3032
* Anything with change type "none" will be ignored.
3133
*/
3234
export function initializePackageChangeTypes(changeSet: ChangeSet): BumpInfo['calculatedChangeTypes'] {

‎src/types/BeachballOptions.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,9 +193,10 @@ export interface RepoOptions {
193193
path: string;
194194
/** Prerelease prefix for packages that are specified to receive a prerelease bump */
195195
prereleasePrefix?: string | null;
196-
/** This is for prerelease. Set it to "0" for zero-based or "1" for one-based.
197-
* Set it to false to omit the prerelease number.
198-
* @default "0"
196+
/**
197+
* This is for prerelease. Set it to "0" for zero-based or "1" for one-based.
198+
* Set it to false to omit the prerelease number.
199+
* @default "0"
199200
*/
200201
identifierBase?: '0' | '1' | false;
201202
/**

0 commit comments

Comments
 (0)
⚡