Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add validity check to Electron Archaeologist runs #42

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
name: Test

on:
push:
branches:
- main
pull_request:
branches:
- main

permissions:
contents: read

jobs:
build:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Setup Node.js
uses: actions/setup-node@b39b52d1213e96004bfcb1c61a8a6fa8ab84f3e8 # v4.0.1
with:
node-version: lts/-1
- name: Install
run: yarn install
- name: Test
run: yarn test

20 changes: 20 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
process.env.SPEC_RUNNING = '1';

module.exports = {
roots: [
'<rootDir>/spec'
],
transform: {
'^.+\\.tsx?$': 'ts-jest'
},
clearMocks: true,
testRegex: '(/spec/.*|(\\.|/)(test|spec))\\.tsx?$',
moduleFileExtensions: [
'ts',
'tsx',
'js',
'jsx',
'json',
'node'
],
}
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,20 @@
},
"devDependencies": {
"@types/fs-extra": "^9.0.13",
"@types/jest": "^29.5.12",
"@types/node": "^18.0.41",
"@types/shortid": "^0.0.29",
"jest": "^29.7.0",
"prettier": "^3.0.0",
"ts-jest": "^29.1.2",
"typescript": "^4.8.3"
},
"scripts": {
"build": "tsc",
"start": "probot run ./lib/index.js",
"lint": "prettier --check \"src/**/*.ts\"",
"format": "prettier --write \"src/**/*.ts\"",
"postinstall": "tsc"
"postinstall": "tsc",
"test": "jest --testPathIgnorePatterns=/working/ --testPathIgnorePatterns=/node_modules/"
}
}
39 changes: 39 additions & 0 deletions spec/fixtures/pull_request.opened.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"name": "pull_request",
"payload": {
"action": "opened",
"number": 7,
"pull_request": {
"url": "my_cool_url",
"title": "CHANGE README",
"merged": true,
"user": {
"login": "codebytere"
},
"body": "New cool stuff \nNotes: new cool stuff added",
"labels": [],
"head": {
"sha": "ABC"
},
"base": {
"ref": "main",
"repo": {
"default_branch": "main"
}
}
},
"label": {
"name": "todo",
"color": "8cb728"
},
"repository": {
"name": "probot-test",
"owner": {
"login": "codebytere"
}
},
"installation": {
"id": 103619
}
}
}
57 changes: 57 additions & 0 deletions spec/utils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { SEMVER_LABELS } from '../src/constants';
import {getCheckStatusItems, checkStatuses} from '../src/utils/check-utils'
const PROpenedEvent = require('./fixtures/pull_request.opened.json');

const semverNoneLabel = {
name: SEMVER_LABELS.NONE,
color: '000'
};
const semverMinorLabel = {
name: SEMVER_LABELS.MINOR,
color: '000'
};
const semverMajorLabel = {
name: SEMVER_LABELS.MAJOR,
color: '000'
};

describe('utils', () => {
describe('getCheckStatusItems()', () => {

const context = {
octokit: {},
repo: jest.fn(),
...PROpenedEvent,
};

it('should return the valid changes status when changes are detected with a semver/minor label', () => {
context.payload.pull_request.labels = [semverMinorLabel]
const result = getCheckStatusItems({context, hasChanges: true});
expect(result).toEqual(checkStatuses.validChanges);
});

it('should return the invalid changes status when changes are detected with a semver/none label', () => {
context.payload.pull_request.labels = [semverNoneLabel]
const result = getCheckStatusItems({context, hasChanges: true});
expect(result).toEqual(checkStatuses.invalidChanges);
});

it('should return the invalid changes status when changes are detected with no semver labels', () => {
context.payload.pull_request.labels = []
const result = getCheckStatusItems({context, hasChanges: true});
expect(result).toEqual(checkStatuses.invalidChanges);
});

it('should return the valid status when no changes are detected with a semver/none label', () => {
context.payload.pull_request.labels = [semverNoneLabel]
const result = getCheckStatusItems({context, hasChanges: false});
expect(result).toEqual(checkStatuses.validNoChanges);
});

it('should return the invalid status when no changes are detected with a semver/major label', () => {
context.payload.pull_request.labels = [semverMajorLabel]
const result = getCheckStatusItems({context, hasChanges: false});
expect(result).toEqual(checkStatuses.invalidNoChanges);
});
})
})
8 changes: 8 additions & 0 deletions src/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export const SEMVER_PREFIX = 'semver/';

export const SEMVER_LABELS = {
NONE: 'semver/none',
PATCH: 'semver/patch',
MINOR: 'semver/minor',
MAJOR: 'semver/major',
};
18 changes: 11 additions & 7 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Logger } from './logger';
import { getCircleArtifacts } from './circleci/artifacts';
import { REPO_SLUG } from './circleci/constants';
import { withTempDir } from './tmp';
import { getCheckStatusItems } from './utils/check-utils';

const stripVersion = (dts: string) => dts.replace(/Type definitions for Electron .+?\n/g, '');

