From 9bcd3f582e44752b38cbc8b54f510188e99dbdf0 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sun, 29 Dec 2024 10:12:01 +0100 Subject: [PATCH] add tests for automatic reviewer assignment Signed-off-by: Hghtwr --- .../gitlabMergeRequest.examples.test.ts | 52 ++++++ .../src/actions/gitlabMergeRequest.test.ts | 156 +++++++++++++++++- .../src/actions/gitlabMergeRequest.ts | 1 - 3 files changed, 204 insertions(+), 5 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts index 087d474807..e6aea81285 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts @@ -42,6 +42,58 @@ const mockGitlabClient = { default_branch: 'main', }; }), + show: jest.fn(async (_: any) => { + return { + default_branch: 'main', + }; + }), + edit: jest.fn(async (_: any) => { + return { + default_branch: 'main', + }; + }), + }, + MergeRequestApprovals: { + allApprovalRules: jest.fn(async (_: any) => { + return [ + { + id: 123, + name: 'rule1', + rule_type: 'regular', + eligible_approvers: [ + { + id: 123, + username: 'John Smith', + }, + { + id: 456, + username: 'Jane Doe', + }, + ], + approvals_required: 1, + users: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + { + id: 456, + name: 'All Members', + rule_type: 'any_approver', + eligible_approvers: [], + approvals_required: 1, + users: [], + groups: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + ]; + }), }, Projects: { create: jest.fn(), diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts index e95856658e..a61a26ebb5 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -51,11 +51,86 @@ const mockGitlabClient = { create: jest.fn(), }, MergeRequests: { - create: jest.fn(async (_: any) => { + create: jest.fn(async (repoId: string) => { + if (repoId === 'owner/repo-without-approvals') { + return { + iid: 5, + }; + } + return { + default_branch: 'main', + iid: 4, + }; + }), + show: jest.fn(async (repoId: string, iid: number) => { + if (repoId === 'owner/repo' && iid === 4) { + return { + iid: 4, + }; + } else if (repoId === 'owner/repo-without-approvals' && iid === 5) { + return { + iid: 5, + }; + } return { default_branch: 'main', }; }), + edit: jest.fn(async (_: any) => { + return { + default_branch: 'main', + }; + }), + }, + MergeRequestApprovals: { + allApprovalRules: jest.fn( + async (repoId: string, options: { mergerequestIId: number }) => { + if ( + repoId === 'owner/repo-without-approvals' && + options.mergerequestIId === 5 + ) { + return []; + } + return [ + { + id: 123, + name: 'rule1', + rule_type: 'regular', + eligible_approvers: [ + { + id: 234, + username: 'Bob Vance', + }, + { + id: 345, + username: 'Dina Fox', + }, + ], + approvals_required: 1, + users: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + { + id: 456, + name: 'All Members', + rule_type: 'any_approver', + eligible_approvers: [], + approvals_required: 1, + users: [], + groups: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + ]; + }, + ), }, Projects: { create: jest.fn(), @@ -74,6 +149,12 @@ const mockGitlabClient = { id: 123, }, ]; + case 'Bob Vance': + return [ + { + id: 234, + }, + ]; case 'Jane Doe': return [ { @@ -571,9 +652,19 @@ describe('createGitLabMergeRequest', () => { assigneeId: 123, }, ); + expect( + mockGitlabClient.MergeRequestApprovals.allApprovalRules, + ).toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.edit).toHaveBeenCalledWith( + 'owner/repo', + 4, + { + reviewerIds: [234, 345], // Approval Rule Members + }, + ); }); - it('reviewer is set correcly when a valid reviewer username is passed in options', async () => { + it('reviewer is set correcly when a valid reviewer username is passed in options in combination with MR approval rules', async () => { const input = { repoUrl: 'gitlab.com?repo=repo&owner=owner', title: 'Create my new MR', @@ -582,7 +673,7 @@ describe('createGitLabMergeRequest', () => { removeSourceBranch: false, targetPath: 'Subdirectory', assignee: 'John Smith', - reviewers: ['Jane Doe'], + reviewers: ['Jane Doe', 'Bob Vance'], }; mockDir.setContent({ [workspacePath]: { @@ -609,9 +700,66 @@ describe('createGitLabMergeRequest', () => { description: 'This is an important change', removeSourceBranch: false, assigneeId: 123, - reviewerIds: [456], + reviewerIds: [456, 234], // Jane Doe and Bob Vance }, ); + expect( + mockGitlabClient.MergeRequestApprovals.allApprovalRules, + ).toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.edit).toHaveBeenCalledWith( + 'owner/repo', + 4, + { + reviewerIds: [234, 345, 456], // Approval Rule Members + Jane Doe (individual reviewer) but no duplicates (Bob Vance) + }, + ); + }); + + it('reviewer is set correcly when a valid reviewer username is passed in options and no MR rules exist', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo-without-approvals&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'This is an important change', + removeSourceBranch: false, + targetPath: 'Subdirectory', + assignee: 'John Smith', + reviewers: ['Jane Doe', 'Bob Vance'], + }; + mockDir.setContent({ + [workspacePath]: { + source: { 'foo.txt': 'Hello there!' }, + irrelevant: { 'bar.txt': 'Nothing to see here' }, + }, + }); + + const ctx = createMockActionContext({ input, workspacePath }); + await instance.handler(ctx); + + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo-without-approvals', + 'new-mr', + 'main', + ); + expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo-without-approvals', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'This is an important change', + removeSourceBranch: false, + assigneeId: 123, + reviewerIds: [456, 234], // Jane Doe and Bob Vance + }, + ); + expect( + mockGitlabClient.MergeRequestApprovals.allApprovalRules, + ).toHaveBeenCalledWith('owner/repo-without-approvals', { + mergerequestIId: 5, + }); + expect(mockGitlabClient.MergeRequests.edit).not.toHaveBeenCalled(); }); it('reviewers are set correcly when valid reviewers username are passed in options', async () => { diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 9493b899b5..607a904d0a 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -424,7 +424,6 @@ which uses additional API calls in order to detect whether to 'create', 'update' }, ); - // This works right away for users/groups set in the rules. if (approvalRules.length !== 0) { const eligibleApprovers = approvalRules .filter(rule => rule.eligible_approvers !== undefined)