Include catalog dependency changes when checking for changed packages (#1220)

Author: ecraig12345

change/change-a2652114-ff41-439f-bf46-7913b4e8473a.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "type": "patch",
+      "comment": "Include catalog dependency changes when checking for changed packages",
+      "packageName": "beachball",
+      "email": "elcraig@microsoft.com",
+      "dependentChangeType": "patch"
+    }
+  ]
+}

packages/beachball/src/__e2e__/getChangedPackages.test.ts

@@ -1,4 +1,12 @@
 import { describe, expect, it, afterEach, beforeAll, afterAll, jest } from '@jest/globals';
+import {
+  addGitObserver,
+  clearGitObservers,
+  getPackageInfo as getWsPackageInfo,
+  type Catalogs,
+  catalogsToYaml,
+  getCatalogs,
+} from 'workspace-tools';
 import { defaultBranchName, defaultRemoteBranchName } from '../__fixtures__/gitDefaults';
 import { RepositoryFactory } from '../__fixtures__/repositoryFactory';
 import { getPackageInfos } from '../monorepo/getPackageInfos';
@@ -9,7 +17,6 @@ import { generateChangeFiles } from '../__fixtures__/changeFiles';
 import type { Repository } from '../__fixtures__/repository';
 import { getParsedOptions } from '../options/getOptions';
 import { getScopedPackages } from '../monorepo/getScopedPackages';
-import { addGitObserver, clearGitObservers, getPackageInfo as getWsPackageInfo } from 'workspace-tools';
 
 //
 // NOTE: These tests should mostly cover logic specific to getChangePackages itself.
