Skip to content

Commit

Permalink
fix(gerrit): not auto-approving if change already had a Code-Review +1
Browse files Browse the repository at this point in the history
  • Loading branch information
@felipecrs
felipecrs committed Nov 29, 2024
1 parent 5459f59 commit 5b42180
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 10 deletions.
47 changes: 43 additions & 4 deletions lib/modules/platform/gerrit/client.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,10 +393,20 @@ describe('modules/platform/gerrit/client', () => {

describe('approveChange()', () => {
it('already approved - do nothing', async () => {
const change = partial<GerritChange>({});
const change = partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 2 }],
},
},
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
});
Expand All @@ -405,14 +415,43 @@ describe('modules/platform/gerrit/client', () => {
const change = partial<GerritChange>({ labels: {} });
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);

await expect(client.approveChange(123456)).toResolve();
});

it('not already approved - approve now', async () => {

Check failure on line 428 in lib/modules/platform/gerrit/client.spec.ts

View workflow job for this annotation

GitHub Actions / test (7/16)

modules/platform/gerrit/client › approveChange() › not already approved - approve now 

*** Missing HTTP mocks ***

- POST https://dev.gerrit.com/renovate/a/changes/123456/revisions/current/review

Requests done:

- GET https://dev.gerrit.com/renovate/a/changes/123456?o=SUBMITTABLE&o=CHECK&o=MESSAGES&o=DETAILED_ACCOUNTS&o=LABELS&o=CURRENT_ACTIONS&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=DETAILED_LABELS [200]

---

Renovate testing strategy requires that every HTTP request
has a corresponding mock.

This error occurs when some of the request aren't mocked.

Let's suppose your code performs two HTTP calls:

  GET   https://example.com/foo/bar/fail     404  <without body>
  POST  https://example.com/foo/bar/success  200  { "ok": true }

The unit test should have this mock:

  httpMock.scope('https://example.com/foo/bar')
    .get('/fail')
    .reply(404)
    .post('/success')
    .reply(200, { ok: true });

Note: `httpMock.scope(...)` is the Renovate-specific construct.
      The scope object itself is provided by the `nock` library.

Details: https://github.com/nock/nock#usage


    at <test> (lib/modules/platform/gerrit/client.spec.ts:428:9)
const change = partial<GerritChange>({ labels: { 'Code-Review': {} } });
const change = partial<GerritChange>({
labels: { 'Code-Review': { all: [] } },
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.reply(200, gerritRestResponse(change), jsonResultHeader);
const approveMock = httpMock
.scope(gerritEndpointUrl)
.post('/a/changes/123456/revisions/current/review', {
labels: { 'Code-Review': +2 },
notify: 'NONE',
})
.reply(200, gerritRestResponse(''), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();

Check failure on line 443 in lib/modules/platform/gerrit/client.spec.ts

View workflow job for this annotation

GitHub Actions / test (7/16)

modules/platform/gerrit/client › approveChange() › not already approved - approve now 

expect(received).toResolve()

Expected promise to resolve, however it rejected.


    at Object.<anonymous> (lib/modules/platform/gerrit/client.spec.ts:443:50)
expect(approveMock.isDone()).toBeTrue();
});

it('not already approved because of +1 - approve now', async () => {
const change = partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 1 }],
},
},
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
Expand Down
18 changes: 12 additions & 6 deletions lib/modules/platform/gerrit/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,13 @@ class GerritClient {
return changes.body;
}

async getChange(changeNumber: number): Promise<GerritChange> {
async getChange(
changeNumber: number,
extraOptions?: string[],
): Promise<GerritChange> {
const options = [...this.requestDetails, ...(extraOptions ?? [])];
const changes = await this.gerritHttp.getJson<GerritChange>(
`a/changes/${changeNumber}?` +
this.requestDetails.map((det) => `o=${det}`).join('&'),
`a/changes/${changeNumber}?` + options.map((det) => `o=${det}`).join('&'),
);
return changes.body;
}
Expand Down Expand Up @@ -189,9 +192,12 @@ class GerritClient {
}

async checkIfApproved(changeId: number): Promise<boolean> {
const change = await client.getChange(changeId);
const reviewLabels = change?.labels?.['Code-Review'];
return reviewLabels === undefined || reviewLabels.approved !== undefined;
const change = await client.getChange(changeId, ['DETAILED_LABELS']);
const reviewLabel = change?.labels?.['Code-Review'];
return (
reviewLabel === undefined ||
Boolean(reviewLabel.all?.some((label) => label.value === 2))
);
}

wasApprovedBy(change: GerritChange, username: string): boolean | undefined {
Expand Down
6 changes: 6 additions & 0 deletions lib/modules/platform/gerrit/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ export interface GerritChangeMessageInfo {
export interface GerritLabelInfo {
approved?: GerritAccountInfo;
rejected?: GerritAccountInfo;
/** List of votes. Only set when o=DETAILED_LABELS. */
all?: GerritApprovalInfo[];
}

export interface GerritActionInfo {
Expand All @@ -99,3 +101,7 @@ export interface GerritMergeableInfo {
| 'CHERRY_PICK';
mergeable: boolean;
}

export interface GerritApprovalInfo {
value?: number;
}

0 comments on commit 5b42180

Please sign in to comment.