Prevent accidentally writing dep bumps to grouped changelog (and test…

… improvements) (#984)

change/beachball-5d27a64f-c23d-4ae8-8848-d424d7fef666.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Prevent accidentally writing dep bumps to grouped changelog",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

src/__fixtures__/changeFiles.ts

@@ -2,29 +2,51 @@ import fs from 'fs';
 import path from 'path';
 import { writeChangeFiles } from '../changefile/writeChangeFiles';
 import { getChangePath } from '../paths';
-import { ChangeFileInfo } from '../types/ChangeInfo';
+import { ChangeFileInfo, ChangeType } from '../types/ChangeInfo';
 import type { BeachballOptions } from '../types/BeachballOptions';
 
 /** Change file with `packageName` required and other props optional */
 export type PartialChangeFile = { packageName: string } & Partial<ChangeFileInfo>;
 
+/** Placeholder email/author */
+export const fakeEmail = 'test@test.com';
+
+/**
+ * Generate a change file for the given package.
+ */
+export function getChange(
+  packageName: string,
+  comment: string = `${packageName} comment`,
+  type: ChangeType = 'minor'
+): ChangeFileInfo {
+  return {
+    comment,
+    email: fakeEmail,
+    packageName,
+    type,
+    dependentChangeType: 'patch',
+  };
+}
+
 /**
  * Generates and writes change files for the given packages.
+ * Also commits if `options.commit` is true.
  * @param changes Array of package names or partial change files (which must include `packageName`).
- * Default values are `type: 'minor'`, `dependentChangeType: 'patch'`, and placeholders for other fields.
+ * Default values:
+ * - `type: 'minor'`
+ * - `dependentChangeType: 'patch'`
+ * - `comment: '<packageName> comment'`
+ * - `email: 'test@test.com'`
  */
 export function generateChangeFiles(
   changes: (string | PartialChangeFile)[],
-  options: Pick<BeachballOptions, 'path' | 'groupChanges' | 'changeDir'>
+  options: Parameters<typeof writeChangeFiles>[1]
 ): void {
   writeChangeFiles(
     changes.map(change => {
       change = typeof change === 'string' ? { packageName: change } : change;
       return {
-        comment: `${change.packageName} test comment`,
-        email: 'test@test.com',
-        type: 'minor',
-        dependentChangeType: 'patch',
+        ...getChange(change.packageName, undefined, 'minor'),
         ...change,
       };
     }),

src/__fixtures__/changelog.ts

@@ -1,8 +1,11 @@
 import fs from 'fs-extra';
 import path from 'path';
 import _ from 'lodash';
-import { SortedChangeTypes } from '../changefile/changeTypes';
 import { ChangelogJson } from '../types/ChangeLog';
+import { markerComment } from '../changelog/renderChangelog';
+
+/** Placeholder commit as replaced by cleanChangelogJson */
+export const fakeCommit = '(sha1)';
 
 /**
  * Read the CHANGELOG.md under the given package path, sanitizing any dates for snapshots.
@@ -17,6 +20,11 @@ export function readChangelogMd(packagePath: string): string | null {
   return text.replace(/\w\w\w, \d\d \w\w\w [\d :]+?GMT/gm, '(date)');
 }
 
+/** Get only the part of CHANGELOG.md after the marker comment. */
+export function trimChangelogMd(changelogMd: string): string {
+  return changelogMd.split(markerComment)[1].trim();
+}
+
 /**
  * Read the CHANGELOG.json under the given package path.
  * Returns null if it doesn't exist.
@@ -39,19 +47,17 @@ export function cleanChangelogJson(changelog: ChangelogJson | null): ChangelogJs
     return null;
   }
   changelog = _.cloneDeep(changelog);
-  // for a better snapshot, make the fake commit match if the real commit did
-  const fakeCommits: { [commit: string]: string } = {};
-  let fakeHashNum = 0;
 
   for (const entry of changelog.entries) {
-    entry.date = '(date)';
-    for (const changeType of SortedChangeTypes) {
-      if (entry.comments[changeType]) {
-        for (const comment of entry.comments[changeType]!) {
-          if (!fakeCommits[comment.commit]) {
-            fakeCommits[comment.commit] = `(sha1-${fakeHashNum++})`;
-          }
-          comment.commit = fakeCommits[comment.commit];
+    // Only replace properties if they existed, to help catch bugs if things are no longer written
+    if (entry.date) {
+      entry.date = '(date)';
+    }
+
+    for (const comments of Object.values(entry.comments)) {
+      for (const comment of comments!) {
+        if (comment.commit) {
+          comment.commit = fakeCommit;
         }
       }
     }

src/__fixtures__/repository.ts

@@ -2,7 +2,13 @@ import path from 'path';
 import * as fs from 'fs-extra';
 import { tmpdir } from './tmpdir';
 import { git } from 'workspace-tools';
-import { defaultBranchName, defaultRemoteName, optsWithLang, setDefaultBranchName } from './gitDefaults';
+import {
+  defaultBranchName,
+  defaultRemoteBranchName,
+  defaultRemoteName,
+  optsWithLang,
+  setDefaultBranchName,
+} from './gitDefaults';
 import { env } from '../env';
 
 /**
@@ -204,6 +210,12 @@ ${gitResult.stderr.toString()}`);
     this.git(['push', defaultRemoteName, `HEAD:${branchName}`]);
   }
 
+  /** `git reset --hard <ref>` and `git clean -dfx` */
+  resetAndClean(ref: string = defaultRemoteBranchName) {
+    this.git(['reset', '--hard', ref]);
+    this.git(['clean', '-dfx']);
+  }
+
   /**
    * Clean up the repo IF this is a local build.
    *

src/__functional__/changefile/readChangeFiles.test.ts

@@ -15,6 +15,8 @@ describe('readChangeFiles', () => {
   let repositoryFactory: RepositoryFactory;
   let monoRepoFactory: RepositoryFactory;
   let repo: Repository | undefined;
+  let sharedSingleRepo: Repository;
+  let sharedMonoRepo: Repository;
 
   const logs = initMockLogs();
 
@@ -28,13 +30,17 @@ describe('readChangeFiles', () => {
   }
 
   beforeAll(() => {
-    // These tests can share the same repo factories because they don't push to origin
-    // (the actual tests run against a clone)
+    // These tests can share the same factories and repos because they don't push to the remote,
+    // and the repo used is reset after each test (which is faster than making new clones).
     repositoryFactory = new RepositoryFactory('single');
     monoRepoFactory = new RepositoryFactory('monorepo');
+    sharedSingleRepo = repositoryFactory.cloneRepository();
+    sharedMonoRepo = monoRepoFactory.cloneRepository();
   });
 
   afterEach(() => {
+    // Revert whichever shared repo was used to the original state
+    repo?.resetAndClean();
     repo = undefined;
   });
 
@@ -44,7 +50,7 @@ describe('readChangeFiles', () => {
   });
 
   it('does not add commit hash', () => {
-    repo = repositoryFactory.cloneRepository();
+    repo = sharedSingleRepo;
     repo.commitChange('foo');
 
     const options = getOptions();
@@ -57,7 +63,7 @@ describe('readChangeFiles', () => {
   });
 
   it('reads from a custom changeDir', () => {
-    repo = repositoryFactory.cloneRepository();
+    repo = sharedSingleRepo;
     repo.commitChange('foo');
 
     const options = getOptions({ changeDir: 'changeDir' });
@@ -69,7 +75,7 @@ describe('readChangeFiles', () => {
   });
 
   it('excludes invalid change files', () => {
-    repo = monoRepoFactory.cloneRepository();
+    repo = sharedMonoRepo;
     repo.updateJsonFile('packages/bar/package.json', { private: true });
     const options = getOptions();
 
@@ -87,7 +93,7 @@ describe('readChangeFiles', () => {
   });
 
   it('excludes invalid changes from grouped change file in monorepo', () => {
-    repo = monoRepoFactory.cloneRepository();
+    repo = sharedMonoRepo;
     repo.updateJsonFile('packages/bar/package.json', { private: true });
 
     const options = getOptions({ groupChanges: true });
@@ -106,7 +112,7 @@ describe('readChangeFiles', () => {
   });
 
   it('excludes out of scope change files in monorepo', () => {
-    repo = monoRepoFactory.cloneRepository();
+    repo = sharedMonoRepo;
 
     const options = getOptions({ scope: ['packages/foo'] });
 
@@ -119,7 +125,7 @@ describe('readChangeFiles', () => {
   });
 
   it('excludes out of scope changes from grouped change file in monorepo', () => {
-    repo = monoRepoFactory.cloneRepository();
+    repo = sharedMonoRepo;
 
     const options = getOptions({ scope: ['packages/foo'], groupChanges: true });
 
@@ -133,7 +139,7 @@ describe('readChangeFiles', () => {
 
   it('runs transform.changeFiles functions if provided', async () => {
     const editedComment: string = 'Edited comment for testing';
-    repo = monoRepoFactory.cloneRepository();
+    repo = sharedMonoRepo;
 
     const options = getOptions({
       command: 'change',

src/__functional__/changefile/writeChangeFiles.test.ts

@@ -23,7 +23,7 @@ function cleanChangeFilePaths(root: string, changeFiles: string[]) {
 
 describe('writeChangeFiles', () => {
   let monorepoFactory: RepositoryFactory;
-  let repo: Repository | undefined;
+  let repo: Repository;
 
   initMockLogs();
 
@@ -40,18 +40,18 @@ describe('writeChangeFiles', () => {
     // These tests can share the same repo factories because they don't push to origin
     // (the actual tests run against a clone)
     monorepoFactory = new RepositoryFactory('monorepo');
+    repo = monorepoFactory.cloneRepository();
   });
 
   afterEach(() => {
-    repo = undefined;
+    repo?.resetAndClean();
   });
 
   afterAll(() => {
     monorepoFactory.cleanUp();
   });
 
   it('writes individual change files', () => {
-    repo = monorepoFactory.cloneRepository();
     const previousHead = repo.getCurrentHash();
     const options = getOptions();
 
@@ -76,8 +76,6 @@ describe('writeChangeFiles', () => {
   });
 
   it('respects changeDir option', () => {
-    repo = monorepoFactory.cloneRepository();
-
     const testChangeDir = 'myChangeDir';
     const options = getOptions({ changeDir: testChangeDir });
 
@@ -95,7 +93,6 @@ describe('writeChangeFiles', () => {
   });
 
   it('respects commit=false', () => {
-    repo = monorepoFactory.cloneRepository();
     const previousHead = repo.getCurrentHash();
 
     const options = getOptions({ commit: false });
@@ -117,8 +114,6 @@ describe('writeChangeFiles', () => {
   });
 
   it('writes grouped change files', () => {
-    repo = monorepoFactory.cloneRepository();
-
     const options = getOptions({
       groupChanges: true,
     });