Skip to content

Commit

Permalink
feat(cache): pr fingerprint implementation (#18850)
Browse files Browse the repository at this point in the history 
Co-authored-by: Rhys Arkins <rhys@arkins.net>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
  • Loading branch information
@RahulGautamSingh @rarkins @viceice
3 people authored Feb 21, 2023
1 parent 27c46cc commit 63fde6b
Show file tree
Hide file tree
Showing 9 changed files with 403 additions and 8 deletions.
16 changes: 14 additions & 2 deletions lib/util/cache/repository/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,17 @@ export interface BranchUpgradeCache {
sourceUrl?: string;
}

export interface PrCache {
fingerprint: string;
/**
* last PR modified ISO timestamp
*/
lastEdited: string;
}

export interface BranchCache {
/**
*Whether this branch has automerge enabled
* Whether this branch has automerge enabled
*/
automerge: boolean;
/**
Expand All @@ -40,7 +48,7 @@ export interface BranchCache {
*/
baseBranchSha: string | null;
/**
* Hash of the manager fingerprints and the update branch config
* Hash of the manager fingerprints and the filtered update branch config
*/
branchFingerprint?: string;
/**
Expand Down Expand Up @@ -75,6 +83,10 @@ export interface BranchCache {
* Details on the dependency upgrades that have been applied in this branch
*/
upgrades: BranchUpgradeCache[];
/**
* Object that has PR info
*/
prCache?: PrCache | null;
}

export interface RepoCacheData {
Expand Down
2 changes: 1 addition & 1 deletion lib/util/date.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('util/date', () => {
expect(getElapsedHours(Jan1)).toBe(elapsedHours); // JS Date
});

it('throws when invalid date is passed', () => {
it('returns zero when date passed is invalid', () => {
expect(getElapsedHours(new Date('invalid_date_string'))).toBe(0);
});
});
Expand Down
6 changes: 3 additions & 3 deletions lib/util/date.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ export function getElapsedMinutes(date: Date): number {
}

export function getElapsedHours(date: Date | string): number {
const lastDate =
const pastDate =
typeof date === 'string'
? DateTime.fromISO(date)
: DateTime.fromJSDate(date);

if (!lastDate.isValid) {
if (!pastDate.isValid) {
return 0;
}

const diff = DateTime.now().diff(lastDate, 'hours');
const diff = DateTime.now().diff(pastDate, 'hours');
return Math.floor(diff.hours);
}
3 changes: 3 additions & 0 deletions lib/workers/repository/cache.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from '../../util/git';
import { getCachedPristineResult } from '../../util/git/pristine';
import type { BranchConfig, BranchUpgradeConfig } from '../types';
import { getPrCache } from './update/pr/pr-cache';

function generateBranchUpgradeCache(
upgrade: BranchUpgradeConfig
Expand Down Expand Up @@ -73,6 +74,7 @@ async function generateBranchCache(
? branch.upgrades.map(generateBranchUpgradeCache)
: [];
const branchFingerprint = branch.branchFingerprint;
const prCache = getPrCache(branchName);
return {
automerge,
baseBranchSha,
Expand All @@ -83,6 +85,7 @@ async function generateBranchCache(
isConflicted,
isModified,
pristine,
prCache,
prNo,
sha,
upgrades,
Expand Down
140 changes: 140 additions & 0 deletions lib/workers/repository/update/pr/index.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,17 @@ import * as _comment from '../../../../modules/platform/comment';
import { getPrBodyStruct } from '../../../../modules/platform/pr-body';
import type { Pr } from '../../../../modules/platform/types';
import { ExternalHostError } from '../../../../types/errors/external-host-error';
import type { PrCache } from '../../../../util/cache/repository/types';
import { fingerprint } from '../../../../util/fingerprint';
import * as _limits from '../../../global/limits';
import type { BranchConfig, BranchUpgradeConfig } from '../../../types';
import { embedChangelog } from '../../changelog';
import * as _statusChecks from '../branch/status-checks';
import * as _prBody from './body';
import type { ChangeLogChange, ChangeLogRelease } from './changelog/types';
import * as _participants from './participants';
import * as _prCache from './pr-cache';
import { generatePrFingerprintConfig } from './pr-fingerprint';
import { ensurePr } from '.';

jest.mock('../../../../util/git');
Expand All @@ -36,6 +41,9 @@ const participants = mocked(_participants);
jest.mock('../../../../modules/platform/comment');
const comment = mocked(_comment);

jest.mock('./pr-cache');
const prCache = mocked(_prCache);

describe('workers/repository/update/pr/index', () => {
describe('ensurePr', () => {
const number = 123;
Expand Down Expand Up @@ -79,6 +87,7 @@ describe('workers/repository/update/pr/index', () => {
{ pr: pr.number, prTitle },
'PR created'
);
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('aborts PR creation once limit is exceeded', async () => {
Expand All @@ -91,6 +100,7 @@ describe('workers/repository/update/pr/index', () => {

expect(res).toEqual({ type: 'without-pr', prBlockedBy: 'RateLimited' });
expect(platform.createPr).not.toHaveBeenCalled();
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('ignores PR limits on vulnerability alert', async () => {
Expand All @@ -101,6 +111,7 @@ describe('workers/repository/update/pr/index', () => {

expect(res).toEqual({ type: 'with-pr', pr });
expect(platform.createPr).toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('creates rollback PR', async () => {
Expand All @@ -110,6 +121,7 @@ describe('workers/repository/update/pr/index', () => {

expect(res).toEqual({ type: 'with-pr', pr });
expect(logger.logger.info).toHaveBeenCalledWith('Creating Rollback PR');
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('skips PR creation due to non-green branch check', async () => {
Expand All @@ -121,6 +133,7 @@ describe('workers/repository/update/pr/index', () => {
type: 'without-pr',
prBlockedBy: 'AwaitingTests',
});
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('creates PR for green branch checks', async () => {
Expand All @@ -131,6 +144,7 @@ describe('workers/repository/update/pr/index', () => {

expect(res).toEqual({ type: 'with-pr', pr });
expect(platform.createPr).toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('skips PR creation for unapproved dependencies', async () => {
Expand All @@ -142,6 +156,7 @@ describe('workers/repository/update/pr/index', () => {
type: 'without-pr',
prBlockedBy: 'NeedsApproval',
});
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('skips PR creation before prNotPendingHours is hit', async () => {
Expand All @@ -161,6 +176,7 @@ describe('workers/repository/update/pr/index', () => {
type: 'without-pr',
prBlockedBy: 'AwaitingTests',
});
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('skips PR creation due to stabilityStatus', async () => {
Expand All @@ -180,6 +196,7 @@ describe('workers/repository/update/pr/index', () => {
type: 'without-pr',
prBlockedBy: 'AwaitingTests',
});
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('creates PR after prNotPendingHours is hit', async () => {
Expand All @@ -197,6 +214,7 @@ describe('workers/repository/update/pr/index', () => {
});

expect(res).toEqual({ type: 'with-pr', pr });
expect(prCache.setPrCache).toHaveBeenCalled();
});

describe('Error handling', () => {
Expand All @@ -207,6 +225,7 @@ describe('workers/repository/update/pr/index', () => {
const res = await ensurePr(config);

expect(res).toEqual({ type: 'without-pr', prBlockedBy: 'Error' });
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('handles error for PR that already exists', async () => {
Expand All @@ -223,6 +242,7 @@ describe('workers/repository/update/pr/index', () => {
expect(logger.logger.warn).toHaveBeenCalledWith(
'A pull requests already exists'
);
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('deletes branch on 502 error', async () => {
Expand All @@ -233,6 +253,7 @@ describe('workers/repository/update/pr/index', () => {
const res = await ensurePr(config);

expect(res).toEqual({ type: 'without-pr', prBlockedBy: 'Error' });
expect(prCache.setPrCache).not.toHaveBeenCalled();
expect(git.deleteBranch).toHaveBeenCalledWith('renovate-branch');
});
});
Expand All @@ -252,6 +273,7 @@ describe('workers/repository/update/pr/index', () => {
{ pr: changedPr.number, prTitle },
`PR updated`
);
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('updates PR due to body change', async () => {
Expand All @@ -266,6 +288,11 @@ describe('workers/repository/update/pr/index', () => {
expect(res).toEqual({ type: 'with-pr', pr: changedPr });
expect(platform.updatePr).toHaveBeenCalled();
expect(platform.createPr).not.toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
expect(logger.logger.info).toHaveBeenCalledWith(
{ pr: changedPr.number, prTitle },
`PR updated`
);
});

it('ignores reviewable content ', async () => {
Expand All @@ -284,6 +311,10 @@ describe('workers/repository/update/pr/index', () => {
expect(res).toEqual({ type: 'with-pr', pr: changedPr });
expect(platform.updatePr).not.toHaveBeenCalled();
expect(platform.createPr).not.toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
expect(logger.logger.debug).toHaveBeenCalledWith(
'Pull Request #123 does not need updating'
);
});
});

Expand Down Expand Up @@ -353,6 +384,7 @@ describe('workers/repository/update/pr/index', () => {
});
expect(platform.updatePr).not.toHaveBeenCalled();
expect(platform.createPr).not.toHaveBeenCalled();
expect(prCache.setPrCache).not.toHaveBeenCalled();
});

it('adds assignees for PR automerge with red status', async () => {
Expand All @@ -372,6 +404,7 @@ describe('workers/repository/update/pr/index', () => {

expect(res).toEqual({ type: 'with-pr', pr: changedPr });
expect(participants.addParticipants).toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('adds reviewers for PR automerge with red status and existing ignorable reviewers that can be ignored', async () => {
Expand Down Expand Up @@ -408,6 +441,7 @@ describe('workers/repository/update/pr/index', () => {
expect(res).toEqual({ type: 'with-pr', pr });
expect(platform.createPr).toHaveBeenCalled();
expect(participants.addParticipants).not.toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('skips branch automerge and forces PR creation due to prNotPendingHours exceeded', async () => {
Expand All @@ -428,6 +462,7 @@ describe('workers/repository/update/pr/index', () => {

expect(res).toEqual({ type: 'with-pr', pr });
expect(platform.createPr).toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
});

it('automerges branch when prNotPendingHours are not exceeded', async () => {
Expand Down Expand Up @@ -711,5 +746,110 @@ describe('workers/repository/update/pr/index', () => {
});
});
});

describe('prCache', () => {
const existingPr: Pr = {
...pr,
};
let cachedPr: PrCache | null = null;

it('adds pr-cache when not present', async () => {
platform.getBranchPr.mockResolvedValue(existingPr);
cachedPr = null;
prCache.getPrCache.mockReturnValueOnce(cachedPr);
const res = await ensurePr(config);
expect(res).toEqual({
type: 'with-pr',
pr: existingPr,
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'Pull Request #123 does not need updating'
);
expect(prCache.setPrCache).toHaveBeenCalledTimes(1);
});

it('does not update lastEdited pr-cache when pr fingerprint is same but pr was edited within 24hrs', async () => {
platform.getBranchPr.mockResolvedValue(existingPr);
cachedPr = {
fingerprint: fingerprint(generatePrFingerprintConfig(config)),
lastEdited: new Date().toISOString(),
};
prCache.getPrCache.mockReturnValueOnce(cachedPr);
const res = await ensurePr(config);
expect(res).toEqual({
type: 'with-pr',
pr: existingPr,
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'Pull Request #123 does not need updating'
);
expect(logger.logger.debug).toHaveBeenCalledWith(
'PR cache matches but it has been edited in the past 24hrs, so processing PR'
);
expect(prCache.setPrCache).toHaveBeenCalledWith(
sourceBranch,
cachedPr.fingerprint,
false
);
});

it('updates pr-cache when pr fingerprint is different', async () => {
platform.getBranchPr.mockResolvedValue({
...existingPr,
title: 'Another title',
});
cachedPr = {
fingerprint: 'old',
lastEdited: new Date('2020-01-20T00:00:00Z').toISOString(),
};
prCache.getPrCache.mockReturnValueOnce(cachedPr);
const res = await ensurePr(config);
expect(res).toEqual({
type: 'with-pr',
pr: { ...existingPr, title: 'Another title' },
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'PR fingerprints mismatch, processing PR'
);
expect(prCache.setPrCache).toHaveBeenCalledTimes(1);
});

it('skips fetching changelogs when cache is valid and pr was lastEdited before 24hrs', async () => {
config.repositoryCache = 'enabled';
platform.getBranchPr.mockResolvedValue(existingPr);
cachedPr = {
fingerprint: fingerprint(generatePrFingerprintConfig(config)),
lastEdited: new Date('2020-01-20T00:00:00Z').toISOString(),
};
prCache.getPrCache.mockReturnValueOnce(cachedPr);
const res = await ensurePr(config);
expect(res).toEqual({
type: 'with-pr',
pr: existingPr,
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'PR cache matches and no PR changes in last 24hrs, so skipping PR body check'
);
expect(embedChangelog).toHaveBeenCalledTimes(0);
});

it('logs when cache is enabled but pr-cache is absent', async () => {
config.repositoryCache = 'enabled';
platform.getBranchPr.mockResolvedValue(existingPr);
prCache.getPrCache.mockReturnValueOnce(null);
await ensurePr(config);
expect(logger.logger.debug).toHaveBeenCalledWith('PR cache not found');
});

it('does not log when cache is disabled and pr-cache is absent', async () => {
config.repositoryCache = 'disabled';
platform.getBranchPr.mockResolvedValue(existingPr);
prCache.getPrCache.mockReturnValueOnce(null);
await ensurePr(config);
expect(logger.logger.debug).not.toHaveBeenCalledWith(
'PR cache not found'
);
});
});
});
});
Loading

0 comments on commit 63fde6b

Please sign in to comment.