diff --git a/action.yml b/action.yml index e75b43d..9689a0a 100644 --- a/action.yml +++ b/action.yml @@ -32,4 +32,4 @@ outputs: runs: using: 'docker' - image: 'docker://ghcr.io/paritytech/review-bot/action:2.3.1' + image: 'Dockerfile' diff --git a/src/failures/commonRules.ts b/src/failures/commonRules.ts new file mode 100644 index 0000000..710af73 --- /dev/null +++ b/src/failures/commonRules.ts @@ -0,0 +1,48 @@ +import { summary } from "@actions/core"; + +import { toHandle } from "../util"; +import { RequiredReviewersData, ReviewFailure, RuleFailedSummary } from "./types"; + +export class CommonRuleFailure extends ReviewFailure { + public readonly usersToRequest: string[]; + public readonly teamsToRequest: string[]; + + constructor(report: RuleFailedSummary & RequiredReviewersData) { + super(report); + this.usersToRequest = report.usersToRequest ?? []; + this.teamsToRequest = report.teamsToRequest ?? []; + } + + generateSummary(): typeof summary { + let text = super.generateSummary(); + + if (this.usersToRequest.length > 0) { + text = text.addHeading("Missing users", 3).addList(this.usersToRequest); + } + if (this.teamsToRequest.length > 0) { + text = text.addHeading("Missing reviews from teams", 3).addList(this.teamsToRequest); + } + + if (this.countingReviews.length > 0) { + text = text + .addHeading("Users approvals that counted towards this rule", 3) + .addEOL() + .addList(this.countingReviews.map(toHandle)) + .addEOL(); + } + + if (this.missingUsers.length > 0) + text = text.addDetails( + "GitHub users whose approval counts", + `This is a list of all the GitHub users whose approval would count towards fulfilling this rule:\n\n - ${this.missingUsers + .map(toHandle) + .join("\n - ")}`, + ); + + return text; + } + + getRequestLogins(): { users: string[]; teams: string[] } { + return { users: this.usersToRequest, teams: this.teamsToRequest }; + } +} diff --git a/src/failures/fellowsRules.ts b/src/failures/fellowsRules.ts new file mode 100644 index 0000000..623d399 --- /dev/null +++ b/src/failures/fellowsRules.ts @@ -0,0 +1,44 @@ +import { summary } from "@actions/core"; + +import { toHandle } from "../util"; +import { ReviewFailure, RuleFailedSummary } from "./types"; + +export class FellowMissingRankFailure extends ReviewFailure { + constructor( + ruleInfo: RuleFailedSummary, + public readonly missingRank: number, + ) { + super(ruleInfo); + } + + generateSummary(): typeof summary { + let text = super.generateSummary(); + + text = text + .addHeading("Missing reviews from Fellows", 3) + .addEOL() + .addRaw(`Missing reviews from rank \`${this.missingRank}\` or above`) + .addEOL(); + if (this.missingUsers.length > 0) + text = text.addDetails( + "GitHub users whose approval counts", + `This is a list of all the GitHub users who are rank ${this.missingRank} or above:\n\n - ${this.missingUsers + .map(toHandle) + .join("\n - ")}`, + ); + + if (this.countingReviews.length > 0) { + text = text + .addHeading("Users approvals that counted towards this rule", 3) + .addEOL() + .addList(this.countingReviews.map(toHandle)) + .addEOL(); + } + + return text; + } + + getRequestLogins(): { users: string[]; teams: string[] } { + return { users: [], teams: [] }; + } +} diff --git a/src/failures/index.ts b/src/failures/index.ts new file mode 100644 index 0000000..a7d2a65 --- /dev/null +++ b/src/failures/index.ts @@ -0,0 +1,3 @@ +export * from "./types"; +export * from "./commonRules"; +export * from "./fellowsRules"; diff --git a/src/failures/types.ts b/src/failures/types.ts new file mode 100644 index 0000000..9e6dbbb --- /dev/null +++ b/src/failures/types.ts @@ -0,0 +1,86 @@ +import { summary } from "@actions/core"; + +import { RuleTypes } from "../rules/types"; + +/** Object containing the report on why a rule failed */ +export type RuleFailedReport = { + /** The amount of missing reviews to fulfill the requirements */ + missingReviews: number; + /** The users who would qualify to complete those reviews */ + missingUsers: string[]; + /** If applicable, reviews that count towards this rule */ + countingReviews: string[]; +}; + +export type RuleFailedSummary = { + type: RuleTypes; + name: string; +} & RuleFailedReport; + +export type RequiredReviewersData = { + /** If applicable, the teams that should be requested to review */ + teamsToRequest?: string[]; + /** If applicable, the users that should be requested to review */ + usersToRequest?: string[]; +}; + +/** Class which contains the reports of a failed rule + * Here you can find details on why a rule failed and what requirements it has + */ +export abstract class ReviewFailure { + public readonly name: string; + public readonly type: RuleTypes; + /** The amount of missing reviews */ + public readonly missingReviews: number; + + /** Approvals that counted towards this rule */ + public readonly countingReviews: string[]; + + /** List of users who would classify to approve this rule */ + public readonly missingUsers: string[]; + + constructor(ruleInfo: RuleFailedSummary) { + this.name = ruleInfo.name; + this.type = ruleInfo.type; + this.missingReviews = ruleInfo.missingReviews; + this.countingReviews = ruleInfo.countingReviews; + this.missingUsers = ruleInfo.missingUsers; + } + + ruleExplanation(type: RuleTypes): string { + switch (type) { + case RuleTypes.Basic: + return "Rule 'Basic' requires a given amount of reviews from users/teams"; + case RuleTypes.And: + return "Rule 'And' has many required reviewers/teams and requires all of them to be fulfilled."; + case RuleTypes.Or: + return "Rule 'Or' has many required reviewers/teams and requires at least one of them to be fulfilled."; + case RuleTypes.AndDistinct: + return ( + "Rule 'And Distinct' has many required reviewers/teams and requires all of them to be fulfilled **by different users**.\n\n" + + "The approval of one user that belongs to _two teams_ will count only towards one team." + ); + case RuleTypes.Fellows: + return "Rule 'Fellows' requires a given amount of reviews from users whose Fellowship ranking is the required rank or great."; + default: + console.error("Out of range for rule type", type); + throw new Error("Unhandled rule"); + } + } + + generateSummary(): typeof summary { + return summary + .emptyBuffer() + .addHeading(this.name, 2) + .addHeading(`Missing ${this.missingReviews} review${this.missingReviews > 1 ? "s" : ""}`, 4) + .addDetails( + "Rule explanation", + this.ruleExplanation(this.type) + + "\n\n" + + "For more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)", + ); + } + + /** Get the users/teams whose review should be requested */ + abstract getRequestLogins(): { users: string[]; teams: string[] }; +} diff --git a/src/github/check.ts b/src/github/check.ts index 73bd32e..e29c414 100644 --- a/src/github/check.ts +++ b/src/github/check.ts @@ -49,6 +49,7 @@ export class GitHubChecksApi { this.logger.debug(`Found match: ${JSON.stringify(check)}`); await this.api.rest.checks.update({ ...checkData, check_run_id: check.id }); this.logger.debug("Updated check data"); + await this.writeSummary(checkData, check.html_url ?? ""); return; } } @@ -57,16 +58,20 @@ export class GitHubChecksApi { const check = await this.api.rest.checks.create(checkData); + await this.writeSummary(checkData, check.data.html_url ?? ""); + + this.logger.debug(JSON.stringify(check.data)); + } + + private async writeSummary(checkResult: CheckData, resultUrl: string) { // We publish it in the action summary await summary .emptyBuffer() .addHeading(checkResult.output.title) // We redirect to the check as it can changed if it is triggered again - .addLink("Find the result here", check.data.html_url ?? "") + .addLink("Find the result here", resultUrl) .addBreak() .addRaw(checkResult.output.text) .write(); - - this.logger.debug(JSON.stringify(check.data)); } } diff --git a/src/runner.ts b/src/runner.ts index 9dd0c9d..387feb9 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,38 +1,29 @@ -import { setOutput, summary } from "@actions/core"; +import { setOutput } from "@actions/core"; import { parse } from "yaml"; import { Inputs } from "."; +import { + CommonRuleFailure, + FellowMissingRankFailure, + RequiredReviewersData, + ReviewFailure, + RuleFailedReport, + RuleFailedSummary, +} from "./failures"; import { GitHubChecksApi } from "./github/check"; import { PullRequestApi } from "./github/pullRequest"; import { ActionLogger, CheckData, TeamApi } from "./github/types"; import { AndDistinctRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./rules/types"; import { validateConfig, validateRegularExpressions } from "./rules/validator"; -import { caseInsensitiveEqual, concatArraysUniquely } from "./util"; - -type ReviewReport = { - /** The amount of missing reviews to fulfill the requirements */ - missingReviews: number; - /** The users who would qualify to complete those reviews */ - missingUsers: string[]; - /** If applicable, the teams that should be requested to review */ - teamsToRequest?: string[]; - /** If applicable, the users that should be requested to review */ - usersToRequest?: string[]; - /** If applicable, the missing minimum fellows rank required to review */ - missingRank?: number; - /** If applicable, reviews that count towards this rule */ - countingReviews: string[]; -}; - -export type RuleReport = { name: string; type: RuleTypes } & ReviewReport; +import { concatArraysUniquely } from "./util"; -type ReviewState = [true] | [false, ReviewReport]; +type BaseRuleReport = RuleFailedReport & RequiredReviewersData; type PullRequestReport = { /** List of files that were modified by the PR */ files: string[]; /** List of all the failed review requirements */ - reports: RuleReport[]; + reports: ReviewFailure[]; }; /** Action in charge of running the GitHub action */ @@ -73,7 +64,7 @@ export class ActionRunner { async validatePullRequest({ rules }: ConfigurationFile): Promise { const modifiedFiles = await this.prApi.listModifiedFiles(); - const errorReports: RuleReport[] = []; + const errorReports: ReviewFailure[] = []; ruleCheck: for (const rule of rules) { try { @@ -95,42 +86,42 @@ export class ActionRunner { } switch (rule.type) { case RuleTypes.Basic: { - const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor); - if (!result) { - this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push({ ...missingData, name: rule.name, type: rule.type }); + const ruleError = await this.evaluateCondition(rule, rule.countAuthor); + if (ruleError) { + this.logger.error(`Missing the reviews from ${JSON.stringify(ruleError.missingUsers)}`); + errorReports.push(new CommonRuleFailure({ ...rule, ...ruleError })); } break; } case RuleTypes.And: { - const reports: ReviewReport[] = []; + const reports: RuleFailedReport[] = []; // We evaluate every individual condition for (const reviewer of rule.reviewers) { - const [result, missingData] = await this.evaluateCondition(reviewer, rule.countAuthor); - if (!result) { + const ruleError = await this.evaluateCondition(reviewer, rule.countAuthor); + if (ruleError) { // If one of the conditions failed, we add it to a report - reports.push(missingData); + reports.push(ruleError); } } if (reports.length > 0) { const finalReport = unifyReport(reports, rule.name, rule.type); this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); - errorReports.push(finalReport); + errorReports.push(new CommonRuleFailure(finalReport)); } break; } case RuleTypes.Or: { - const reports: ReviewReport[] = []; + const reports: RuleFailedReport[] = []; for (const reviewer of rule.reviewers) { - const [result, missingData] = await this.evaluateCondition(reviewer, rule.countAuthor); - if (result) { - // This is a OR condition, so with ONE positive result + const ruleError = await this.evaluateCondition(reviewer, rule.countAuthor); + if (!ruleError) { + // This is an OR condition, so if we have one iteration without an error // we can continue the loop to check the following rule continue ruleCheck; } // But, until we get a positive case we add all the failed cases - reports.push(missingData); + reports.push(ruleError); } // If the loop was not skipped it means that we have errors @@ -146,23 +137,24 @@ export class ActionRunner { finalReport.missingReviews = lowerAmountOfReviewsNeeded; this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); // We unify the reports and push them for handling - errorReports.push(finalReport); + errorReports.push(new CommonRuleFailure(finalReport)); } break; } case RuleTypes.AndDistinct: { - const [result, missingData] = await this.andDistinctEvaluation(rule); - if (!result) { - this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push({ ...missingData, name: rule.name, type: rule.type }); + const ruleFailure = await this.andDistinctEvaluation(rule); + if (ruleFailure) { + this.logger.error(`Missing the reviews from ${JSON.stringify(ruleFailure.missingUsers)}`); + errorReports.push(new CommonRuleFailure({ ...rule, ...ruleFailure })); } break; } case RuleTypes.Fellows: { - const [result, missingData] = await this.fellowsEvaluation(rule); - if (!result) { - this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push({ ...missingData, name: rule.name, type: rule.type }); + const fellowReviewError = await this.fellowsEvaluation(rule); + if (fellowReviewError) { + this.logger.error(`Missing the reviews from ${JSON.stringify(fellowReviewError.missingReviews)}`); + // errorReports.push({ ...missingData, name: rule.name, type: rule.type }); + errorReports.push(fellowReviewError); } break; } @@ -180,13 +172,13 @@ export class ActionRunner { } async requestReviewers( - reports: RuleReport[], + reports: ReviewFailure[], preventReviewRequests: ConfigurationFile["preventReviewRequests"], ): Promise { if (reports.length === 0) { return; } - const finalReport: ReviewReport = { + const finalReport: RuleFailedReport & RequiredReviewersData = { missingReviews: 0, missingUsers: [], teamsToRequest: [], @@ -195,10 +187,11 @@ export class ActionRunner { }; for (const report of reports) { + const { teams, users } = report.getRequestLogins(); finalReport.missingReviews += report.missingReviews; finalReport.missingUsers = concatArraysUniquely(finalReport.missingUsers, report.missingUsers); - finalReport.teamsToRequest = concatArraysUniquely(finalReport.teamsToRequest, report.teamsToRequest); - finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest); + finalReport.teamsToRequest = concatArraysUniquely(finalReport.teamsToRequest, teams); + finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, users); finalReport.countingReviews = concatArraysUniquely(finalReport.countingReviews, report.countingReviews); } @@ -233,7 +226,7 @@ export class ActionRunner { /** Aggregates all the reports and generate a status report * This also filters the author of the PR if he belongs to the group of users */ - generateCheckRunData(reports: RuleReport[]): CheckData { + generateCheckRunData(reports: ReviewFailure[]): CheckData { // Count how many reviews are missing const missingReviews = reports.reduce((a, b) => a + b.missingReviews, 0); const failed = missingReviews > 0; @@ -251,62 +244,8 @@ export class ActionRunner { } for (const report of reports) { - const ruleExplanation = (type: RuleTypes) => { - switch (type) { - case RuleTypes.Basic: - return "Rule 'Basic' requires a given amount of reviews from users/teams"; - case RuleTypes.And: - return "Rule 'And' has many required reviewers/teams and requires all of them to be fulfilled."; - case RuleTypes.Or: - return "Rule 'Or' has many required reviewers/teams and requires at least one of them to be fulfilled."; - case RuleTypes.AndDistinct: - return ( - "Rule 'And Distinct' has many required reviewers/teams and requires all of them to be fulfilled **by different users**.\n\n" + - "The approval of one user that belongs to _two teams_ will count only towards one team." - ); - case RuleTypes.Fellows: - return "Rule 'Fellows' requires a given amount of reviews from users whose Fellowship ranking is the required rank or great."; - default: - console.error("Out of range for rule type", type); - return "Unhandled rule"; - } - }; - check.output.summary += `- **${report.name}**\n`; - let text = summary - .emptyBuffer() - .addHeading(report.name, 2) - .addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4) - .addDetails( - "Rule explanation", - `${ruleExplanation( - report.type, - )}\n\nFor more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)`, - ); - if (report.usersToRequest && report.usersToRequest.length > 0) { - text = text - .addHeading("Missing users", 3) - .addList(report.usersToRequest.filter((u) => !caseInsensitiveEqual(u, this.prApi.getAuthor()))); - } - if (report.teamsToRequest && report.teamsToRequest.length > 0) { - text = text.addHeading("Missing reviews from teams", 3).addList(report.teamsToRequest); - } - if (report.missingRank) { - text = text - .addHeading("Missing reviews from Fellows", 3) - .addEOL() - .addRaw(`Missing reviews from rank \`${report.missingRank}\` or above`) - .addEOL(); - } - if (report.countingReviews.length > 0) { - text = text - .addHeading("Users approvals that counted towards this rule", 3) - .addEOL() - .addList(report.countingReviews) - .addEOL(); - } - - check.output.text += text.stringify() + "\n"; + check.output.text += report.generateSummary().stringify() + "\n"; } return check; @@ -318,7 +257,7 @@ export class ActionRunner { * It splits all the required reviews into individual cases and applies a sudoku solving algorithm * Until it finds a perfect match or ran out of possible matches */ - async andDistinctEvaluation(rule: AndDistinctRule): Promise { + async andDistinctEvaluation(rule: AndDistinctRule): Promise { const requirements: { users: string[]; requiredApprovals: number }[] = []; // We get all the users belonging to each 'and distinct' review condition for (const reviewers of rule.reviewers) { @@ -334,7 +273,7 @@ export class ActionRunner { let countingReviews: string[] = []; // Utility method used to generate error - const generateErrorReport = (): ReviewReport => { + const generateErrorReport = (): BaseRuleReport => { const filterMissingUsers = (reviewData: { users?: string[] }[]): string[] => Array.from(new Set(reviewData.flatMap((r) => r.users ?? []).filter((u) => approvals.indexOf(u) < 0))); @@ -353,7 +292,7 @@ export class ActionRunner { if (approvals.length < requiredAmountOfReviews) { this.logger.warn(`Not enough approvals. Need at least ${requiredAmountOfReviews} and got ${approvals.length}`); // We return an error and request reviewers - return [false, generateErrorReport()]; + return generateErrorReport(); } this.logger.debug(`Required users to review: ${JSON.stringify(requirements)}`); @@ -377,10 +316,10 @@ export class ActionRunner { // If one of the rules doesn't have the required approval we fail the evaluation if (conditionApprovals.some((cond) => cond.matchingUsers.length === 0)) { this.logger.warn("One of the groups does not have any approvals"); - return [false, generateErrorReport()]; + return generateErrorReport(); } else if (conditionApprovals.some((cond) => cond.matchingUsers.length < cond.requiredApprovals)) { this.logger.warn("Not enough positive reviews to match a subcondition"); - return [false, generateErrorReport()]; + return generateErrorReport(); } /** @@ -424,24 +363,23 @@ export class ActionRunner { // We check by the end of this iteration if all the approvals could be assigned // and we have ran out of elements in the array if (workingArray.length === 0) { - return [true]; + return null; } } this.logger.warn("Didn't find any matches to match all the rules requirements"); // If, by the end of all the loops, there are still matches, we didn't find a solution so we fail the rule - return [false, generateErrorReport()]; + return generateErrorReport(); } /** Evaluates if the required reviews for a condition have been meet * @param rule Every rule check has this values which consist on the min required approvals and the reviewers. - * @returns a [bool, error data] tuple which evaluates if the condition (not the rule itself) has fulfilled the requirements - * @see-also ReviewError + * @returns an object with the error report if the rule failed, or a null object if the rule passed */ async evaluateCondition( rule: { minApprovals: number } & Reviewers, countAuthor: boolean = false, - ): Promise { + ): Promise { this.logger.debug(JSON.stringify(rule)); // This is a list of all the users that need to approve a PR @@ -487,25 +425,22 @@ export class ActionRunner { this.logger.warn(`${missingReviews} reviews are missing.`); // If we have at least one missing review, we return an object with the list of missing reviewers, and // which users/teams we should request to review - return [ - false, - { - missingReviews, - // Remove all the users who approved the PR + the author (if he belongs to the group) - missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), - teamsToRequest: rule.teams ? rule.teams : undefined, - usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined, - countingReviews, - }, - ]; + return { + missingReviews, + // Remove all the users who approved the PR + the author (if he belongs to the group) + missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), + teamsToRequest: rule.teams ? rule.teams : undefined, + usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined, + countingReviews, + }; } else { this.logger.info("Rule requirements fulfilled"); // If we don't have any missing reviews, we return the succesful case - return [true]; + return null; } } - async fellowsEvaluation(rule: FellowsRule): Promise { + async fellowsEvaluation(rule: FellowsRule): Promise { // This is a list of all the users that need to approve a PR const requiredUsers: string[] = await this.polkadotApi.getTeamMembers(rule.minRank.toString()); @@ -543,20 +478,19 @@ export class ActionRunner { this.logger.warn(`${missingReviews} reviews are missing.`); // If we have at least one missing review, we return an object with the list of missing reviewers, and // which users/teams we should request to review - return [ - false, + return new FellowMissingRankFailure( { + ...rule, missingReviews, - // Remove all the users who approved the PR + the author (if he belongs to the group) - missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), - missingRank: rule.minRank, countingReviews, + missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), }, - ]; + rule.minRank, + ); } else { this.logger.info("Rule requirements fulfilled"); - // If we don't have any missing reviews, we return the succesful case - return [true]; + // If we don't have any missing reviews, we return no error + return null; } } @@ -637,19 +571,11 @@ export class ActionRunner { } } -const getRequiredRanks = (reports: Pick[]): number[] | undefined => { - const ranks = reports.map((r) => r.missingRank).filter((rank) => rank !== undefined && rank !== null) as number[]; - if (ranks.length > 0) { - return ranks; - } else { - return undefined; - } -}; - -const unifyReport = (reports: ReviewReport[], name: string, type: RuleTypes): RuleReport => { - const ranks = getRequiredRanks(reports); - const missingRank = ranks ? Math.max(...(ranks as number[])) : undefined; - +const unifyReport = ( + reports: (RuleFailedReport & RequiredReviewersData)[], + name: string, + type: RuleTypes, +): RuleFailedSummary & RequiredReviewersData => { return { missingReviews: reports.reduce((a, b) => a + b.missingReviews, 0), missingUsers: [...new Set(reports.flatMap((r) => r.missingUsers))], @@ -657,7 +583,6 @@ const unifyReport = (reports: ReviewReport[], name: string, type: RuleTypes): Ru usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))], name, type, - missingRank, countingReviews: [...new Set(reports.flatMap((r) => r.countingReviews))], }; }; diff --git a/src/test/index.test.ts b/src/test/index.test.ts index 01c09bb..304e8ab 100644 --- a/src/test/index.test.ts +++ b/src/test/index.test.ts @@ -4,11 +4,12 @@ import { existsSync, openSync, readFileSync, unlinkSync } from "fs"; import { any, DeepMockProxy, mock, mockDeep, MockProxy } from "jest-mock-extended"; import { join } from "path"; +import { ReviewFailure } from "../failures"; import { GitHubChecksApi } from "../github/check"; import { PullRequestApi } from "../github/pullRequest"; import { GitHubTeamsApi } from "../github/teams"; import { ActionLogger, GitHubClient, TeamApi } from "../github/types"; -import { ActionRunner, RuleReport } from "../runner"; +import { ActionRunner } from "../runner"; type ReportName = | "CI files" @@ -19,7 +20,7 @@ type ReportName = | "FRAME coders substrate"; /** Utility method to get a particular report from a list */ -const getReport = (reports: RuleReport[], name: ReportName): RuleReport => { +const getReport = (reports: ReviewFailure[], name: ReportName): ReviewFailure => { for (const report of reports) { if (report.name === name) { return report; diff --git a/src/test/runner/conditions.test.ts b/src/test/runner/conditions.test.ts index 4bad1f0..d9a2a10 100644 --- a/src/test/runner/conditions.test.ts +++ b/src/test/runner/conditions.test.ts @@ -52,19 +52,18 @@ describe("evaluateCondition tests", () => { }); test("should pass if required users approved the PR", async () => { - const [result] = await runner.evaluateCondition({ minApprovals: 1, users: [users[0]] }); - expect(result).toBeTruthy(); + const result = await runner.evaluateCondition({ minApprovals: 1, users: [users[0]] }); + expect(result).toBeNull(); }); test("should pass if required amount of users approved the PR", async () => { - const [result] = await runner.evaluateCondition({ minApprovals: 2, users: [users[0], users[users.length - 1]] }); - expect(result).toBeTruthy(); + const result = await runner.evaluateCondition({ minApprovals: 2, users: [users[0], users[users.length - 1]] }); + expect(result).toBeNull(); }); test("should fail if not all required users approved the PR", async () => { const newUser = "missing-user"; - const [result, missingData] = await runner.evaluateCondition({ minApprovals: 2, users: [users[0], newUser] }); - expect(result).toBeFalsy(); + const missingData = await runner.evaluateCondition({ minApprovals: 2, users: [users[0], newUser] }); expect(missingData?.missingUsers).toContainEqual(newUser); expect(missingData?.missingUsers).not.toContainEqual(users[0]); expect(missingData?.usersToRequest).toContainEqual(newUser); @@ -82,21 +81,20 @@ describe("evaluateCondition tests", () => { test("should pass if required users approved the PR", async () => { teamsApi.getTeamMembers.mockResolvedValue(users); - const [result] = await runner.evaluateCondition({ minApprovals: 1, teams: [team] }); - expect(result).toBeTruthy(); + const result = await runner.evaluateCondition({ minApprovals: 1, teams: [team] }); + expect(result).toBeNull(); }); test("should pass if required amount of users approved the PR", async () => { teamsApi.getTeamMembers.mockResolvedValue(users); - const [result] = await runner.evaluateCondition({ minApprovals: 2, teams: [team] }); - expect(result).toBeTruthy(); + const result = await runner.evaluateCondition({ minApprovals: 2, teams: [team] }); + expect(result).toBeNull(); }); test("should fail if not enough members of a team approved the PR", async () => { api.listApprovedReviewsAuthors.mockResolvedValue([users[0]]); teamsApi.getTeamMembers.mockResolvedValue(users); - const [result, missingData] = await runner.evaluateCondition({ minApprovals: 2, teams: [team] }); - expect(result).toBeFalsy(); + const missingData = await runner.evaluateCondition({ minApprovals: 2, teams: [team] }); expect(missingData?.missingUsers).toEqual(users.slice(1)); expect(missingData?.missingUsers).not.toContainEqual(users[0]); expect(missingData?.usersToRequest).toBeUndefined(); @@ -116,8 +114,8 @@ describe("evaluateCondition tests", () => { ]); teamsApi.getTeamMembers.calledWith(team1.name).mockResolvedValue(team1.users); teamsApi.getTeamMembers.calledWith(team2.name).mockResolvedValue(team2.users); - const [result] = await runner.evaluateCondition({ minApprovals: 4, teams: [team1.name, team2.name] }); - expect(result).toBeTruthy(); + const result = await runner.evaluateCondition({ minApprovals: 4, teams: [team1.name, team2.name] }); + expect(result).toBeNull(); }); test("should not duplicate user if they belong to more than one team", async () => { @@ -126,8 +124,7 @@ describe("evaluateCondition tests", () => { teamsApi.getTeamMembers.calledWith(team1.name).mockResolvedValue(team1.users); teamsApi.getTeamMembers.calledWith(team2.name).mockResolvedValue(team2.users); api.listApprovedReviewsAuthors.mockResolvedValue([]); - const [result, report] = await runner.evaluateCondition({ minApprovals: 3, teams: [team1.name, team2.name] }); - expect(result).toBeFalsy(); + const report = await runner.evaluateCondition({ minApprovals: 3, teams: [team1.name, team2.name] }); // Should not send required users more than once expect(report?.missingUsers).toEqual([...team1.users, team2.users[0]]); expect(report?.teamsToRequest).toEqual([team1.name, team2.name]); @@ -137,12 +134,11 @@ describe("evaluateCondition tests", () => { test("should not duplicate users if they belong to team and user list", async () => { teamsApi.getTeamMembers.calledWith(team).mockResolvedValue(users); api.listApprovedReviewsAuthors.mockResolvedValue([]); - const [result, report] = await runner.evaluateCondition({ + const report = await runner.evaluateCondition({ minApprovals: 1, teams: [team], users: [users[0]], }); - expect(result).toBeFalsy(); // Should not send required users more than once expect(report?.missingUsers).toEqual(users); expect(report?.teamsToRequest).toEqual([team]); diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 8dfd5e7..e1dca23 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -1,11 +1,12 @@ /* eslint-disable @typescript-eslint/unbound-method */ import { mock, MockProxy } from "jest-mock-extended"; +import { ReviewFailure } from "../../failures"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../github/types"; import { ConfigurationFile, Rule, RuleTypes } from "../../rules/types"; -import { ActionRunner, RuleReport } from "../../runner"; +import { ActionRunner } from "../../runner"; describe("Shared validations", () => { let api: MockProxy; @@ -94,36 +95,29 @@ describe("Shared validations", () => { }); describe("Validation in requestReviewers", () => { - const exampleReport: RuleReport = { - name: "Example", - missingUsers: ["user-1", "user-2", "user-3"], - missingReviews: 2, - teamsToRequest: ["team-1"], - usersToRequest: ["user-1"], - type: RuleTypes.Basic, - countingReviews: [], - }; + const exampleFailure = mock(); + exampleFailure.getRequestLogins.mockReturnValue({ users: ["user-1"], teams: ["team-1"] }); test("should request reviewers if object is not defined", async () => { - await runner.requestReviewers([exampleReport], undefined); + await runner.requestReviewers([exampleFailure], undefined); expect(api.requestReview).toHaveBeenCalledWith({ users: ["user-1"], teams: ["team-1"] }); }); test("should not request user if he is defined", async () => { - await runner.requestReviewers([exampleReport], { users: ["user-1"] }); + await runner.requestReviewers([exampleFailure], { users: ["user-1"] }); expect(logger.info).toHaveBeenCalledWith("Filtering users to request a review from."); expect(api.requestReview).toHaveBeenCalledWith({ teams: ["team-1"], users: [] }); }); test("should not request team if it is defined", async () => { - await runner.requestReviewers([exampleReport], { teams: ["team-1"] }); + await runner.requestReviewers([exampleFailure], { teams: ["team-1"] }); expect(logger.info).toHaveBeenCalledWith("Filtering teams to request a review from."); expect(api.requestReview).toHaveBeenCalledWith({ teams: [], users: ["user-1"] }); }); test("should request reviewers if the team and user are not the same", async () => { - await runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] }); + await runner.requestReviewers([exampleFailure], { users: ["user-pi"], teams: ["team-alpha"] }); expect(api.requestReview).toHaveBeenCalledWith({ users: ["user-1"], teams: ["team-1"] }); }); }); diff --git a/src/test/runner/validation/and.test.ts b/src/test/runner/validation/and.test.ts index 51bc9b4..8f4c77b 100644 --- a/src/test/runner/validation/and.test.ts +++ b/src/test/runner/validation/and.test.ts @@ -77,8 +77,9 @@ describe("'And' rule validation", () => { const [result] = reports; expect(result.missingReviews).toEqual(2); expect(result.missingUsers).toEqual(users); - expect(result.teamsToRequest).toContainEqual("abc"); - expect(result.usersToRequest).toContainEqual(users[0]); + const toRequest = result.getRequestLogins(); + expect(toRequest.teams).toContainEqual("abc"); + expect(toRequest.users).toContainEqual(users[0]); }); test("should report the agregated amount of missing reviews", async () => { @@ -143,8 +144,9 @@ describe("'And' rule validation", () => { const [result] = reports; expect(result.missingReviews).toEqual(1); expect(result.missingUsers).toEqual(teamCba); - expect(result.teamsToRequest).toEqual(["cba"]); - expect(result.usersToRequest).toHaveLength(0); + const toRequest = result.getRequestLogins(); + expect(toRequest.teams).toEqual(["cba"]); + expect(toRequest.users).toHaveLength(0); }); test("should report missing individual user if one of the rules have not been met", async () => { @@ -168,8 +170,9 @@ describe("'And' rule validation", () => { const [result] = reports; expect(result.missingReviews).toEqual(2); expect(result.missingUsers).toEqual(individualUsers); - expect(result.teamsToRequest).toHaveLength(0); - expect(result.usersToRequest).toEqual(individualUsers); + const toRequest = result.getRequestLogins(); + expect(toRequest.teams).toHaveLength(0); + expect(toRequest.users).toEqual(individualUsers); }); }); }); diff --git a/src/test/runner/validation/andDistinct.test.ts b/src/test/runner/validation/andDistinct.test.ts index 39a6594..4eb278b 100644 --- a/src/test/runner/validation/andDistinct.test.ts +++ b/src/test/runner/validation/andDistinct.test.ts @@ -37,8 +37,7 @@ describe("'And distinct' rule validation", () => { name: "test", condition: { include: [] }, }; - const [result, error] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const error = await runner.andDistinctEvaluation(rule); expect(error?.missingReviews).toBe(4); expect(logger.warn).toHaveBeenCalledWith("Not enough approvals. Need at least 4 and got 0"); }); @@ -56,8 +55,7 @@ describe("'And distinct' rule validation", () => { }; api.listApprovedReviewsAuthors.mockResolvedValue([users[0]]); - const [result, error] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const error = await runner.andDistinctEvaluation(rule); expect(error?.missingReviews).toBe(4); expect(logger.warn).toHaveBeenCalledWith("Not enough approvals. Need at least 4 and got 1"); }); @@ -75,8 +73,7 @@ describe("'And distinct' rule validation", () => { }; api.listApprovedReviewsAuthors.mockResolvedValue(users); - const [result, error] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const error = await runner.andDistinctEvaluation(rule); expect(error?.missingReviews).toBe(3); expect(logger.warn).toHaveBeenCalledWith("One of the groups does not have any approvals"); }); @@ -94,8 +91,7 @@ describe("'And distinct' rule validation", () => { }; api.listApprovedReviewsAuthors.mockResolvedValue([users[1], users[2], "abc"]); - const [result, error] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const error = await runner.andDistinctEvaluation(rule); expect(error?.missingReviews).toBe(3); expect(logger.warn).toHaveBeenCalledWith("Didn't find any matches to match all the rules requirements"); }); @@ -112,8 +108,7 @@ describe("'And distinct' rule validation", () => { }; api.listApprovedReviewsAuthors.mockResolvedValue(users); - const [result, error] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const error = await runner.andDistinctEvaluation(rule); expect(error?.missingReviews).toBe(3); expect(logger.warn).toHaveBeenCalledWith("Not enough positive reviews to match a subcondition"); }); @@ -129,8 +124,7 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; - const [result, error] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const error = await runner.andDistinctEvaluation(rule); const hasDuplicates = (arr: T[]) => arr.some((item, index) => arr.indexOf(item) !== index); expect(hasDuplicates(error?.missingUsers as string[])).toBeFalsy(); }); @@ -148,8 +142,8 @@ describe("'And distinct' rule validation", () => { }; api.listApprovedReviewsAuthors.mockResolvedValue([...users]); api.getAuthor.mockReturnValue("random"); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(false); + const result = await runner.andDistinctEvaluation(rule); + expect(result).not.toBeNull(); }); }); @@ -165,8 +159,8 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; api.listApprovedReviewsAuthors.mockResolvedValue([users[0], "abc"]); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(true); + const result = await runner.andDistinctEvaluation(rule); + expect(result).toBeNull(); }); test("should pass with a valid combination of matches", async () => { @@ -180,8 +174,8 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; api.listApprovedReviewsAuthors.mockResolvedValue([users[0], "abc"]); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(true); + const result = await runner.andDistinctEvaluation(rule); + expect(result).toBeNull(); }); test("should pass with a valid complicate combination of matches", async () => { @@ -196,8 +190,8 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; api.listApprovedReviewsAuthors.mockResolvedValue([users[0], "abc", "def"]); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(true); + const result = await runner.andDistinctEvaluation(rule); + expect(result).toBeNull(); }); test("should pass with a valid complicate combination of matches and more than one min approval in a rule", async () => { @@ -212,8 +206,8 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; api.listApprovedReviewsAuthors.mockResolvedValue([users[0], "abc", "def", "fgh"]); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(true); + const result = await runner.andDistinctEvaluation(rule); + expect(result).toBeNull(); }); test("should pass with a valid very complicate combination of matches", async () => { @@ -228,8 +222,8 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; api.listApprovedReviewsAuthors.mockResolvedValue([...users, "abc", "def"]); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(true); + const result = await runner.andDistinctEvaluation(rule); + expect(result).toBeNull(); }); test("should call listApprovedReviewsAuthors with true", async () => { @@ -244,8 +238,8 @@ describe("'And distinct' rule validation", () => { condition: { include: [] }, }; api.listApprovedReviewsAuthors.mockResolvedValue(users); - const [result] = await runner.andDistinctEvaluation(rule); - expect(result).toBe(true); + const result = await runner.andDistinctEvaluation(rule); + expect(result).toBeNull(); expect(api.listApprovedReviewsAuthors).lastCalledWith(true); }); }); diff --git a/src/test/runner/validation/fellows.test.ts b/src/test/runner/validation/fellows.test.ts index f0ac700..26355c2 100644 --- a/src/test/runner/validation/fellows.test.ts +++ b/src/test/runner/validation/fellows.test.ts @@ -1,5 +1,6 @@ import { mock, MockProxy } from "jest-mock-extended"; +import { FellowMissingRankFailure } from "../../../failures"; import { GitHubChecksApi } from "../../../github/check"; import { PullRequestApi } from "../../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../../github/types"; @@ -77,7 +78,7 @@ describe("'Fellows' rule validation", () => { const [result] = reports; expect(result.missingReviews).toEqual(1); expect(result.missingUsers).toEqual([users[2]]); - expect(result.missingRank).toEqual(4); + expect((result as FellowMissingRankFailure).missingRank).toEqual(4); }); test("should throw error if no fellows of a given rank are found", async () => { diff --git a/src/test/runner/validation/or.test.ts b/src/test/runner/validation/or.test.ts index 996bfbd..da919ba 100644 --- a/src/test/runner/validation/or.test.ts +++ b/src/test/runner/validation/or.test.ts @@ -100,8 +100,9 @@ describe("'Or' rule validation", () => { const [result] = reports; expect(result.missingReviews).toEqual(1); expect(result.missingUsers).toEqual(users); - expect(result.teamsToRequest).toContainEqual("abc"); - expect(result.usersToRequest).toEqual(individualUsers); + const toRequest = result.getRequestLogins(); + expect(toRequest.teams).toContainEqual("abc"); + expect(toRequest.users).toEqual(individualUsers); }); test("should show the lowest amount of reviews needed to fulfill the rule", async () => { diff --git a/src/util.ts b/src/util.ts index dad646b..54be919 100644 --- a/src/util.ts +++ b/src/util.ts @@ -20,3 +20,9 @@ export function concatArraysUniquely(arr1?: T[], arr2?: T[]): T[] { export function caseInsensitiveEqual(a: T, b: T): boolean { return a.localeCompare(b, undefined, { sensitivity: "accent" }) === 0; } + +/** + * Converts a username to it's handle (adds the '@' at the beggining) + * @param handle The username + */ +export const toHandle = (handle: string): string => `@${handle}`;