diff --git a/README.md b/README.md index ad942a8..14528b3 100644 --- a/README.md +++ b/README.md @@ -237,7 +237,6 @@ rules: users: - user-1 - user-2 - minFellowsRank: 2 countAuthor: true allowedToSkipRule: teams: @@ -257,13 +256,9 @@ It has the same parameters as a normal rule: - **Optional**: Defaults to 1. - Must be greater than the number of users available (you cannot request 5 approvals from a team of 4 users) - **teams**: An array of team *slugs* that need to review this file. - - *Optional if **minFellowsRank** or **users** are defined*. + - *Optional if **users** is defined*. - **users**: An array of the GitHub usernames of the users who need to review this file. - - *Optional if **teams** or **minFellowsRank** are defined*. -- **minFellowsRank**: A number defining the minimum [minFellowsRank a fellow](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship) needs to have to approve this PR. - - *Optional if **teams** or **users** are defined*. - - If it is set to minFellowsRank 3, it will need the approval of a fellow of minFellowsRank 3 or higher to pass. - - The username is fetched [from the additional field](https://github.com/polkadot-fellows/runtimes/issues/7) in the identity of the fellows. If it is null, the fellow’s review won’t count towards the required approval. + - *Optional if **teams** is defined*. - **countAuthor**: If the pull request author should be considered as an approval. - If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total). - ** Optional**: Defaults to `false` @@ -308,13 +303,9 @@ rules: - **Optional**: Defaults to 1. - Must be greater than the number of users available (you cannot request 5 approvals from a team of 4 users) - **teams**: An array of team *slugs* that need to review this file. - - *Optional if **minFellowsRank** or **users** are defined*. + - *Optional if **users** is defined*. - **users**: An array of the GitHub usernames of the users who need to review this file. - - *Optional if **teams** or **minFellowsRank** are defined*. - - **minFellowsRank**: A number defining the minimum [minFellowsRank a fellow](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship) needs to have to approve this PR. - - *Optional if **teams** or **users** are defined*. - - If it is set to minFellowsRank 3, it will need the approval of a fellow of minFellowsRank 3 or higher to pass. - - The username is fetched [from the additional field](https://github.com/polkadot-fellows/runtimes/issues/7) in the identity of the fellows. If it is null, the fellow’s review won’t count towards the required approval. + - *Optional if **teams** is defined*. ##### Or rule logic This is a rule that has at least two available options of reviewers and needs **at least one group to approve**. @@ -365,7 +356,35 @@ The logic in this rule is the *same as the `and` rule **but** with one exception Meaning that if a user belongs to both `team-abc` and `team-example` their approval will count only towards one of the available options *even if they fulfill both needs*. This rule is useful when you need to make sure that at leasts two sets of eyes of two different teams review a Pull Request. - +#### Fellows rule +The fellows rule has a slight difference to all of the rules: +```yaml +- name: Fellows review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 2 + min_approvals: 2 +``` +The biggest difference is that it doesn’t have a reviewers type (it doesn’t have a `teams` or `users` field); instead, it has a `minRank` field. + +This field receives a number, which will be the lowest rank required to evaluate the PR, and then it fetches [all the fellows from the chain data](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship), filters only the one to belong to that rank or above and then [looks into their metadata for a field name `github` and the handle there](https://github.com/polkadot-fellows/runtimes/issues/7). + +After this is done, the resulting handles will be treated like a normal list of required users. + +It also has any other field from the [`basic rule`](#basic-rule) (with the exception of `users` and `teams`): +- **name** +- **conditions**: + - **include** is **required**. + - **exclude** is **optional**. +- **type**: Must be `fellows`. + - **countAuthor**: If the pull request author should be considered as an approval. + - **Optional**: Defaults to `false`. +- **minRank**: Must be a number. + - **Required** ### Evaluating config If you want to evaluate the config file to find problems before merging it, we have a simple `cli` to do so. diff --git a/src/rules/types.ts b/src/rules/types.ts index 9e87f66..6c48e0a 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -3,9 +3,10 @@ export enum RuleTypes { And = "and", Or = "or", AndDistinct = "and-distinct", + Fellows = "fellows", } -export type Reviewers = { users?: string[]; teams?: string[]; minFellowsRank?: number; min_approvals: number }; +export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: number }; export interface Rule { name: string; @@ -33,11 +34,17 @@ export interface AndDistinctRule extends Rule { reviewers: Reviewers[]; } +export interface FellowsRule extends Rule { + type: RuleTypes.Fellows; + minRank: number; + min_approvals: number; +} + export interface ConfigurationFile { /** Based on the `type` parameter, Typescript converts the object to the correct type * @see {@link Rules} */ - rules: (BasicRule | AndRule | OrRule | AndDistinctRule)[]; + rules: (BasicRule | AndRule | OrRule | AndDistinctRule | FellowsRule)[]; preventReviewRequests?: { teams?: string[]; users?: string[]; diff --git a/src/rules/validator.ts b/src/rules/validator.ts index f26882e..a5f9d29 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -2,7 +2,7 @@ import { validate } from "@eng-automation/js"; import Joi from "joi"; import { ActionLogger } from "../github/types"; -import { AndRule, BasicRule, ConfigurationFile, Reviewers, Rule, RuleTypes } from "./types"; +import { AndRule, BasicRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./types"; /** For the users or team schema. Will be recycled A LOT * Remember to add `.or("users", "teams")` to force at least one of the two to be defined @@ -10,7 +10,6 @@ import { AndRule, BasicRule, ConfigurationFile, Reviewers, Rule, RuleTypes } fro const reviewersObj = { users: Joi.array().items(Joi.string()).optional().empty(null), teams: Joi.array().items(Joi.string()).optional().empty(null), - minFellowsRank: Joi.number().optional().min(1).empty(null), }; const reviewerConditionObj = { ...reviewersObj, min_approvals: Joi.number().min(1).default(1) }; @@ -26,7 +25,9 @@ const ruleSchema = Joi.object().keys({ exclude: Joi.array().items(Joi.string()).optional().allow(null), }), allowedToSkipRule: Joi.object>().keys(reviewersObj).optional().or("users", "teams"), - type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(), + type: Joi.string() + .valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct, RuleTypes.Fellows) + .required(), }); /** General Configuration schema. @@ -35,7 +36,7 @@ const ruleSchema = Joi.object().keys({ */ export const generalSchema = Joi.object().keys({ rules: Joi.array().items(ruleSchema).unique("name").required(), - preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams", "minFellowsRank"), + preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"), }); /** Basic rule schema @@ -43,17 +44,23 @@ export const generalSchema = Joi.object().keys({ */ export const basicRuleSchema = Joi.object() .keys({ ...reviewerConditionObj, countAuthor: Joi.boolean().default(false) }) - .or("users", "teams", "minFellowsRank"); + .or("users", "teams"); /** As, with the exception of basic, every other schema has the same structure, we can recycle this */ export const otherRulesSchema = Joi.object().keys({ reviewers: Joi.array() - .items(Joi.object().keys(reviewerConditionObj).or("users", "teams", "minFellowsRank")) + .items(Joi.object().keys(reviewerConditionObj).or("users", "teams")) .min(2) .required(), countAuthor: Joi.boolean().default(false), }); +export const fellowsRuleSchema = Joi.object().keys({ + countAuthor: Joi.boolean().default(false), + minRank: Joi.number().required().min(1).empty(null), + min_approvals: Joi.number().min(1).default(1), +}); + /** * Evaluates a config thoroughly. If there is a problem with it, it will throw. * @@ -77,6 +84,9 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n // Aside from the type, every other field in this rules is identical so we can // use any of these rules to validate the other fields. validatedConfig.rules[i] = validate(rule, otherRulesSchema, { message }); + } else if (type === "fellows") { + // Fellows have a specific config that uses ranks instead of usernames + validatedConfig.rules[i] = validate(rule, fellowsRuleSchema, { message }); } else { // eslint-disable-next-line @typescript-eslint/restrict-template-expressions throw new Error(`Rule ${name} has an invalid type: ${type}`); diff --git a/src/runner.ts b/src/runner.ts index 6cbce0a..885ff9c 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -5,7 +5,7 @@ import { Inputs } from "."; import { GitHubChecksApi } from "./github/check"; import { PullRequestApi } from "./github/pullRequest"; import { ActionLogger, CheckData, TeamApi } from "./github/types"; -import { AndDistinctRule, ConfigurationFile, Reviewers, Rule } from "./rules/types"; +import { AndDistinctRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./rules/types"; import { validateConfig, validateRegularExpressions } from "./rules/validator"; import { caseInsensitiveEqual, concatArraysUniquely } from "./util"; @@ -75,7 +75,7 @@ export class ActionRunner { ruleCheck: for (const rule of rules) { try { - this.logger.info(`Validating rule '${rule.name}'`); + this.logger.info(`Validating rule '${rule.name}' of type '${rule.type}'`); // We get all the files that were modified and match the rules condition const files = this.listFilesThatMatchRuleCondition(modifiedFiles, rule); // We check if there are any matches @@ -91,63 +91,81 @@ export class ActionRunner { continue; } } - if (rule.type === "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 }); - } - } else if (rule.type === "and") { - const reports: ReviewReport[] = []; - // We evaluate every individual condition - for (const reviewer of rule.reviewers) { - const [result, missingData] = await this.evaluateCondition(reviewer, rule.countAuthor); + switch (rule.type) { + case RuleTypes.Basic: { + const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor); if (!result) { - // If one of the conditions failed, we add it to a report - reports.push(missingData); + this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); + errorReports.push({ ...missingData, name: rule.name }); } + + break; } - if (reports.length > 0) { - const finalReport = unifyReport(reports, rule.name); - this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); - errorReports.push(finalReport); - } - } else if (rule.type === "or") { - const reports: ReviewReport[] = []; - 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 - // we can continue the loop to check the following rule - continue ruleCheck; + case RuleTypes.And: { + const reports: ReviewReport[] = []; + // We evaluate every individual condition + for (const reviewer of rule.reviewers) { + const [result, missingData] = await this.evaluateCondition(reviewer, rule.countAuthor); + if (!result) { + // If one of the conditions failed, we add it to a report + reports.push(missingData); + } } - // But, until we get a positive case we add all the failed cases - reports.push(missingData); + if (reports.length > 0) { + const finalReport = unifyReport(reports, rule.name); + this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); + errorReports.push(finalReport); + } + break; } + case RuleTypes.Or: { + const reports: ReviewReport[] = []; + 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 + // 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); + } - // If the loop was not skipped it means that we have errors - if (reports.length > 0) { - // We get the lowest amount of reviews needed to fulfill one of the reviews - const lowerAmountOfReviewsNeeded = reports - .map((r) => r.missingReviews) - .reduce((a, b) => (a < b ? a : b), 999); - // We get the lowest rank required - const ranks = getRequiredRanks(reports); - // We unify the reports - const finalReport = unifyReport(reports, rule.name); - // We set the value to the minimum neccesary - finalReport.missingReviews = lowerAmountOfReviewsNeeded; - finalReport.missingRank = ranks ? Math.min(...(ranks as number[])) : undefined; - this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); - // We unify the reports and push them for handling - errorReports.push(finalReport); + // If the loop was not skipped it means that we have errors + if (reports.length > 0) { + // We get the lowest amount of reviews needed to fulfill one of the reviews + const lowerAmountOfReviewsNeeded = reports + .map((r) => r.missingReviews) + .reduce((a, b) => (a < b ? a : b), 999); + // We get the lowest rank required + // We unify the reports + const finalReport = unifyReport(reports, rule.name); + // We set the value to the minimum neccesary + 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); + } + 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 }); + } + break; } - } else if (rule.type === "and-distinct") { - 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 }); + 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 }); + } + break; } + default: + throw new Error(`Rule type not found!`); } } catch (error: unknown) { // We only throw if there was an unexpected error, not if the check fails @@ -240,7 +258,11 @@ export class ActionRunner { text = text.addHeading("Missing reviews from teams", 3).addList(report.teamsToRequest); } if (report.missingRank) { - text = text.addHeading(`Missing reviews from Fellows of rank ${report.missingRank} or above`, 3); + text = text + .addHeading("Missing reviews from Fellows", 3) + .addEOL() + .addRaw(`Missing reviews from rank \`${report.missingRank}\` or above`) + .addEOL(); } check.output.text += text.stringify() + "\n"; @@ -273,9 +295,6 @@ export class ActionRunner { const filterMissingUsers = (reviewData: { users?: string[] }[]): string[] => Array.from(new Set(reviewData.flatMap((r) => r.users ?? []).filter((u) => approvals.indexOf(u) < 0))); - const ranks = rule.reviewers.map((r) => r.minFellowsRank).filter((rank) => rank !== undefined && rank !== null); - const missingRank = ranks.length > 0 ? Math.max(...(ranks as number[])) : undefined; - // Calculating all the possible combinations to see the missing reviewers is very complicated // Instead we request everyone who hasn't reviewed yet return { @@ -283,7 +302,6 @@ export class ActionRunner { missingUsers: filterMissingUsers(requirements), teamsToRequest: rule.reviewers.flatMap((r) => r.teams ?? []), usersToRequest: filterMissingUsers(rule.reviewers), - missingRank, }; }; @@ -427,7 +445,56 @@ export class ActionRunner { 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, - missingRank: rule.minFellowsRank, + }, + ]; + } else { + this.logger.info("Rule requirements fulfilled"); + // If we don't have any missing reviews, we return the succesful case + return [true]; + } + } + + 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()); + + if (requiredUsers.length === 0) { + throw new Error(`No users have been found with the rank ${rule.minRank} or above`); + } + + if (requiredUsers.length < rule.min_approvals) { + this.logger.error( + `${rule.min_approvals} approvals are required but only ${requiredUsers.length} user's approval count.`, + ); + throw new Error("The amount of required approvals is smaller than the amount of available users."); + } + + // We get the list of users that approved the PR + const approvals = await this.prApi.listApprovedReviewsAuthors(rule.countAuthor ?? false); + this.logger.info(`Found ${approvals.length} approvals.`); + + // This is the amount of reviews required. To succeed this should be 0 or lower + let missingReviews = rule.min_approvals; + for (const requiredUser of requiredUsers) { + // We check for the approvals, if it is a required reviewer we lower the amount of missing reviews + if (approvals.indexOf(requiredUser) > -1) { + missingReviews--; + } + } + + // Now we verify if we have any remaining missing review. + if (missingReviews > 0) { + const author = this.prApi.getAuthor(); + 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), + missingRank: rule.minRank, }, ]; } else { @@ -479,12 +546,6 @@ export class ActionRunner { users.add(user); } } - if (reviewers.minFellowsRank) { - const members = await this.polkadotApi.getTeamMembers(reviewers.minFellowsRank.toString()); - for (const member of members) { - users.add(member); - } - } return Array.from(users); } diff --git a/src/test/rules/basicRule.test.ts b/src/test/rules/basicRule.test.ts index 7869d68..00fcbbf 100644 --- a/src/test/rules/basicRule.test.ts +++ b/src/test/rules/basicRule.test.ts @@ -83,9 +83,7 @@ describe("Basic rule parsing", () => { - 'example' type: basic `); - await expect(runner.getConfigFile("")).rejects.toThrowError( - '"value" must contain at least one of [users, teams, minFellowsRank]', - ); + await expect(runner.getConfigFile("")).rejects.toThrowError('"value" must contain at least one of [users, teams]'); }); test("should default min_approvals to 1", async () => { diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index 1977694..f4d372a 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -6,7 +6,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../github/types"; -import { BasicRule, RuleTypes } from "../../rules/types"; +import { RuleTypes } from "../../rules/types"; import { ActionRunner } from "../../runner"; describe("Config Parsing", () => { @@ -358,65 +358,4 @@ describe("Config Parsing", () => { }); }); }); - - describe("rank field", () => { - it("should get rank as an number", async () => { - api.getConfigFile.mockResolvedValue(` - rules: - - name: Default review - condition: - include: - - 'example-include-rule-1' - type: basic - minFellowsRank: 2 - `); - const config = await runner.getConfigFile(""); - const rule = config.rules[0] as BasicRule; - expect(rule.minFellowsRank).toEqual(2); - }); - - it("should default rank to undefined", async () => { - api.getConfigFile.mockResolvedValue(` - rules: - - name: Default review - condition: - include: - - 'example-include-rule-1' - type: basic - teams: - - abc - `); - const config = await runner.getConfigFile(""); - const rule = config.rules[0] as BasicRule; - expect(rule.minFellowsRank).toBeUndefined(); - }); - - it("should throw with an invalid number", async () => { - api.getConfigFile.mockResolvedValue(` - rules: - - name: Default review - condition: - include: - - 'example-include-rule-1' - type: basic - minFellowsRank: -9 - `); - await expect(runner.getConfigFile("")).rejects.toThrowError( - '"minFellowsRank" must be greater than or equal to 1', - ); - }); - - it("should throw with an non number", async () => { - api.getConfigFile.mockResolvedValue(` - rules: - - name: Default review - condition: - include: - - 'example-include-rule-1' - type: basic - minFellowsRank: example - `); - await expect(runner.getConfigFile("")).rejects.toThrowError('"minFellowsRank" must be a number'); - }); - }); }); diff --git a/src/test/rules/fellows.test.ts b/src/test/rules/fellows.test.ts new file mode 100644 index 0000000..f5b7bd0 --- /dev/null +++ b/src/test/rules/fellows.test.ts @@ -0,0 +1,209 @@ +/* eslint-disable @typescript-eslint/unbound-method */ +/* eslint-disable @typescript-eslint/no-unsafe-assignment */ +/* eslint-disable @typescript-eslint/no-unsafe-call */ +import { mock, MockProxy } from "jest-mock-extended"; + +import { GitHubChecksApi } from "../../github/check"; +import { PullRequestApi } from "../../github/pullRequest"; +import { ActionLogger, TeamApi } from "../../github/types"; +import { FellowsRule } from "../../rules/types"; +import { ActionRunner } from "../../runner"; + +describe("Fellows rule parsing", () => { + let api: MockProxy; + let runner: ActionRunner; + beforeEach(() => { + api = mock(); + runner = new ActionRunner(api, mock(), mock(), mock(), mock()); + }); + test("should get minimal config", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 2 + `); + const config = await runner.getConfigFile(""); + expect(config.rules[0].name).toEqual("Test review"); + expect(config.rules[0].type).toEqual("fellows"); + }); + + test("should set the rank", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 4 + `); + const { rules } = await runner.getConfigFile(""); + expect(rules[0].type).toEqual("fellows"); + const fellowsRule = rules[0] as FellowsRule; + expect(fellowsRule.minRank).toEqual(4); + }); + + test("should fail without rank", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"minRank" is required'); + }); + + test("should fail without negative number", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: -3 + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"minRank" must be greater than or equal to 1'); + }); + + test("should fail with invalid number", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: cuatro + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"minRank" must be a number'); + }); + + test("should default min_approvals to 1", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 2 + `); + const config = await runner.getConfigFile(""); + const [rule] = config.rules; + if (rule.type === "fellows") { + expect(rule.min_approvals).toEqual(1); + } else { + throw new Error(`Rule type ${rule.type} is invalid`); + } + }); + + test("should fail with min_approvals in negative", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + min_approvals: -99 + minRank: 4 + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1'); + }); + + test("should fail with min_approvals in 0", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + min_approvals: 0 + minRank: 4 + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1'); + }); + + test("should default countAuthor to false", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 4 + `); + const config = await runner.getConfigFile(""); + const [rule] = config.rules; + if (rule.type === "fellows") { + expect(rule.countAuthor).toBeFalsy(); + } else { + throw new Error(`Rule type ${rule.type} is invalid`); + } + }); + + test("should fail if countAuthor is not a boolean", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 4 + countAuthor: bla + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"countAuthor" must be a boolean'); + }); + + test("should set countAuthor to true", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 4 + countAuthor: true + `); + const config = await runner.getConfigFile(""); + const [rule] = config.rules; + if (rule.type === "fellows") { + expect(rule.countAuthor).toBeTruthy(); + } else { + throw new Error(`Rule type ${rule.type} is invalid`); + } + }); +}); diff --git a/src/test/runner/conditions.test.ts b/src/test/runner/conditions.test.ts index ae00060..fd4e5f3 100644 --- a/src/test/runner/conditions.test.ts +++ b/src/test/runner/conditions.test.ts @@ -151,31 +151,4 @@ describe("evaluateCondition tests", () => { }); }); }); - describe("rank tests", () => { - test("should require rank users", async () => { - fellowsApi.getTeamMembers.mockResolvedValue(["user-1"]); - api.listApprovedReviewsAuthors.mockResolvedValue([]); - const [result, report] = await runner.evaluateCondition({ - min_approvals: 1, - minFellowsRank: 3, - }); - - expect(result).toBeFalsy(); - expect(report?.missingUsers).toEqual(["user-1"]); - expect(report?.teamsToRequest).toBeUndefined(); - expect(report?.usersToRequest).toBeUndefined(); - expect(report?.missingRank).toBe(3); - }); - - test("should pass with required rank users", async () => { - fellowsApi.getTeamMembers.mockResolvedValue(["user-1"]); - api.listApprovedReviewsAuthors.mockResolvedValue(["user-1"]); - const [result] = await runner.evaluateCondition({ - min_approvals: 1, - minFellowsRank: 3, - }); - - expect(result).toBeTruthy(); - }); - }); }); diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 6cf7135..38cc4e5 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -55,29 +55,6 @@ describe("Shared validations", () => { ); }); - test("validatePullRequest should return true if author belongs to allowedToSkipRule by rank", async () => { - const config: ConfigurationFile = { - rules: [ - { - name: "Rule allowedToSkipRule", - type: RuleTypes.Basic, - condition: { include: ["src"] }, - min_approvals: 1, - allowedToSkipRule: { minFellowsRank: 2 }, - teams: ["abc"], - }, - ], - }; - api.listModifiedFiles.mockResolvedValue(["src/polkadot/init.rs", "LICENSE"]); - fellowsApi.getTeamMembers.mockResolvedValue(["user-1"]); - api.getAuthor.mockReturnValue("user-1"); - const evaluation = await runner.validatePullRequest(config); - expect(evaluation).toBeTruthy(); - expect(logger.info).toHaveBeenCalledWith( - "Skipping rule Rule allowedToSkipRule as author belong to greenlight rule.", - ); - }); - test("fetchAllUsers should not return duplicates", async () => { teamsApi.getTeamMembers.mockResolvedValue(["user-1", "user-2", "user-3"]); const users = await runner.fetchAllUsers({ teams: ["abc"], users: ["user-1", "user-2", "user-4"] }); diff --git a/src/test/runner/validation/and.test.ts b/src/test/runner/validation/and.test.ts index c037743..34d6df4 100644 --- a/src/test/runner/validation/and.test.ts +++ b/src/test/runner/validation/and.test.ts @@ -172,72 +172,4 @@ describe("'And' rule validation", () => { expect(result.usersToRequest).toEqual(individualUsers); }); }); - - describe("rank tests", () => { - const config: ConfigurationFile = { - rules: [ - { - name: "And rule", - type: RuleTypes.And, - condition: { include: ["review-bot.yml"] }, - reviewers: [ - { minFellowsRank: 1, min_approvals: 1 }, - { minFellowsRank: 2, min_approvals: 1 }, - ], - }, - ], - }; - - test("should request rank equivalent to highest value", async () => { - fellowsApi.getTeamMembers.mockResolvedValue([users[2]]); - const { reports } = await runner.validatePullRequest(config); - const [result] = reports; - expect(result.missingReviews).toEqual(2); - expect(result.missingUsers).toEqual([users[2]]); - expect(result.missingRank).toEqual(2); - }); - - test("should request rank equivalent to highest value when one is fulfilled", async () => { - fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); - fellowsApi.getTeamMembers.calledWith("2").mockResolvedValue([users[2]]); - api.listApprovedReviewsAuthors.mockResolvedValue([users[1]]); - const { reports } = await runner.validatePullRequest(config); - const [result] = reports; - expect(result.missingReviews).toEqual(1); - expect(result.missingUsers).toEqual([users[2]]); - expect(result.missingRank).toEqual(2); - }); - - test("should accept higher rank for both cases", async () => { - fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); - fellowsApi.getTeamMembers.calledWith("2").mockResolvedValue([users[2]]); - api.listApprovedReviewsAuthors.mockResolvedValue([users[2]]); - const { reports } = await runner.validatePullRequest(config); - expect(reports).toHaveLength(0); - }); - - test("should request missing rank if highest rank is fulfilled", async () => { - fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); - fellowsApi.getTeamMembers.calledWith("2").mockResolvedValue([users[2]]); - api.listApprovedReviewsAuthors.mockResolvedValue([users[2]]); - - const { reports } = await runner.validatePullRequest({ - rules: [ - { - name: "And rule", - type: RuleTypes.And, - condition: { include: ["review-bot.yml"] }, - reviewers: [ - { minFellowsRank: 1, min_approvals: 2 }, - { minFellowsRank: 2, min_approvals: 1 }, - ], - }, - ], - }); - const [result] = reports; - expect(result.missingReviews).toEqual(1); - expect(result.missingUsers).toEqual([users[0], users[1]]); - expect(result.missingRank).toEqual(1); - }); - }); }); diff --git a/src/test/runner/validation/andDistinct.test.ts b/src/test/runner/validation/andDistinct.test.ts index 6d33663..35c1e4d 100644 --- a/src/test/runner/validation/andDistinct.test.ts +++ b/src/test/runner/validation/andDistinct.test.ts @@ -151,47 +151,6 @@ describe("'And distinct' rule validation", () => { const [result] = await runner.andDistinctEvaluation(rule); expect(result).toBe(false); }); - - describe("rank", () => { - test("should request highest missing rank", async () => { - const rule: AndDistinctRule = { - type: RuleTypes.AndDistinct, - countAuthor: true, - reviewers: [ - { minFellowsRank: 1, min_approvals: 1 }, - { minFellowsRank: 3, min_approvals: 1 }, - ], - name: "test", - condition: { include: [] }, - }; - api.listApprovedReviewsAuthors.mockResolvedValue(users); - fellowsApi.getTeamMembers.mockResolvedValue([users[1]]); - - const [result, report] = await runner.andDistinctEvaluation(rule); - expect(result).toBeFalsy(); - expect(report?.missingRank).toEqual(3); - }); - - test("should request highest missing rank even if lowest rank was fulfilled", async () => { - const rule: AndDistinctRule = { - type: RuleTypes.AndDistinct, - countAuthor: true, - reviewers: [ - { minFellowsRank: 1, min_approvals: 1 }, - { minFellowsRank: 3, min_approvals: 1 }, - ], - name: "test", - condition: { include: [] }, - }; - api.listApprovedReviewsAuthors.mockResolvedValue([users[1]]); - fellowsApi.getTeamMembers.calledWith("3").mockResolvedValue([users[1]]); - fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); - - const [result, report] = await runner.andDistinctEvaluation(rule); - expect(result).toBeFalsy(); - expect(report?.missingRank).toEqual(3); - }); - }); }); describe("Passing scenarios", () => { diff --git a/src/test/runner/validation/fellows.test.ts b/src/test/runner/validation/fellows.test.ts new file mode 100644 index 0000000..58bceda --- /dev/null +++ b/src/test/runner/validation/fellows.test.ts @@ -0,0 +1,122 @@ +import { mock, MockProxy } from "jest-mock-extended"; + +import { GitHubChecksApi } from "../../../github/check"; +import { PullRequestApi } from "../../../github/pullRequest"; +import { ActionLogger, TeamApi } from "../../../github/types"; +import { ConfigurationFile, RuleTypes } from "../../../rules/types"; +import { ActionRunner } from "../../../runner"; + +describe("'Fellows' rule validation", () => { + let api: MockProxy; + let teamsApi: MockProxy; + let fellowsApi: MockProxy; + let runner: ActionRunner; + const users = ["user-1", "user-2", "user-3"]; + beforeEach(() => { + api = mock(); + teamsApi = mock(); + fellowsApi = mock(); + teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); + api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); + api.listApprovedReviewsAuthors.mockResolvedValue([]); + runner = new ActionRunner(api, teamsApi, fellowsApi, mock(), mock()); + }); + + test("should not report errors when users from rank approved", async () => { + fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); + api.listApprovedReviewsAuthors.mockResolvedValue([users[0], users[1]]); + const { reports } = await runner.validatePullRequest({ + rules: [ + { + name: "Fellows rule", + type: RuleTypes.Fellows, + condition: { include: ["review-bot.yml"] }, + minRank: 1, + min_approvals: 2, + }, + ], + }); + expect(reports).toHaveLength(0); + }); + + test("should count author", async () => { + fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); + api.listApprovedReviewsAuthors.mockResolvedValue([users[0], users[1]]); + const { reports } = await runner.validatePullRequest({ + rules: [ + { + name: "Fellows rule", + type: RuleTypes.Fellows, + condition: { include: ["review-bot.yml"] }, + minRank: 1, + min_approvals: 1, + countAuthor: true, + }, + ], + }); + expect(reports).toHaveLength(0); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(api.listApprovedReviewsAuthors).toHaveBeenCalledWith(true); + }); + + describe("errors", () => { + test("should report users from missing rank", async () => { + fellowsApi.getTeamMembers.mockResolvedValue([users[2]]); + fellowsApi.getTeamMembers.calledWith("4").mockResolvedValue([users[0]]); + const { reports } = await runner.validatePullRequest({ + rules: [ + { + name: "Or rule", + type: RuleTypes.Fellows, + condition: { include: ["review-bot.yml"] }, + minRank: 4, + min_approvals: 1, + }, + ], + }); + const [result] = reports; + expect(result.missingReviews).toEqual(1); + expect(result.missingUsers).toEqual([users[2]]); + expect(result.missingRank).toEqual(4); + }); + + test("should throw error if no fellows of a given rank are found", async () => { + fellowsApi.getTeamMembers.calledWith("4").mockResolvedValue([]); + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Fellows, + condition: { include: ["review-bot.yml"] }, + minRank: 4, + min_approvals: 1, + }, + ], + }; + + await expect(runner.validatePullRequest(config)).rejects.toThrow( + `No users have been found with the rank ${4} or above`, + ); + }); + + test("should throw error if not enough fellows of a given rank are found to fulfill min_approvals requirement", async () => { + fellowsApi.getTeamMembers.mockResolvedValue([users[2]]); + fellowsApi.getTeamMembers.calledWith("4").mockResolvedValue(users); + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Fellows, + condition: { include: ["review-bot.yml"] }, + minRank: 4, + min_approvals: 5, + }, + ], + }; + + await expect(runner.validatePullRequest(config)).rejects.toThrow( + "The amount of required approvals is smaller than the amount of available users.", + ); + }); + }); +}); diff --git a/src/test/runner/validation/or.test.ts b/src/test/runner/validation/or.test.ts index 9837769..f4e86ae 100644 --- a/src/test/runner/validation/or.test.ts +++ b/src/test/runner/validation/or.test.ts @@ -78,94 +78,53 @@ describe("'Or' rule validation", () => { expect(reports).toHaveLength(0); }); - test("should accept lowest rank for both cases", async () => { - fellowsApi.getTeamMembers.calledWith("1").mockResolvedValue(users); - fellowsApi.getTeamMembers.calledWith("2").mockResolvedValue([users[2]]); - api.listApprovedReviewsAuthors.mockResolvedValue([users[0]]); - const { reports } = await runner.validatePullRequest({ - rules: [ - { - name: "Or rule", - type: RuleTypes.Or, - condition: { include: ["review-bot.yml"] }, - reviewers: [ - { minFellowsRank: 1, min_approvals: 1 }, - { minFellowsRank: 2, min_approvals: 1 }, - ], - }, - ], - }); - expect(reports).toHaveLength(0); - }); - }); + describe("errors", () => { + test("should report all missing individual users if all of the rules have not been met", async () => { + const individualUsers = [users[0], users[1]]; - describe("errors", () => { - test("should report all missing individual users if all of the rules have not been met", async () => { - const individualUsers = [users[0], users[1]]; - - const config: ConfigurationFile = { - rules: [ - { - name: "Or rule", - type: RuleTypes.Or, - condition: { include: ["review-bot.yml"] }, - reviewers: [ - { teams: ["abc"], min_approvals: 1 }, - { users: individualUsers, min_approvals: 2 }, - ], - }, - ], - }; - api.listApprovedReviewsAuthors.mockResolvedValue([]); - const { reports } = await runner.validatePullRequest(config); - const [result] = reports; - expect(result.missingReviews).toEqual(1); - expect(result.missingUsers).toEqual(users); - expect(result.teamsToRequest).toContainEqual("abc"); - expect(result.usersToRequest).toEqual(individualUsers); - }); - - test("should show the lowest amount of reviews needed to fulfill the rule", async () => { - const config: ConfigurationFile = { - rules: [ - { - name: "Or rule", - type: RuleTypes.Or, - condition: { include: ["review-bot.yml"] }, - reviewers: [ - { users: ["abc"], min_approvals: 1 }, - { users: ["bcd", "cef"], min_approvals: 2 }, - { users: ["bds", "cj9", "dij", "ehu"], min_approvals: 4 }, - { users: ["bob", "cat", "dpo", "eio", "fgy"], min_approvals: 5 }, - ], - }, - ], - }; - api.listApprovedReviewsAuthors.mockResolvedValue([]); - const { reports } = await runner.validatePullRequest(config); - const [result] = reports; - expect(result.missingReviews).toEqual(1); - }); + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [ + { teams: ["abc"], min_approvals: 1 }, + { users: individualUsers, min_approvals: 2 }, + ], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([]); + const { reports } = await runner.validatePullRequest(config); + const [result] = reports; + expect(result.missingReviews).toEqual(1); + expect(result.missingUsers).toEqual(users); + expect(result.teamsToRequest).toContainEqual("abc"); + expect(result.usersToRequest).toEqual(individualUsers); + }); - test("should request lowest rank", async () => { - fellowsApi.getTeamMembers.mockResolvedValue([users[2]]); - const { reports } = await runner.validatePullRequest({ - rules: [ - { - name: "Or rule", - type: RuleTypes.Or, - condition: { include: ["review-bot.yml"] }, - reviewers: [ - { minFellowsRank: 1, min_approvals: 1 }, - { minFellowsRank: 2, min_approvals: 1 }, - ], - }, - ], + test("should show the lowest amount of reviews needed to fulfill the rule", async () => { + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [ + { users: ["abc"], min_approvals: 1 }, + { users: ["bcd", "cef"], min_approvals: 2 }, + { users: ["bds", "cj9", "dij", "ehu"], min_approvals: 4 }, + { users: ["bob", "cat", "dpo", "eio", "fgy"], min_approvals: 5 }, + ], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([]); + const { reports } = await runner.validatePullRequest(config); + const [result] = reports; + expect(result.missingReviews).toEqual(1); }); - const [result] = reports; - expect(result.missingReviews).toEqual(1); - expect(result.missingUsers).toEqual([users[2]]); - expect(result.missingRank).toEqual(1); }); }); });