From aa10135b87e140a4bb9fa084651cb3135bbe805e Mon Sep 17 00:00:00 2001 From: Bharat Middha <5100938+bmiddha@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:55:40 -0700 Subject: [PATCH 1/2] fix: preserve publishing change-file formatting Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0c4de41b-5d91-49d1-b662-d7a29415bc16 --- .github/workflows/ci.yml | 4 + common/scripts/publish-cohort.js | 15 +- common/scripts/test/publish-cohort.test.js | 171 +++++++++++++++++++++ 3 files changed, 183 insertions(+), 7 deletions(-) create mode 100644 common/scripts/test/publish-cohort.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c7df805438..fa424585b94 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,10 @@ jobs: with: node-version: ${{ matrix.NodeVersion }} + - name: Test publishing cohort script + run: node --test common/scripts/test/publish-cohort.test.js + working-directory: repo-a + - name: Verify Change Logs run: node common/scripts/install-run-rush.js change --verify working-directory: repo-a diff --git a/common/scripts/publish-cohort.js b/common/scripts/publish-cohort.js index 21e007c1a0a..eff8280bed1 100644 --- a/common/scripts/publish-cohort.js +++ b/common/scripts/publish-cohort.js @@ -97,7 +97,7 @@ function forEachFileRecursive(folderPath, extension, callback) { function writeJson(filePath, value) { fs.mkdirSync(path.dirname(filePath), { recursive: true }); - fs.writeFileSync(filePath, `${JSON.stringify(value)}\n`); + fs.writeFileSync(filePath, `${JSON.stringify(value, null, 2)}\n`); } function partitionChanges(repoPath, backupPath, cohort) { @@ -125,16 +125,16 @@ function partitionChanges(repoPath, backupPath, cohort) { const backupFilePath = path.join(backupPath, relativePath); if (includedChanges.length === 0) { fs.mkdirSync(path.dirname(backupFilePath), { recursive: true }); - fs.writeFileSync(backupFilePath, changeFileText); + fs.copyFileSync(changeFilePath, backupFilePath); } else { writeJson(backupFilePath, { ...changeFile, changes: excludedChanges }); } } - if (includedChanges.length > 0) { - writeJson(changeFilePath, { ...changeFile, changes: includedChanges }); - } else { + if (includedChanges.length === 0) { fs.rmSync(changeFilePath); + } else if (excludedChanges.length > 0) { + writeJson(changeFilePath, { ...changeFile, changes: includedChanges }); } }); } @@ -144,16 +144,17 @@ function restoreChanges(repoPath, backupPath) { forEachFileRecursive(backupPath, '.json', (backupFilePath, relativePath) => { const changeFilePath = path.join(changesPath, relativePath); - const backupChangeFile = JSON.parse(fs.readFileSync(backupFilePath, 'utf8')); if (fs.existsSync(changeFilePath)) { + const backupChangeFile = JSON.parse(fs.readFileSync(backupFilePath, 'utf8')); const currentChangeFile = JSON.parse(fs.readFileSync(changeFilePath, 'utf8')); writeJson(changeFilePath, { ...backupChangeFile, changes: [...currentChangeFile.changes, ...backupChangeFile.changes] }); } else { - writeJson(changeFilePath, backupChangeFile); + fs.mkdirSync(path.dirname(changeFilePath), { recursive: true }); + fs.copyFileSync(backupFilePath, changeFilePath); } }); } diff --git a/common/scripts/test/publish-cohort.test.js b/common/scripts/test/publish-cohort.test.js new file mode 100644 index 00000000000..0aab55fd875 --- /dev/null +++ b/common/scripts/test/publish-cohort.test.js @@ -0,0 +1,171 @@ +const assert = require('node:assert/strict'); +const childProcess = require('node:child_process'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { test } = require('node:test'); + +const scriptPath = path.resolve(__dirname, '../publish-cohort.js'); +const includedChange = { packageName: 'individual-package', comment: 'Keep this change.', type: 'none' }; +const consumedChange = { packageName: 'individual-package', comment: 'Fix a bug.', type: 'patch' }; +const excludedChange = { packageName: 'rush-package', comment: 'Next Rush release.', type: 'minor' }; +const metadata = { + packageName: 'individual-package', + email: 'author@example.com', + customMetadata: { source: 'test' } +}; + +function prettyJson(value) { + return `${JSON.stringify(value, null, 2)}\n`; +} + +function assertPrettyJson(filePath, expected) { + const text = fs.readFileSync(filePath, 'utf8'); + assert.deepStrictEqual(JSON.parse(text), expected); + assert.strictEqual(text, prettyJson(expected)); +} + +function createFixture(t, changeFileText) { + const repoPath = fs.mkdtempSync(path.join(os.tmpdir(), 'publish-cohort-')); + t.after(() => fs.rmSync(repoPath, { recursive: true, force: true })); + + const scriptsPath = path.join(repoPath, 'common/scripts'); + fs.mkdirSync(scriptsPath, { recursive: true }); + const projectList = { + projects: [ + { name: 'individual-package' }, + { name: 'rush-package', versionPolicyName: 'rush' } + ] + }; + fs.writeFileSync( + path.join(scriptsPath, 'install-run-rush.js'), + `console.log(JSON.stringify(${JSON.stringify(projectList)}));\n` + ); + + const relativePath = path.join('@scope', 'package', 'change.json'); + const changeFilePath = path.join(repoPath, 'common/changes', relativePath); + fs.mkdirSync(path.dirname(changeFilePath), { recursive: true }); + fs.writeFileSync(changeFilePath, changeFileText); + + const backupPath = path.join(repoPath, 'excluded changes'); + return { + changeFilePath, + backupPath, + backupFilePath: path.join(backupPath, relativePath), + run(command, cohort) { + const result = childProcess.spawnSync( + process.execPath, + [ + scriptPath, + command, + '--repo-path', + repoPath, + '--backup-path', + backupPath, + ...(cohort ? ['--cohort', cohort] : []) + ], + { encoding: 'utf8' } + ); + assert.ifError(result.error); + assert.strictEqual(result.status, 0, result.stdout + result.stderr); + } + }; +} + +const formats = [ + ['two-space indentation and LF', prettyJson], + ['tabs and CRLF', (value) => `${JSON.stringify(value, null, '\t').replace(/\n/g, '\r\n')}\r\n`], + ['compact JSON without a final newline', (value) => JSON.stringify(value)] +]; + +for (const [description, format] of formats) { + test(`fully included files retain ${description}`, (t) => { + const text = format({ ...metadata, changes: [includedChange] }); + const fixture = createFixture(t, text); + + fixture.run('partition-changes', 'noRush'); + assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); + assert.strictEqual(fs.existsSync(fixture.backupFilePath), false); + + fixture.run('restore-changes'); + assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); + }); + + test(`fully excluded files retain ${description}`, (t) => { + const text = format({ ...metadata, changes: [excludedChange] }); + const fixture = createFixture(t, text); + + fixture.run('partition-changes', 'noRush'); + assert.strictEqual(fs.existsSync(fixture.changeFilePath), false); + assert.strictEqual(fs.readFileSync(fixture.backupFilePath, 'utf8'), text); + + fs.rmdirSync(path.dirname(fixture.changeFilePath)); + fixture.run('restore-changes'); + assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); + }); +} + +for (const cohort of ['rush', 'all']) { + test(`${cohort} selects the expected changes without rewriting included files`, (t) => { + const changes = cohort === 'rush' ? [excludedChange] : [includedChange, excludedChange]; + const text = JSON.stringify({ ...metadata, changes }); + const fixture = createFixture(t, text); + + fixture.run('partition-changes', cohort); + assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); + assert.strictEqual(fs.existsSync(fixture.backupFilePath), false); + }); +} + +test('mixed files preserve metadata and use pretty JSON when split and merged', (t) => { + const fixture = createFixture( + t, + JSON.stringify({ ...metadata, changes: [includedChange, excludedChange] }) + ); + + fixture.run('partition-changes', 'noRush'); + assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange] }); + assertPrettyJson(fixture.backupFilePath, { ...metadata, changes: [excludedChange] }); + + fixture.run('restore-changes'); + assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange, excludedChange] }); +}); + +test('mixed files restore excluded entries alongside surviving included entries', (t) => { + const fixture = createFixture( + t, + prettyJson({ ...metadata, changes: [includedChange, excludedChange, consumedChange] }) + ); + + fixture.run('partition-changes', 'noRush'); + assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange, consumedChange] }); + assertPrettyJson(fixture.backupFilePath, { ...metadata, changes: [excludedChange] }); + + fs.writeFileSync(fixture.changeFilePath, prettyJson({ ...metadata, changes: [includedChange] })); + fixture.run('restore-changes'); + assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange, excludedChange] }); +}); + +test('mixed files restore only excluded entries when all included entries were consumed', (t) => { + const fixture = createFixture( + t, + prettyJson({ ...metadata, changes: [consumedChange, excludedChange] }) + ); + + fixture.run('partition-changes', 'noRush'); + assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [consumedChange] }); + assertPrettyJson(fixture.backupFilePath, { ...metadata, changes: [excludedChange] }); + + fs.rmSync(fixture.changeFilePath); + fixture.run('restore-changes'); + assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [excludedChange] }); +}); + +test('fully included files stay deleted when their changes were consumed', (t) => { + const fixture = createFixture(t, prettyJson({ ...metadata, changes: [consumedChange] })); + + fixture.run('partition-changes', 'noRush'); + fs.rmSync(fixture.changeFilePath); + fixture.run('restore-changes'); + assert.strictEqual(fs.existsSync(fixture.changeFilePath), false); +}); From 481d8c83fa63cd33bfea9c26a1381df79c5be556 Mon Sep 17 00:00:00 2001 From: Bharat Middha <5100938+bmiddha@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:05:02 -0700 Subject: [PATCH 2/2] test: remove publishing cohort regression suite Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0c4de41b-5d91-49d1-b662-d7a29415bc16 --- .github/workflows/ci.yml | 4 - common/scripts/test/publish-cohort.test.js | 171 --------------------- 2 files changed, 175 deletions(-) delete mode 100644 common/scripts/test/publish-cohort.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fa424585b94..1c7df805438 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,10 +60,6 @@ jobs: with: node-version: ${{ matrix.NodeVersion }} - - name: Test publishing cohort script - run: node --test common/scripts/test/publish-cohort.test.js - working-directory: repo-a - - name: Verify Change Logs run: node common/scripts/install-run-rush.js change --verify working-directory: repo-a diff --git a/common/scripts/test/publish-cohort.test.js b/common/scripts/test/publish-cohort.test.js deleted file mode 100644 index 0aab55fd875..00000000000 --- a/common/scripts/test/publish-cohort.test.js +++ /dev/null @@ -1,171 +0,0 @@ -const assert = require('node:assert/strict'); -const childProcess = require('node:child_process'); -const fs = require('node:fs'); -const os = require('node:os'); -const path = require('node:path'); -const { test } = require('node:test'); - -const scriptPath = path.resolve(__dirname, '../publish-cohort.js'); -const includedChange = { packageName: 'individual-package', comment: 'Keep this change.', type: 'none' }; -const consumedChange = { packageName: 'individual-package', comment: 'Fix a bug.', type: 'patch' }; -const excludedChange = { packageName: 'rush-package', comment: 'Next Rush release.', type: 'minor' }; -const metadata = { - packageName: 'individual-package', - email: 'author@example.com', - customMetadata: { source: 'test' } -}; - -function prettyJson(value) { - return `${JSON.stringify(value, null, 2)}\n`; -} - -function assertPrettyJson(filePath, expected) { - const text = fs.readFileSync(filePath, 'utf8'); - assert.deepStrictEqual(JSON.parse(text), expected); - assert.strictEqual(text, prettyJson(expected)); -} - -function createFixture(t, changeFileText) { - const repoPath = fs.mkdtempSync(path.join(os.tmpdir(), 'publish-cohort-')); - t.after(() => fs.rmSync(repoPath, { recursive: true, force: true })); - - const scriptsPath = path.join(repoPath, 'common/scripts'); - fs.mkdirSync(scriptsPath, { recursive: true }); - const projectList = { - projects: [ - { name: 'individual-package' }, - { name: 'rush-package', versionPolicyName: 'rush' } - ] - }; - fs.writeFileSync( - path.join(scriptsPath, 'install-run-rush.js'), - `console.log(JSON.stringify(${JSON.stringify(projectList)}));\n` - ); - - const relativePath = path.join('@scope', 'package', 'change.json'); - const changeFilePath = path.join(repoPath, 'common/changes', relativePath); - fs.mkdirSync(path.dirname(changeFilePath), { recursive: true }); - fs.writeFileSync(changeFilePath, changeFileText); - - const backupPath = path.join(repoPath, 'excluded changes'); - return { - changeFilePath, - backupPath, - backupFilePath: path.join(backupPath, relativePath), - run(command, cohort) { - const result = childProcess.spawnSync( - process.execPath, - [ - scriptPath, - command, - '--repo-path', - repoPath, - '--backup-path', - backupPath, - ...(cohort ? ['--cohort', cohort] : []) - ], - { encoding: 'utf8' } - ); - assert.ifError(result.error); - assert.strictEqual(result.status, 0, result.stdout + result.stderr); - } - }; -} - -const formats = [ - ['two-space indentation and LF', prettyJson], - ['tabs and CRLF', (value) => `${JSON.stringify(value, null, '\t').replace(/\n/g, '\r\n')}\r\n`], - ['compact JSON without a final newline', (value) => JSON.stringify(value)] -]; - -for (const [description, format] of formats) { - test(`fully included files retain ${description}`, (t) => { - const text = format({ ...metadata, changes: [includedChange] }); - const fixture = createFixture(t, text); - - fixture.run('partition-changes', 'noRush'); - assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); - assert.strictEqual(fs.existsSync(fixture.backupFilePath), false); - - fixture.run('restore-changes'); - assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); - }); - - test(`fully excluded files retain ${description}`, (t) => { - const text = format({ ...metadata, changes: [excludedChange] }); - const fixture = createFixture(t, text); - - fixture.run('partition-changes', 'noRush'); - assert.strictEqual(fs.existsSync(fixture.changeFilePath), false); - assert.strictEqual(fs.readFileSync(fixture.backupFilePath, 'utf8'), text); - - fs.rmdirSync(path.dirname(fixture.changeFilePath)); - fixture.run('restore-changes'); - assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); - }); -} - -for (const cohort of ['rush', 'all']) { - test(`${cohort} selects the expected changes without rewriting included files`, (t) => { - const changes = cohort === 'rush' ? [excludedChange] : [includedChange, excludedChange]; - const text = JSON.stringify({ ...metadata, changes }); - const fixture = createFixture(t, text); - - fixture.run('partition-changes', cohort); - assert.strictEqual(fs.readFileSync(fixture.changeFilePath, 'utf8'), text); - assert.strictEqual(fs.existsSync(fixture.backupFilePath), false); - }); -} - -test('mixed files preserve metadata and use pretty JSON when split and merged', (t) => { - const fixture = createFixture( - t, - JSON.stringify({ ...metadata, changes: [includedChange, excludedChange] }) - ); - - fixture.run('partition-changes', 'noRush'); - assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange] }); - assertPrettyJson(fixture.backupFilePath, { ...metadata, changes: [excludedChange] }); - - fixture.run('restore-changes'); - assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange, excludedChange] }); -}); - -test('mixed files restore excluded entries alongside surviving included entries', (t) => { - const fixture = createFixture( - t, - prettyJson({ ...metadata, changes: [includedChange, excludedChange, consumedChange] }) - ); - - fixture.run('partition-changes', 'noRush'); - assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange, consumedChange] }); - assertPrettyJson(fixture.backupFilePath, { ...metadata, changes: [excludedChange] }); - - fs.writeFileSync(fixture.changeFilePath, prettyJson({ ...metadata, changes: [includedChange] })); - fixture.run('restore-changes'); - assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [includedChange, excludedChange] }); -}); - -test('mixed files restore only excluded entries when all included entries were consumed', (t) => { - const fixture = createFixture( - t, - prettyJson({ ...metadata, changes: [consumedChange, excludedChange] }) - ); - - fixture.run('partition-changes', 'noRush'); - assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [consumedChange] }); - assertPrettyJson(fixture.backupFilePath, { ...metadata, changes: [excludedChange] }); - - fs.rmSync(fixture.changeFilePath); - fixture.run('restore-changes'); - assertPrettyJson(fixture.changeFilePath, { ...metadata, changes: [excludedChange] }); -}); - -test('fully included files stay deleted when their changes were consumed', (t) => { - const fixture = createFixture(t, prettyJson({ ...metadata, changes: [consumedChange] })); - - fixture.run('partition-changes', 'noRush'); - fs.rmSync(fixture.changeFilePath); - fixture.run('restore-changes'); - assert.strictEqual(fs.existsSync(fixture.changeFilePath), false); -});