@@ -19,12 +26,14 @@ describe('getChangedPackages', () => {
   const gitObserver = jest.fn();
   const logs = initMockLogs();
 
-  /** Factory reused for most tests */
+  let singleFactory: RepositoryFactory;
   let monorepoFactory: RepositoryFactory;
   const extraFactories: RepositoryFactory[] = [];
-  /** Monorepo reused for multiple tests where it's safe */
-  let _reusedRepo: Repository;
-  /** Repo for current test */
+  /** Repos reused for some tests where it's safe (don't use directly) */
+  let _reusedRepos: {
+    single: Repository;
+    monorepo: Repository;
+  }; /** Repo for current test */
   let repo: Repository | undefined;
 
   /**
@@ -33,8 +42,8 @@ describe('getChangedPackages', () => {
    * - create uncommitted files or directories (commited files are fine because they'll go away
    *   on branch change)
    */
-  function getReusedRepoWithBranch() {
-    repo = _reusedRepo;
+  function getReusedRepoWithBranch(type: 'single' | 'monorepo') {
+    repo = type === 'single' ? _reusedRepos.single : _reusedRepos.monorepo;
     repo.checkoutTestBranch();
     return repo;
   }
@@ -54,7 +63,9 @@ describe('getChangedPackages', () => {
   }
 
   /** Get options/context, clear `gitObserver` mock, and call `getChangedPackages` */
-  function getChangedPackagesWrapper(params: { repoOptions?: Partial<RepoOptions>; extraArgv?: string[] } = {}) {
+  function getChangedPackagesWrapper(
+    params: { repoOptions?: Partial<RepoOptions>; extraArgv?: string[]; cwd?: string } = {}
+  ) {
     const parsedOptions = getOptions(params);
     const packageInfos = getPackageInfos(parsedOptions);
     const scopedPackages = getScopedPackages(parsedOptions.options, packageInfos);
@@ -63,8 +74,12 @@ describe('getChangedPackages', () => {
   }
 
   beforeAll(() => {
+    singleFactory = new RepositoryFactory('single');
     monorepoFactory = new RepositoryFactory('monorepo');
-    _reusedRepo = monorepoFactory.cloneRepository();
+    _reusedRepos = {
+      single: singleFactory.cloneRepository(),
+      monorepo: monorepoFactory.cloneRepository(),
+    };
     addGitObserver(gitObserver);
   });
 
@@ -81,7 +96,7 @@ describe('getChangedPackages', () => {
   });
 
   it('returns the given package name(s) as-is', () => {
-    repo = getReusedRepoWithBranch();
+    repo = getReusedRepoWithBranch('single');
     expect(getChangedPackagesWrapper({ extraArgv: ['--package', 'foo'] })).toEqual(['foo']);
     expect(gitObserver).not.toHaveBeenCalled();
 
@@ -93,24 +108,108 @@ describe('getChangedPackages', () => {
     expect(gitObserver).not.toHaveBeenCalled();
   });
 
-  // Basic test as a sanity check, but simple file-based cases are mainly covered by getAllChangedPackages
+  // do a full test for each major repo structure
+  it('detects changed files in single-package repo', () => {
+    repo = getReusedRepoWithBranch('single');
+    repo.commitChange('myFilename');
+    expect(getChangedPackagesWrapper()).toEqual(['foo']);
+    expect(gitObserver).toHaveBeenCalled();
+  });
+
   it('detects changed files in monorepo', () => {
-    repo = getReusedRepoWithBranch();
+    repo = getReusedRepoWithBranch('monorepo');
 
     // empty if no changes yet
     expect(getChangedPackagesWrapper()).toEqual([]);
 
-    repo.commitChange('packages/foo/test.js');
-    expect(getChangedPackagesWrapper()).toEqual(['foo']);
+    repo.writeFile('packages/foo/test.js');
+    repo.writeFile('packages/bar/test.js');
+    repo.commitAll();
 
-    expect(logs.getMockLines('all', { sanitize: true })).toMatchInlineSnapshot(`
+    logs.clear();
+    const result = getChangedPackagesWrapper({ extraArgv: ['--verbose'] });
+    expect(result.sort()).toEqual(['bar', 'foo']);
+    expect(gitObserver).toHaveBeenCalled();
+
+    expect(logs.getMockLines('all')).toMatchInlineSnapshot(`
+        "[log] Checking for changes against "origin/master"
+        [log] Found 2 changed files in current branch (before filtering)
+        [log]   - packages/bar/test.js
+        [log]   - packages/foo/test.js
+        [log] Found 2 files in 2 packages that should be published"
+      `);
+  });
+
+  it('detects changed files in multi-project monorepo', () => {
+    // this is the only multi-project test
+    const multiFactory = new RepositoryFactory('multi-project');
+    extraFactories.push(multiFactory);
+    repo = multiFactory.cloneRepository();
+    expect(Object.keys(multiFactory.fixtures)).toEqual(['project-a', 'project-b']);
+
+    const projectARoot = repo.pathTo('project-a');
+    const projectBRoot = repo.pathTo('project-b');
+
+    repo.stageChange('project-a/packages/foo/test.js');
+
+    const changedPackagesA = getChangedPackagesWrapper({ cwd: projectARoot });
+    const changedPackagesB = getChangedPackagesWrapper({ cwd: projectBRoot });
+
+    expect(changedPackagesA).toEqual(['@project-a/foo']);
+    expect(changedPackagesB).toEqual([]);
+  });
+
+  // Do one real combined test with ignores to ensure all the path handling works
+  // (the logic is mostly covered by getAllChangedPackages tests)
+  it('ignores CHANGELOG, change files, and ignorePatterns in single-package repo', () => {
+    repo = getReusedRepoWithBranch('single');
+
+    repo.writeFile('change/change-abc123.json', {});
+    repo.writeFile('CHANGELOG.md');
+    repo.writeFile('src/foo.test.js');
+    repo.writeFile('src/foo.test.js');
+    repo.writeFile('tests/stuff.js');
+    repo.writeFile('yarn.lock');
+    repo.commitAll();
+
+    const result = getChangedPackagesWrapper({
+      repoOptions: { ignorePatterns: ['*.test.js', 'tests/**', 'yarn.lock'] },
+      extraArgv: ['--verbose'],
+    });
+    expect(result).toEqual([]);
+
+    const logLines = logs.getMockLines('all');
+    expect(logLines).toMatch('ignored by pattern');
+    expect(logLines).toMatchInlineSnapshot(`
       "[log] Checking for changes against "origin/master"
-      [log] Checking for changes against "origin/master""
+      [log] Found 4 changed files in current branch (before filtering)
+      [log]   - ~~CHANGELOG.md~~ (ignored by pattern "CHANGELOG.{md,json}")
+      [log]   - ~~change/change-abc123.json~~ (ignored by pattern "change/*.json")
+      [log]   - ~~src/foo.test.js~~ (ignored by pattern "*.test.js")
+      [log]   - ~~tests/stuff.js~~ (ignored by pattern "tests/**")
+      [log] All files were ignored"
     `);
   });
 
+  it('includes staged files', () => {
+    // need a separate repo since it stages changes
+    repo = singleFactory.cloneRepository();
+    repo.checkout('-b', 'test-staged');
+    // write this file and modify it
+    repo.commitChange('src/foo.js');
+    repo.writeFile('src/bar.js');
+    // add a new file that's only staged
+    repo.writeFile('src/foo.js');
+    repo.git(['add', '-A']);
+
+    const result = getChangedPackagesWrapper({ extraArgv: ['--verbose'] });
+    expect(result).toEqual(['foo']);
+    // src/foo.js is both committed and staged, but should be listed only once
+    expect(logs.getMockLines('all')).toContain('Found 2 changed files in current branch (before filtering)');
+  });
+
   it('excludes packages that already have change files', () => {
-    repo = getReusedRepoWithBranch();
+    repo = getReusedRepoWithBranch('monorepo');
 
     // setup: change foo, create a change file, commit
     repo.checkout('-b', 'test');
@@ -152,7 +251,7 @@ describe('getChangedPackages', () => {
   });
 
   it('returns all packages with all: true, removing those with change files', () => {
-    repo = getReusedRepoWithBranch();
+    repo = getReusedRepoWithBranch('monorepo');
     generateChangeFiles(['foo', 'a'], { ...getOptions().options, commit: true });
 
     const result = getChangedPackagesWrapper({ extraArgv: ['--all'] });
@@ -161,7 +260,7 @@ describe('getChangedPackages', () => {
   });
 
   it('throws if the remote is invalid', () => {
-    repo = getReusedRepoWithBranch();
+    repo = getReusedRepoWithBranch('monorepo');
     const customRemote = 'foo';
     repo.git(['remote', 'add', customRemote, 'file:///__nonexistent']);
     repo.commitChange('fake.js');
@@ -216,4 +315,43 @@ describe('getChangedPackages', () => {
       [log] Found 1 file in 1 package that should be published"
     `);
   });
+
+  it('includes catalog changes in the list of changed packages', () => {
+    // This needs a separate factory since it pushes changes
+    const repositoryFactory = new RepositoryFactory('monorepo');
+    extraFactories.push(repositoryFactory);
+    repo = repositoryFactory.cloneRepository();
+
+    // Add catalog dependencies
+    const initialCatalogs: Catalogs = { default: { react: '18.0.0', other: '1.0.0' } };
+    repo.writeFile('.yarnrc.yml', catalogsToYaml(initialCatalogs));
+    repo.updateJsonFile('packages/foo/package.json', { dependencies: { react: 'catalog:', other: 'catalog:' } });
+    repo.updateJsonFile('packages/bar/package.json', { dependencies: { react: 'catalog:' } });
+    repo.commitAll('add catalogs');
+    repo.push();
+    // sanity check
+    expect(getCatalogs(repo.rootPath)).toEqual(initialCatalogs);
+
+    // Update a catalog version, and make unrelated changes to verify merging
+    const newCatalogs: Catalogs = { default: { react: '19.0.0', other: '1.0.0' } };
+    repo.writeFile('.yarnrc.yml', catalogsToYaml(newCatalogs));
+    repo.writeFile('packages/foo/test.js', '');
+    repo.writeFile('packages/grouped/a/foo.ts', '');
+    repo.commitAll();
+
+    const result = getChangedPackagesWrapper({ extraArgv: ['--verbose'] });
+    expect(result.sort()).toEqual(['a', 'bar', 'foo']);
+    expect(logs.getMockLines('all', { sanitize: true })).toMatchInlineSnapshot(`
+      "[log] Checking for changes against "origin/master"
+      [log] Found 3 changed files in current branch (before filtering)
+      [log]   - ~~.yarnrc.yml~~ (not in a package)
+      [log]   - packages/foo/test.js
+      [log]   - packages/grouped/a/foo.ts
+      [log] Found 2 files in 2 packages that should be published
+      [log] Checking for changes to catalog: dependencies...
+      [log] catalog: dependencies referenced by the following packages have changed:
+      [log]   - bar: react
+      [log]   - foo: react"
+    `);
+  });
 });