Skip to content

Commit 446dfec

Browse files
npalmnpalm
authored
chore(runners): refactor test to avoid random failing tests (#3432)
Tests for scale down are failing random. - Fixed test data to esnue scale down is always slecting the same runner to be terminated. This was causing the random failure. - Cleanup the test file a bit, aka boys scout rule
1 parent 30ea662 commit 446dfec

File tree

3 files changed

+63
-61
lines changed

3 files changed

+63
-61
lines changed

‎.github/workflows/lambda.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,14 @@ jobs:
2727
- name: Run linter
2828
run: yarn lint
2929
- name: Run tests
30+
id: test
3031
run: yarn test
3132
- name: Build distribution
3233
run: yarn build
34+
- name: Upload coverage report
35+
uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v31.2
36+
if: ${{ failure() }}
37+
with:
38+
name: coverage-reports
39+
path: ./**/coverage
40+
retention-days: 5

‎lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts‎

Lines changed: 53 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ let DEFAULT_ORG_RUNNERS_ORPHANED: RunnerInfo[];
8686
// i-running-111 | Repo | running and not exceeding minimumRunningTimeInMinutes
8787
// i-running-112 | Org | busy
8888
// i-running-113 | Repo | busy
89-
const oldest = moment(new Date()).subtract(25, 'minutes').toDate();
89+
const oldest = moment(new Date()).subtract(26, 'minutes').toDate();
90+
const old25minutes = moment(new Date()).subtract(25, 'minutes').toDate();
9091
const DEFAULT_RUNNERS_ORIGINAL = [
9192
{
9293
instanceId: 'i-idle-101',
@@ -118,13 +119,13 @@ const DEFAULT_RUNNERS_ORIGINAL = [
118119
},
119120
{
120121
instanceId: 'i-running-cannot-delete-105',
121-
launchTime: moment(new Date()).subtract(25, 'minutes').toDate(),
122+
launchTime: old25minutes,
122123
type: 'Repo',
123124
owner: `doe/another-repo`,
124125
},
125126
{
126127
instanceId: 'i-running-cannot-delete-106',
127-
launchTime: moment(new Date()).subtract(25, 'minutes').toDate(),
128+
launchTime: old25minutes,
128129
type: 'Org',
129130
owner: TEST_DATA.repositoryOwner,
130131
},
@@ -167,13 +168,13 @@ const DEFAULT_RUNNERS_ORIGINAL = [
167168
},
168169
{
169170
instanceId: 'i-running-busy-112',
170-
launchTime: moment(new Date()).subtract(25, 'minutes').toDate(),
171+
launchTime: old25minutes,
171172
type: 'Repo',
172173
owner: `doe/another-repo`,
173174
},
174175
{
175176
instanceId: 'i-running-busy-113',
176-
launchTime: moment(new Date()).subtract(25, 'minutes').toDate(),
177+
launchTime: old25minutes,
177178
type: 'Org',
178179
owner: TEST_DATA.repositoryOwner,
179180
},
@@ -214,7 +215,7 @@ const DEFAULT_REGISTERED_RUNNERS = [
214215
},
215216
];
216217

217-
describe('scaleDown', () => {
218+
describe('Scale down runners', () => {
218219
beforeEach(() => {
219220
process.env = { ...cleanEnv };
220221
process.env.GITHUB_APP_KEY_BASE64 = 'TEST_CERTIFICATE_DATA';
@@ -339,24 +340,20 @@ describe('scaleDown', () => {
339340
);
340341
});
341342

342-
describe('github.com', () => {
343-
describe('no runners running', () => {
344-
beforeEach(() => {
345-
mockListRunners.mockResolvedValue([]);
346-
});
343+
describe('for github.com', () => {
344+
it('Should not call terminate when no runners online.', async () => {
345+
mockListRunners.mockResolvedValue([]);
347346

348-
it('No runners online', async () => {
349-
await scaleDown();
350-
expect(listEC2Runners).toBeCalledWith({
351-
environment: environment,
352-
});
353-
expect(terminateRunner).not;
354-
expect(mockOctokit.apps.getRepoInstallation).not;
355-
expect(mockOctokit.apps.getRepoInstallation).not;
347+
await scaleDown();
348+
expect(listEC2Runners).toBeCalledWith({
349+
environment: environment,
356350
});
351+
expect(terminateRunner).not;
352+
expect(mockOctokit.apps.getRepoInstallation).not;
353+
expect(mockOctokit.apps.getRepoInstallation).not;
357354
});
358355

359-
it('Terminates 3 of 5 runners owned by repos and one orphaned', async () => {
356+
it('Should terminates 3 of 5 runners owned by repos and one orphaned', async () => {
360357
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS_REPO);
361358
await scaleDown();
362359
expect(listEC2Runners).toBeCalledWith({
@@ -374,7 +371,7 @@ describe('scaleDown', () => {
374371
}
375372
});
376373

377-
it('Terminates 2 of 3 runners owned by orgs and one orphaned', async () => {
374+
it('Should terminates 2 of 3 runners owned by orgs and one orphaned', async () => {
378375
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS_ORG);
379376
await scaleDown();
380377
expect(listEC2Runners).toBeCalledWith({
@@ -391,35 +388,38 @@ describe('scaleDown', () => {
391388
}
392389
});
393390

394-
describe('With idle config', () => {
391+
describe('When idle config defined', () => {
395392
const defaultConfig = {
396393
idleCount: 3,
397394
cron: '* * * * * *',
398395
timeZone: 'Europe/Amsterdam',
399396
evictionStrategy: 'oldest_first',
400397
};
398+
401399
beforeEach(() => {
402400
process.env.SCALE_DOWN_CONFIG = JSON.stringify([defaultConfig]);
403401
});
404402

405-
it('Terminates 1 runner owned by orgs', async () => {
403+
it('Should terminate based on the the idle config', async () => {
406404
mockListRunners.mockResolvedValue(RUNNERS_ORG_WITH_AUTO_SCALING_CONFIG);
407405
await scaleDown();
408406

409-
expect(listEC2Runners).toBeCalledWith({
410-
environment: environment,
411-
});
412-
413-
expect(mockOctokit.apps.getOrgInstallation).toBeCalled();
414407
expect(terminateRunner).toBeCalledTimes(1);
415408
for (const toTerminate of RUNNERS_ORG_TO_BE_REMOVED_WITH_AUTO_SCALING_CONFIG) {
416409
expect(terminateRunner).toHaveBeenCalledWith(toTerminate.instanceId);
417410
}
411+
412+
process.env.SCALE_DOWN_CONFIG = JSON.stringify([]);
413+
414+
// run test again with out idle config
415+
jest.clearAllMocks();
416+
mockListRunners.mockResolvedValue(RUNNERS_ORG_WITH_AUTO_SCALING_CONFIG);
417+
await scaleDown();
418+
expect(terminateRunner).toBeCalledTimes(2);
418419
});
419420

420-
it('Terminates 0 runners owned by org', async () => {
421+
it('Should terminates 0 runners owned by org', async () => {
421422
mockListRunners.mockResolvedValue(RUNNERS_REPO_WITH_AUTO_SCALING_CONFIG);
422-
process.env.ENABLE_ORGANIZATION_RUNNERS = 'false';
423423
await scaleDown();
424424

425425
expect(listEC2Runners).toBeCalledWith({
@@ -430,21 +430,17 @@ describe('scaleDown', () => {
430430
expect(terminateRunner).not.toBeCalled();
431431
});
432432

433-
describe('With newest_first eviction strategy', () => {
434-
beforeEach(() => {
435-
process.env.SCALE_DOWN_CONFIG = JSON.stringify([{ ...defaultConfig, evictionStrategy: 'newest_first' }]);
436-
});
433+
it('Should terminates the newest runner.', async () => {
434+
process.env.SCALE_DOWN_CONFIG = JSON.stringify([{ ...defaultConfig, evictionStrategy: 'newest_first' }]);
437435

438-
it('Terminates the newest org', async () => {
439-
mockListRunners.mockResolvedValue(RUNNERS_ORG_WITH_AUTO_SCALING_CONFIG);
440-
await scaleDown();
441-
expect(terminateRunner).toBeCalledTimes(1);
442-
expect(terminateRunner).toHaveBeenCalledWith('i-idle-102');
443-
});
436+
mockListRunners.mockResolvedValue(RUNNERS_ORG_WITH_AUTO_SCALING_CONFIG);
437+
await scaleDown();
438+
expect(terminateRunner).toBeCalledTimes(1);
439+
expect(terminateRunner).toHaveBeenCalledWith('i-idle-102');
444440
});
445441
});
446442

447-
it('No instances terminates when delete runner in github results in a non 204 status.', async () => {
443+
it('Should terminate no instances when delete runner in github results in a non 204 status.', async () => {
448444
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS);
449445
mockOctokit.actions.deleteSelfHostedRunnerFromOrg.mockImplementation(() => {
450446
return { status: 500 };
@@ -460,7 +456,7 @@ describe('scaleDown', () => {
460456
expect(terminateRunner).not.toBeCalled;
461457
});
462458

463-
it('Terminates 4 runners amongst all owners and two orphaned', async () => {
459+
it('Should terminates 4 runners amongst all owners and two orphaned', async () => {
464460
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS);
465461
await scaleDown();
466462

@@ -480,27 +476,23 @@ describe('scaleDown', () => {
480476
});
481477
});
482478

483-
describe('ghes', () => {
479+
describe('for GHES (GitHub Enterprise)', () => {
484480
beforeEach(() => {
485481
process.env.GHES_URL = 'https://github.enterprise.something';
486482
});
487-
describe('no runners running', () => {
488-
beforeEach(() => {
489-
mockListRunners.mockResolvedValue([]);
490-
});
491483

492-
it('No runners online', async () => {
493-
await scaleDown();
494-
expect(listEC2Runners).toBeCalledWith({
495-
environment: environment,
496-
});
497-
expect(terminateRunner).not;
498-
expect(mockOctokit.apps.getRepoInstallation).not;
499-
expect(mockOctokit.apps.getRepoInstallation).not;
484+
it('Should not call terminate when no runners online', async () => {
485+
mockListRunners.mockResolvedValue([]);
486+
await scaleDown();
487+
expect(listEC2Runners).toBeCalledWith({
488+
environment: environment,
500489
});
490+
expect(terminateRunner).not;
491+
expect(mockOctokit.apps.getRepoInstallation).not;
492+
expect(mockOctokit.apps.getRepoInstallation).not;
501493
});
502494

503-
it('Terminates 3 of 5 runners owned by repos and one orphaned', async () => {
495+
it('Should terminates 3 of 5 runners owned by repos and one orphaned', async () => {
504496
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS_REPO);
505497
await scaleDown();
506498
expect(listEC2Runners).toBeCalledWith({
@@ -517,7 +509,7 @@ describe('scaleDown', () => {
517509
}
518510
});
519511

520-
it('Terminates 2 of 3 runners owned by orgs and one orphaned', async () => {
512+
it('Should terminates 2 of 3 runners owned by orgs and one orphaned', async () => {
521513
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS_ORG);
522514
await scaleDown();
523515
expect(listEC2Runners).toBeCalledWith({
@@ -534,7 +526,7 @@ describe('scaleDown', () => {
534526
}
535527
});
536528

537-
describe('With idle config', () => {
529+
describe('When idle config defined', () => {
538530
beforeEach(() => {
539531
process.env.SCALE_DOWN_CONFIG = JSON.stringify([
540532
{
@@ -546,7 +538,7 @@ describe('scaleDown', () => {
546538
]);
547539
});
548540

549-
it('Terminates 1 runner owned by orgs', async () => {
541+
it('Should terminates 1 runner owned by orgs', async () => {
550542
mockListRunners.mockResolvedValue(RUNNERS_ORG_WITH_AUTO_SCALING_CONFIG);
551543
await scaleDown();
552544

@@ -561,7 +553,7 @@ describe('scaleDown', () => {
561553
}
562554
});
563555

564-
it('Terminates 0 runners owned by repos', async () => {
556+
it('Should terminates 0 runners owned by repos', async () => {
565557
mockListRunners.mockResolvedValue(RUNNERS_REPO_WITH_AUTO_SCALING_CONFIG);
566558
process.env.ENABLE_ORGANIZATION_RUNNERS = 'false';
567559
await scaleDown();
@@ -575,7 +567,7 @@ describe('scaleDown', () => {
575567
});
576568
});
577569

578-
it('Terminates 4 runners amongst all owners and two orphaned', async () => {
570+
it('Should terminates 4 runners amongst all owners and two orphaned', async () => {
579571
mockListRunners.mockResolvedValue(DEFAULT_RUNNERS);
580572
await scaleDown();
581573

‎lambdas/jest.base.config.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ const defaultConfig: Config = {
55
testEnvironment: 'node',
66
collectCoverage: true,
77
collectCoverageFrom: ['src/**/*.{ts,js,jsx}', '!src/**/*local*.ts', '!src/**/*.d.ts'],
8+
coverageReporters: ['text', 'lcov', 'html'],
9+
verbose: true
810
};
911

1012
export default defaultConfig;

0 commit comments

Comments
 (0)