diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index d60a20cc3..6ba08796c 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -30,12 +30,12 @@ module.exports = class Branches extends ErrorStash { } sync () { - const resArray = [] return this.github.rest.repos.get(this.repo).then((currentRepo) => { return Promise.all( this.branches .filter(branch => branch.protection !== undefined) .map(branch => { + const branchResults = [] // If branch protection is empty if (this.isEmpty(branch.protection)) { let p = Object.assign(this.repo, { branch: branch.name }) @@ -46,10 +46,10 @@ module.exports = class Branches extends ErrorStash { // Hack to handle closures and keep params from changing const params = Object.assign({}, p) if (this.nop) { - resArray.push( + branchResults.push( new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.deleteBranchProtection.endpoint(params), 'Delete Branch Protection') ) - return Promise.resolve(resArray) + return Promise.resolve(branchResults) } return this.github.rest.repos.deleteBranchProtection(params).catch(e => { return [] }) @@ -71,21 +71,21 @@ module.exports = class Branches extends ErrorStash { if (!changes.hasChanges) { this.log.debug(`There are no changes for branch ${JSON.stringify(params)}. Skipping branch protection changes`) if (this.nop) { - return Promise.resolve(resArray) + return Promise.resolve(branchResults) } return Promise.resolve() } this.log.debug(`There are changes for branch ${JSON.stringify(params)}\n ${JSON.stringify(changes)} \n Branch protection will be applied`) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, null, results)) + branchResults.push(new NopCommand(this.constructor.name, this.repo, null, results)) } Object.assign(params, requiredBranchProtectionDefaults, this.reformatAndReturnBranchProtection(structuredClone(result.data)), Overrides.removeOverrides(overrides, branch.protection, result.data), { headers: previewHeaders }) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) - return Promise.resolve(resArray) + branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) + return Promise.resolve(branchResults) } this.log.debug(`Adding branch protection ${JSON.stringify(params)}`) return this.github.rest.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 [] }) @@ -93,16 +93,16 @@ module.exports = class Branches extends ErrorStash { if (e.status === 404) { Object.assign(params, requiredBranchProtectionDefaults, Overrides.removeOverrides(overrides, branch.protection, {}), { headers: previewHeaders }) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) - return Promise.resolve(resArray) + branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) + return Promise.resolve(branchResults) } this.log.debug(`Adding branch protection ${JSON.stringify(params)}`) return this.github.rest.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 [] }) } else { this.logError(e) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), `${e}`, 'ERROR')) - return Promise.resolve(resArray) + branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), `${e}`, 'ERROR')) + return Promise.resolve(branchResults) } } }) diff --git a/lib/settings.js b/lib/settings.js index fd7ca2693..c5e017ba4 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -196,11 +196,20 @@ class Settings { return } - // remove duplicate rows in this.results - this.results = this.results.filter((thing, index, self) => { - return index === self.findIndex((t) => { - return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin - }) + // Remove exact duplicate rows from this.results. Comparing only on + // type/repo/plugin would also drop legitimately distinct entries, e.g. the + // planned protection changes for two branches of the same repo. + const seenResults = new Set() + this.results = this.results.filter(res => { + if (!res) { + return true + } + const key = JSON.stringify([res.type, res.repo, res.plugin, res.endpoint, res.body, res.action]) + if (seenResults.has(key)) { + return false + } + seenResults.add(key) + return true }) let error = false diff --git a/test/unit/lib/plugins/branches.test.js b/test/unit/lib/plugins/branches.test.js index 49549cc87..c6994a9c7 100644 --- a/test/unit/lib/plugins/branches.test.js +++ b/test/unit/lib/plugins/branches.test.js @@ -363,6 +363,39 @@ describe('Branches', () => { }) }) + describe('in nop mode', () => { + function configureNop (config) { + return new Branches(true, github, { owner: 'bkeepers', repo: 'test' }, config, log, []) + } + + beforeEach(() => { + github.rest.repos.updateBranchProtection.endpoint = jest.fn().mockImplementation(params => { + return { url: 'updateBranchProtection', body: params } + }) + github.rest.repos.deleteBranchProtection.endpoint = jest.fn().mockImplementation(params => { + return { url: 'deleteBranchProtection', body: params } + }) + }) + + describe('when multiple branches have changes', () => { + it('reports each planned change exactly once', () => { + const plugin = configureNop( + [ + { name: 'master', protection: { enforce_admins: true } }, + { name: 'develop', protection: { enforce_admins: true } } + ] + ) + + return plugin.sync().then(res => { + const commands = res.filter(Boolean) + // Each branch produces two NopCommands: the diff record and the + // endpoint command. Two branches must yield four entries, not eight. + expect(commands.length).toBe(4) + }) + }) + }) + }) + describe.skip('return values', () => { it('returns updateBranchProtection Promise', () => { const plugin = configure( diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index e102379a6..b8959cadc 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -549,4 +549,38 @@ repository: expect(mockRepoSync).toHaveBeenCalledTimes(1) }) }) // updateRepos - archived repo skipping + + describe('handleResults', () => { + it('removes exact duplicates but keeps distinct entries for the same repo and plugin', async () => { + stubContext.octokit.rest.checks = { update: jest.fn().mockResolvedValue({}) } + stubContext.octokit.rest.issues = { createComment: jest.fn().mockResolvedValue({}) } + stubContext.payload.check_run = { id: 42, check_suite: { pull_requests: [{ number: 1 }] } } + stubContext.payload.repository = { owner: { login: 'test' }, name: 'test-repo' } + const settings = new Settings(true, stubContext, mockRepo, {}, mockRef) + + const masterDiff = { + type: 'INFO', + plugin: 'Branches', + repo: 'test/test-repo', + endpoint: '', + body: '', + action: { msg: 'Changes for master', additions: {}, modifications: { a: 1 }, deletions: {} } + } + const developDiff = { + type: 'INFO', + plugin: 'Branches', + repo: 'test/test-repo', + endpoint: '', + body: '', + action: { msg: 'Changes for develop', additions: {}, modifications: { b: 2 }, deletions: {} } + } + settings.results = [masterDiff, { ...masterDiff }, developDiff] + + await settings.handleResults() + + // the literal duplicate goes, the distinct same-plugin same-repo entry stays + expect(settings.results).toHaveLength(2) + expect(settings.results.map(res => res.action.msg)).toEqual(['Changes for master', 'Changes for develop']) + }) + }) // handleResults }) // Settings Tests