Expand Down Expand Up @@ -97,19 +98,21 @@ async function runCheckOn(
circleArtifacts.old = stripVersion(circleArtifacts.old);

if (circleArtifacts.new === circleArtifacts.old) {
const { title, summary, conclusion } = getCheckStatusItems({ context, hasChanges: false });
await context.octokit.checks.update(
context.repo({
check_run_id: `${check.data.id}`,
conclusion: 'success' as 'success',
conclusion,
started_at: started_at.toISOString(),
completed_at: new Date().toISOString(),
output: {
title: 'No Changes',
summary: "We couldn't see any changes in the `electron.d.ts` artifact",
title,
summary,
},
}),
);
} else {
// Check label if semvor/none, conclusion error
checkContext.logger.info('creating patch');
const patch = await withTempDir(async (dir) => {
const newPath = path.resolve(dir, 'electron.new.d.ts');
Expand All @@ -123,17 +126,18 @@ async function runCheckOn(
});
checkContext.logger.info('patch created with length:', `${patch.length}`);

const fullSummary = `Looks like the \`electron.d.ts\` file changed.\n\n\`\`\`\`\`\`diff\n${patch}\n\`\`\`\`\`\``;
const tooBigSummary = `Looks like the \`electron.d.ts\` file changed, but the diff is too large to display here. See artifacts on the CircleCI build.`;
const { title, summary, conclusion } = getCheckStatusItems({ context, hasChanges: true });
const fullSummary = `${summary}Looks like the \`electron.d.ts\` file changed.\n\n\`\`\`\`\`\`diff\n${patch}\n\`\`\`\`\`\``;
const tooBigSummary = `${summary}Looks like the \`electron.d.ts\` file changed, but the diff is too large to display here. See artifacts on the CircleCI build.`;

await context.octokit.checks.update(
context.repo({
check_run_id: `${check.data.id}`,
conclusion: 'neutral' as 'neutral',
conclusion,
started_at: started_at.toISOString(),
completed_at: new Date().toISOString(),
output: {
title: 'Changes Detected',
title,
summary: fullSummary.length > 65535 ? tooBigSummary : fullSummary,
},
}),
Expand Down
13 changes: 13 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,20 @@ export type PRContext = Context<
'pull_request.opened' | 'pull_request.reopened' | 'pull_request.synchronize'
>;

export type PR = PRContext['payload']['pull_request'];
export interface IContext {
logger: ILogger;
bot: PRContext;
}

export enum CheckRunStatus {
NEUTRAL = 'neutral',
FAILURE = 'failure',
SUCCESS = 'success',
}

export type CheckStatus = {
conclusion: CheckRunStatus;
title: string;
summary: string;
};
62 changes: 62 additions & 0 deletions src/utils/check-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { SEMVER_LABELS } from '../constants';
import { CheckRunStatus, CheckStatus, PRContext } from '../types';
import { getSemverLabel } from './label-utils';

// Define the possible check statuses
export const checkStatuses = {
validNoChanges: {
conclusion: CheckRunStatus.SUCCESS,
title: 'No Changes',
summary: "We couldn't see any changes in the `electron.d.ts` artifact",
},
invalidNoChanges: {
conclusion: CheckRunStatus.FAILURE,
title: 'Label Mismatch with No Changes',
summary: "No changes detected despite the presence of 'semver/minor' or 'semver/major' labels.",
Comment on lines +13 to +15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be a way of overriding this, something like like our fast-track or skip-backport-check labels to bypass trop / cation checks.

This isn't 100% provably correct, for instance:

  • You can make a semver/major change with 0 API surface changing, for instance enabling a feature by default (Cooke Encryption comes to mind as a semver/major with 0 API)
  • You can make a semver/none change that modifies the API surface. For instance docs fixes commonly "modify" the electron.d.ts file but don't actually change the API surface. So unless this check gets smarter around diffing actual API surface rather than just the electron.d.ts file we need a way to say "in this case we're all good"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out the edge cases that I haven't considered!

I can add a new label that's skip-type-diff-check but do you think it adds unnecessary complexity to the process? After all, the primary purpose of these checks is to provide informational cues to users.

What if for expected valid changes, we set the status to CheckRunStatus.SUCCESS and for suspected invalid changes, we set the status to CheckRunStatus.NEUTRAL with a summary that gives a warning that unexpected changes exist but feel free to skip if it's intentional.

This way, it serves the purpose of providing information without adding process overhead. If we feel like the override tag is better, happy to add it!!

},
validChanges: {
conclusion: CheckRunStatus.NEUTRAL,
title: 'Changes Detected',
summary: '',
},
invalidChanges: {
conclusion: CheckRunStatus.FAILURE,
title: 'Label Mismatch with Changes Detected',
summary: "Changes detected despite the presence of 'semver/none' label. ",
Comment on lines +23 to +25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be a way of overriding this

},
};

// Helper function to check if a semver label is valid for if `electron.d.ts` changes detected
function isSemverLabelValidForChanges(semverLabel: string | undefined) {
if (!semverLabel) {
return false;
}

return new Set([SEMVER_LABELS.PATCH, SEMVER_LABELS.MINOR, SEMVER_LABELS.MAJOR]).has(semverLabel);
}

function isSemverLabelValidForNoChanges(semverLabel: string | undefined) {
return !semverLabel || semverLabel === SEMVER_LABELS.NONE;
}

// Function to get the appropriate check status
export function getCheckStatusItems({
context,
hasChanges,
}: {
context: PRContext;
hasChanges: boolean;
}) {
const semverLabel = getSemverLabel(context.payload.pull_request);

if (hasChanges) {
// If changes are detected
return isSemverLabelValidForChanges(semverLabel?.name)
? checkStatuses.validChanges
: checkStatuses.invalidChanges;
}
// If no changes are detected
return isSemverLabelValidForNoChanges(semverLabel?.name)
? checkStatuses.validNoChanges
: checkStatuses.invalidNoChanges;
}
6 changes: 6 additions & 0 deletions src/utils/label-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { SEMVER_PREFIX } from '../constants';
import { PR } from '../types';

export const getSemverLabel = (pr: Pick<PR, 'labels'>) => {
return pr.labels.find((l: any) => l.name.startsWith(SEMVER_PREFIX));
};
Loading