From a7590b41036b38276f0c201ce1834eb79f0b20f2 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:49:55 -0400 Subject: [PATCH] fix(branches): incorporate PR #1017 log/label fixes + mitigate integration appId harness Incorporate PR github-community-projects/safe-settings#1017 (absent from this branch), adapted to this branch's `this.github.repos` convention: - Branch-protection diff message read `params.branch.name` (always undefined, since `params.branch` is already the branch string) -> use `params.branch`, and JSON.stringify the results in the debug log. - NOP update path (protection already exists) was mislabeled 'Add Branch Protection' -> 'Update Branch Protection' (debug 'Updating'); the 404/add path keeps its 'Add' label. - Add NOP-mode unit tests asserting the update label when protection exists, the add label on 404, and that the diff message names the real branch. Integration harness: `createProbot` in probot 13 only reads overrides/defaults/env, so the old `{ id, cert, githubToken }` args were dropped, making @octokit/auth-app throw "appId option is required". Pass dummy credentials via `overrides` (token auth) and stub the startup `/app/installations` call so the app loads under nock.disableNetConnect(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c735bbe7-feb9-472f-827c-d56ddfe7fe6a --- lib/plugins/branches.js | 8 ++-- test/integration/common.js | 23 ++++++++++- test/unit/lib/plugins/branches.test.js | 54 ++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 5 deletions(-) diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index d28e2f905..2738d7003 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -57,8 +57,8 @@ module.exports = class Branches extends ErrorStash { return this.github.repos.getBranchProtection(params).then((result) => { const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields) const changes = mergeDeep.compareDeep({ branch: { protection: this.reformatAndReturnBranchProtection(result.data) } }, { branch: { protection: Overrides.removeOverrides(overrides, branch.protection, result.data) } }) - const results = { msg: `Followings changes will be applied to the branch protection for ${params.branch.name} branch`, additions: changes.additions, modifications: changes.modifications, deletions: changes.deletions } - this.log.debug(`Result of compareDeep = ${results}`) + const results = { msg: `The following changes will be applied to the branch protection for ${params.branch} branch`, additions: changes.additions, modifications: changes.modifications, deletions: changes.deletions } + this.log.debug(`Result of compareDeep = ${JSON.stringify(results)}`) if (!changes.hasChanges) { this.log.debug(`There are no changes for branch ${JSON.stringify(params)}. Skipping branch protection changes`) @@ -76,10 +76,10 @@ module.exports = class Branches extends ErrorStash { Object.assign(params, branch.protection, { headers: previewHeaders }) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) + resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Update Branch Protection')) return Promise.resolve(resArray) } - this.log.debug(`Adding branch protection ${JSON.stringify(params)}`) + this.log.debug(`Updating branch protection ${JSON.stringify(params)}`) return this.github.repos.updateBranchProtection(params).then(res => this.log.debug(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] }) }).catch((e) => { if (e.status === 404) { diff --git a/test/integration/common.js b/test/integration/common.js index 4d47b210a..4474db761 100644 --- a/test/integration/common.js +++ b/test/integration/common.js @@ -16,7 +16,28 @@ const repository = { } function loadInstance () { - const probot = createProbot({ id: 1, cert: 'test', githubToken: 'test' }) + // Probot 13's `createProbot` only reads `overrides`/`defaults`/`env`, so the + // old positional `{ id, cert, githubToken }` args were silently dropped, + // leaving no credentials and making `@octokit/auth-app` throw + // "appId option is required". Provide dummy credentials via `overrides`. + // Using a `githubToken` selects Octokit's token auth strategy, which avoids + // the app-auth JWT/installation-token calls that the nock scopes don't mock. + // + // The app also runs `info()` on load, which lists app installations. Stub + // that startup call with an empty list so it resolves cleanly under + // `nock.disableNetConnect()` without interfering with the per-test scopes. + nock('https://api.github.com') + .persist() + .get('/app/installations') + .query(true) + .reply(200, []) + + const probot = createProbot({ + overrides: { + appId: 1, + githubToken: 'test' + } + }) probot.load(settingsBot) return probot diff --git a/test/unit/lib/plugins/branches.test.js b/test/unit/lib/plugins/branches.test.js index 4b3683f34..ac33888ce 100644 --- a/test/unit/lib/plugins/branches.test.js +++ b/test/unit/lib/plugins/branches.test.js @@ -272,6 +272,60 @@ describe('Branches', () => { }) }) + describe('in nop mode', () => { + function configureNop (config) { + return new Branches(true, github, { owner: 'bkeepers', repo: 'test' }, config, log, []) + } + + beforeEach(() => { + github.repos.updateBranchProtection.endpoint = jest.fn().mockImplementation(params => { + return { url: 'updateBranchProtection', body: params } + }) + github.repos.deleteBranchProtection.endpoint = jest.fn().mockImplementation(params => { + return { url: 'deleteBranchProtection', body: params } + }) + }) + + describe('when branch protection already exists', () => { + it('labels the NopCommand as an update and names the branch in the diff message', () => { + const plugin = configureNop( + [{ + name: 'master', + protection: { enforce_admins: true } + }] + ) + + return plugin.sync().then(res => { + const messages = res.map(nopCommand => nopCommand.action.msg) + expect(messages).toContain('Update Branch Protection') + expect(messages).not.toContain('Add Branch Protection') + const diffMessage = messages.find(msg => typeof msg === 'string' && msg.includes('will be applied to the branch protection')) + expect(diffMessage).toBeDefined() + expect(diffMessage).toContain('for master branch') + expect(diffMessage).not.toContain('undefined') + }) + }) + }) + + describe('when branch protection does not exist yet', () => { + it('labels the NopCommand as an add', () => { + github.repos.getBranchProtection = jest.fn().mockRejectedValue({ status: 404 }) + const plugin = configureNop( + [{ + name: 'master', + protection: { enforce_admins: true } + }] + ) + + return plugin.sync().then(res => { + const messages = res.map(nopCommand => nopCommand.action.msg) + expect(messages).toContain('Add Branch Protection') + expect(messages).not.toContain('Update Branch Protection') + }) + }) + }) + }) + describe.skip('return values', () => { it('returns updateBranchProtection Promise', () => { const plugin = configure(