From 53718073fee3b3af31a9d1bba9c900d989699e55 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Thu, 25 Jan 2024 17:38:18 +0100 Subject: [PATCH] Refactor of failed reviews objects (#112) This creates a new type of object: `ReviewFailure`. This object is an abstract class that contains all the information of the errors. By having each kind of error implementing it, we can customize the reports. This was done because reporting problems in the PR is basically the most exposed part (and the clearer it is, the less that people will ask for help). Each class has it's own summary to generate and returns the reviewers that should be requested. It allows customization per class. It can solve a lot of problems in the future. Two error classes were created: `CommonRuleFailure` and `FellowMissingRankFailure` ### CommonRuleFailure Wraps all the errors for all the non fellows rules and list what users are required (and which reviews counted towards fulfilling the rule). ### FellowMissingRankFailure Quite similar to the `CommonRuleFailure` error, but also handles the rank required. In the future I'll add one for `Fellow Score`, which will be a requirement for detailing the information in #110. # Other changes - Renamed structs to contain more clear names. - Fixed summary not being written for action when check was already existing - Simplified `generateCheckRunData` - now it is only a couple of lines and the `if` checks are done inside the errors - Changed `evaluateCondition` and `andDistinctEvaluation` return type. - Before they were returning an `[true] | [false, ErrorStruct]` tuple. After thinking about it, it is simpler to return a `ErrorStruct | null` type and simply checking if it's null. - If I would be returning a different object with the `true` condition I could have keep it, but for this case it was over engineered. - Changed returned type for `fellowsEvaluation` - It now returns the error object directly when applicable. --- action.yml | 2 +- src/failures/commonRules.ts | 48 ++++ src/failures/fellowsRules.ts | 44 ++++ src/failures/index.ts | 3 + src/failures/types.ts | 86 +++++++ src/github/check.ts | 11 +- src/runner.ts | 225 ++++++------------ src/test/index.test.ts | 5 +- src/test/runner/conditions.test.ts | 32 ++- src/test/runner/runner.test.ts | 22 +- src/test/runner/validation/and.test.ts | 15 +- .../runner/validation/andDistinct.test.ts | 46 ++-- src/test/runner/validation/fellows.test.ts | 3 +- src/test/runner/validation/or.test.ts | 5 +- src/util.ts | 6 + 15 files changed, 330 insertions(+), 223 deletions(-) create mode 100644 src/failures/commonRules.ts create mode 100644 src/failures/fellowsRules.ts create mode 100644 src/failures/index.ts create mode 100644 src/failures/types.ts 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}`;