Skip to content

Commit c267d3a

Browse files
QbandevCopilot
andauthored
fix(INFRA-3081): implement duplicate detection for all commit types (#254)
## Summary Implements duplicate detection for changelog entries to prevent re-adding the same commits across multiple workflow runs. ## Problem The auto-changelog tool was creating duplicate entries when CI ran multiple times on a release branch. The existing deduplication only tracked PR numbers, so direct commits (without PR numbers) like "Update Attributions" or "chore: bump version" would be added repeatedly. ## Solution ### 1. Dual-Tracking Duplicate Detection - **PR-based commits**: Check if PR number already exists in changelog - **Direct commits**: Check if exact description text already exists in changelog ### 2. Handle Squash Merge Duplicates Squash merges create two commits with the same content: - Feature branch commit: `feat: Add feature` (no PR number) - Merge commit: `feat: Add feature (#123)` (has PR number) The feature branch commit is now skipped when its corresponding merge commit exists. ### 3. Fix Merge Commit Categorization Regular merge commits (format: `Merge pull request #123 from branch`) were being excluded because categorization used the merge subject instead of the actual PR content. Now uses the extracted description from the commit body for proper categorization. ## Changes - Added `loggedDescriptions` parameter to track all changelog entry descriptions - Updated duplicate detection to check both PR numbers and descriptions - Pre-build Set of PR descriptions for O(1) lookup performance - Refined exclusion keyword `'merge'` → `'Merge pull request'` to avoid false exclusions - Use description for categorizing merge commits instead of subject ## Testing Verified with test repository [`consensys-test/metamask-extension-test`](https://github.com/consensys-test/metamask-extension-test) using multiple release branches with: - Regular merge PRs - Squash merge PRs - Direct commits (feat, fix, docs, chore) - Multiple workflow runs to test idempotency **Results:** ✅ No duplicates, all commits correctly categorized, merge commits properly handled - [Latest job run](https://github.com/consensys-test/metamask-extension-test/actions/runs/19496632321/job/55800299530) - [Changelog](https://github.com/consensys-test/metamask-extension-test/blob/release/13.10.0-Changelog/CHANGELOG.md) ## Related Fixes INFRA-3081 <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Add robust duplicate detection using PR numbers and exact descriptions (incl. squash/merge cases), update exclusions, and add comprehensive tests. > > - **Changelog generation (`src/get-new-changes.ts`)**: > - Add `loggedDescriptions` support and dedup logic for PR-based and direct commits, skipping direct commits when a matching PR merge exists. > - Use description (not subject) for merge-commit categorization; append PR link with optional short format. > - Replace assertion with explicit error when `git show` subject is empty; refine GITHUB_TOKEN retrieval. > - **Changelog update (`src/update-changelog.ts`)**: > - Extract trimmed `loggedDescriptions` from existing changelog and pass to `getNewChangeEntries`. > - **Exclusions (`src/constants.ts`)**: > - Adjust exclusion keyword from `merge` to `Merge pull request`. > - **Tests**: > - Add `src/get-new-changes.test.ts` with cases for PR vs direct commits, duplicate detection across runs, squash/merge handling, link formatting, and edge cases. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit d4fea63. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 47346f4 commit c267d3a

File tree

4 files changed

+432
-29
lines changed

4 files changed

+432
-29
lines changed

src/constants.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,6 @@ export const keywordsToIndicateExcluded: string[] = [
103103
'e2e',
104104
'flaky test',
105105
'INFRA-',
106-
'merge',
106+
'Merge pull request',
107107
'New Crowdin translations',
108108
].map((word) => word.toLowerCase());

src/get-new-changes.test.ts

Lines changed: 339 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,339 @@
1+
import { getNewChangeEntries } from './get-new-changes';
2+
import { runCommand, runCommandAndSplit } from './run-command';
3+
4+
jest.mock('./run-command');
5+
6+
const mockRunCommand = runCommand as jest.MockedFunction<typeof runCommand>;
7+
const mockRunCommandAndSplit = runCommandAndSplit as jest.MockedFunction<
8+
typeof runCommandAndSplit
9+
>;
10+
11+
const repoUrl = 'https://github.com/MetaMask/metamask-mobile';
12+
13+
describe('getNewChangeEntries', () => {
14+
beforeEach(() => {
15+
mockRunCommandAndSplit.mockResolvedValue([]);
16+
});
17+
18+
describe('PR-tagged commits', () => {
19+
it('should include commits with PR numbers', async () => {
20+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1', 'commit2']);
21+
mockRunCommand
22+
.mockResolvedValueOnce('add feature (#12345)')
23+
.mockResolvedValueOnce('bug fix (#12346)');
24+
25+
const result = await getNewChangeEntries({
26+
mostRecentTag: 'v1.0.0',
27+
repoUrl,
28+
loggedPrNumbers: [],
29+
loggedDescriptions: [],
30+
useChangelogEntry: false,
31+
useShortPrLink: false,
32+
});
33+
34+
expect(result).toStrictEqual([
35+
{
36+
description:
37+
'add feature ([#12345](https://github.com/MetaMask/metamask-mobile/pull/12345))',
38+
subject: 'add feature (#12345)',
39+
},
40+
{
41+
description:
42+
'bug fix ([#12346](https://github.com/MetaMask/metamask-mobile/pull/12346))',
43+
subject: 'bug fix (#12346)',
44+
},
45+
]);
46+
});
47+
48+
it('should exclude commits with PR numbers already in changelog', async () => {
49+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1', 'commit2']);
50+
mockRunCommand
51+
.mockResolvedValueOnce('add feature (#12345)')
52+
.mockResolvedValueOnce('bug fix (#12346)');
53+
54+
const result = await getNewChangeEntries({
55+
mostRecentTag: 'v1.0.0',
56+
repoUrl,
57+
loggedPrNumbers: ['12345'],
58+
loggedDescriptions: [],
59+
useChangelogEntry: false,
60+
useShortPrLink: false,
61+
});
62+
63+
expect(result).toStrictEqual([
64+
{
65+
description:
66+
'bug fix ([#12346](https://github.com/MetaMask/metamask-mobile/pull/12346))',
67+
subject: 'bug fix (#12346)',
68+
},
69+
]);
70+
});
71+
});
72+
73+
describe('direct commits (no PR numbers)', () => {
74+
it('should include direct commits', async () => {
75+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1', 'commit2']);
76+
mockRunCommand
77+
.mockResolvedValueOnce('Update Attributions')
78+
.mockResolvedValueOnce('Bump version to 7.58.0');
79+
80+
const result = await getNewChangeEntries({
81+
mostRecentTag: 'v1.0.0',
82+
repoUrl,
83+
loggedPrNumbers: [],
84+
loggedDescriptions: [],
85+
useChangelogEntry: false,
86+
useShortPrLink: false,
87+
});
88+
89+
expect(result).toStrictEqual([
90+
{
91+
description: 'Update Attributions',
92+
subject: 'Update Attributions',
93+
},
94+
{
95+
description: 'Bump version to 7.58.0',
96+
subject: 'Bump version to 7.58.0',
97+
},
98+
]);
99+
});
100+
101+
it('should exclude direct commits already in changelog', async () => {
102+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1', 'commit2']);
103+
mockRunCommand
104+
.mockResolvedValueOnce('Update Attributions')
105+
.mockResolvedValueOnce('Bump version to 7.58.0');
106+
107+
const result = await getNewChangeEntries({
108+
mostRecentTag: 'v1.0.0',
109+
repoUrl,
110+
loggedPrNumbers: [],
111+
loggedDescriptions: ['Update Attributions'],
112+
useChangelogEntry: false,
113+
useShortPrLink: false,
114+
});
115+
116+
expect(result).toStrictEqual([
117+
{
118+
description: 'Bump version to 7.58.0',
119+
subject: 'Bump version to 7.58.0',
120+
},
121+
]);
122+
});
123+
});
124+
125+
describe('merge commits', () => {
126+
it('should extract PR numbers from merge commits', async () => {
127+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1', 'commit2']);
128+
mockRunCommand
129+
.mockResolvedValueOnce('Merge pull request #12345 from feature-branch')
130+
.mockResolvedValueOnce('Merge pull request #12346 from fix-branch');
131+
// Mock body fetches for merge commits
132+
mockRunCommandAndSplit
133+
.mockResolvedValueOnce(['implement new feature'])
134+
.mockResolvedValueOnce(['fix critical bug']);
135+
136+
const result = await getNewChangeEntries({
137+
mostRecentTag: 'v1.0.0',
138+
repoUrl,
139+
loggedPrNumbers: [],
140+
loggedDescriptions: [],
141+
useChangelogEntry: false,
142+
useShortPrLink: false,
143+
});
144+
145+
expect(result).toStrictEqual([
146+
{
147+
description:
148+
'implement new feature ([#12345](https://github.com/MetaMask/metamask-mobile/pull/12345))',
149+
subject: 'implement new feature',
150+
},
151+
{
152+
description:
153+
'fix critical bug ([#12346](https://github.com/MetaMask/metamask-mobile/pull/12346))',
154+
subject: 'fix critical bug',
155+
},
156+
]);
157+
});
158+
});
159+
160+
describe('squash merge deduplication', () => {
161+
it('should skip direct commit when PR-tagged commit with same description exists', async () => {
162+
// Simulates squash merge where both original and merged commits appear
163+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1', 'commit2']);
164+
mockRunCommand
165+
.mockResolvedValueOnce('add new feature') // Direct commit (no PR)
166+
.mockResolvedValueOnce('add new feature (#12345)'); // PR-tagged commit with same description
167+
168+
const result = await getNewChangeEntries({
169+
mostRecentTag: 'v1.0.0',
170+
repoUrl,
171+
loggedPrNumbers: [],
172+
loggedDescriptions: [],
173+
useChangelogEntry: false,
174+
useShortPrLink: false,
175+
});
176+
177+
// Should only include the PR-tagged version
178+
expect(result).toStrictEqual([
179+
{
180+
description:
181+
'add new feature ([#12345](https://github.com/MetaMask/metamask-mobile/pull/12345))',
182+
subject: 'add new feature (#12345)',
183+
},
184+
]);
185+
});
186+
});
187+
188+
describe('duplicate detection', () => {
189+
it('should return empty array when all commits are duplicates', async () => {
190+
mockRunCommandAndSplit.mockResolvedValueOnce([
191+
'commit1',
192+
'commit2',
193+
'commit3',
194+
]);
195+
mockRunCommand
196+
.mockResolvedValueOnce('add feature (#12345)')
197+
.mockResolvedValueOnce('Update Attributions')
198+
.mockResolvedValueOnce('Bump version');
199+
200+
const result = await getNewChangeEntries({
201+
mostRecentTag: 'v1.0.0',
202+
repoUrl,
203+
loggedPrNumbers: ['12345'],
204+
loggedDescriptions: ['Update Attributions', 'Bump version'],
205+
useChangelogEntry: false,
206+
useShortPrLink: false,
207+
});
208+
209+
expect(result).toStrictEqual([]);
210+
});
211+
});
212+
213+
describe('edge cases', () => {
214+
it('should return empty array when there are no commits', async () => {
215+
mockRunCommandAndSplit.mockResolvedValueOnce([]);
216+
217+
const result = await getNewChangeEntries({
218+
mostRecentTag: 'v1.0.0',
219+
repoUrl,
220+
loggedPrNumbers: [],
221+
loggedDescriptions: [],
222+
useChangelogEntry: false,
223+
useShortPrLink: false,
224+
});
225+
226+
expect(result).toStrictEqual([]);
227+
});
228+
229+
it('should use HEAD as commit range when no tag is available', async () => {
230+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1']);
231+
mockRunCommand.mockResolvedValueOnce('add feature (#12345)');
232+
233+
const result = await getNewChangeEntries({
234+
mostRecentTag: null,
235+
repoUrl,
236+
loggedPrNumbers: [],
237+
loggedDescriptions: [],
238+
useChangelogEntry: false,
239+
useShortPrLink: false,
240+
});
241+
242+
expect(mockRunCommandAndSplit).toHaveBeenCalledWith('git', [
243+
'rev-list',
244+
'HEAD',
245+
]);
246+
expect(result).toStrictEqual([
247+
{
248+
description:
249+
'add feature ([#12345](https://github.com/MetaMask/metamask-mobile/pull/12345))',
250+
subject: 'add feature (#12345)',
251+
},
252+
]);
253+
});
254+
255+
it('should throw error when git show returns empty subject', async () => {
256+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1']);
257+
mockRunCommand.mockResolvedValueOnce('');
258+
259+
await expect(
260+
getNewChangeEntries({
261+
mostRecentTag: 'v1.0.0',
262+
repoUrl,
263+
loggedPrNumbers: [],
264+
loggedDescriptions: [],
265+
useChangelogEntry: false,
266+
useShortPrLink: false,
267+
}),
268+
).rejects.toThrow(
269+
'"git show" returned empty subject for commit "commit1".',
270+
);
271+
});
272+
});
273+
274+
describe('PR link formatting', () => {
275+
it('should include full PR link when useShortPrLink is false', async () => {
276+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1']);
277+
mockRunCommand.mockResolvedValueOnce('add feature (#12345)');
278+
279+
const result = await getNewChangeEntries({
280+
mostRecentTag: 'v1.0.0',
281+
repoUrl,
282+
loggedPrNumbers: [],
283+
loggedDescriptions: [],
284+
useChangelogEntry: false,
285+
useShortPrLink: false,
286+
});
287+
288+
expect(result).toStrictEqual([
289+
{
290+
description:
291+
'add feature ([#12345](https://github.com/MetaMask/metamask-mobile/pull/12345))',
292+
subject: 'add feature (#12345)',
293+
},
294+
]);
295+
});
296+
297+
it('should use short PR link when useShortPrLink is true', async () => {
298+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1']);
299+
mockRunCommand.mockResolvedValueOnce('add feature (#12345)');
300+
301+
const result = await getNewChangeEntries({
302+
mostRecentTag: 'v1.0.0',
303+
repoUrl,
304+
loggedPrNumbers: [],
305+
loggedDescriptions: [],
306+
useChangelogEntry: false,
307+
useShortPrLink: true,
308+
});
309+
310+
expect(result).toStrictEqual([
311+
{
312+
description: 'add feature (#12345)',
313+
subject: 'add feature (#12345)',
314+
},
315+
]);
316+
});
317+
318+
it('should not add PR link suffix for direct commits', async () => {
319+
mockRunCommandAndSplit.mockResolvedValueOnce(['commit1']);
320+
mockRunCommand.mockResolvedValueOnce('Update Attributions');
321+
322+
const result = await getNewChangeEntries({
323+
mostRecentTag: 'v1.0.0',
324+
repoUrl,
325+
loggedPrNumbers: [],
326+
loggedDescriptions: [],
327+
useChangelogEntry: false,
328+
useShortPrLink: false,
329+
});
330+
331+
expect(result).toStrictEqual([
332+
{
333+
description: 'Update Attributions',
334+
subject: 'Update Attributions',
335+
},
336+
]);
337+
});
338+
});
339+
});

0 commit comments

Comments
 (0)