diff --git a/README.md b/README.md index 5032e7a..f6f536c 100644 --- a/README.md +++ b/README.md @@ -411,21 +411,37 @@ Meaning that if a user belongs to both `team-abc` and `team-example` their appro 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: +The fellows rule requires an extra collection to be added to the configuration and distinguishes itself from the other rules with some new fields: ```yaml -- name: Fellows review - condition: - include: - - '.*' - exclude: - - 'example' - type: fellows - minRank: 2 - minApprovals: 2 +rules: + - name: Fellows review + condition: + include: + - '.*' + exclude: + - 'example' + type: fellows + minRank: 2 + minApprovals: 2 + minTotalScore: 4 +scores: + dan1: 1 + dan2: 2 + dan3: 4 + dan4: 8 + dan5: 12 + dan6: 15 + dan7: 20 + dan8: 25 + dan9: 30 ``` -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. +The biggest difference in the rule configuration is that it doesn’t have a reviewers type (it doesn’t have a `teams` or `users` field); instead, it has a `minRank` field and `minTotalScore` field. + +The `minRank` 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). + +The `minTotalScore` field relies on the `scores` collection. In this collection, each rank (dan) receives a score, so, in the example, a fellow dan 3 will have a score of 4. If the field `minTotalScore` is enabled, the conditions to approve a pull request, aside from requiring a given amount of reviews from a given rank, will be that _the total sum of all the scores from the fellows that have approved the pull request are equal or greater to the `minTotalScore` field_. This field is optional. -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). +For *every dan that has not been attributed a score, their score will default to 0*. After this is done, the resulting handles will be treated like a normal list of required users. @@ -439,6 +455,8 @@ It also has any other field from the [`basic rule`](#basic-rule) (with the excep - **Optional**: Defaults to `false`. - **minRank**: Must be a number. - **Required** +- **minTotalScore**: Must be a number + - **Optional**: Defaults to 0. ##### Note The fellows rule will never request reviewers, even if `request-reviewers` rule is enabled. diff --git a/action.yml b/action.yml index 9689a0a..48e8aa9 100644 --- a/action.yml +++ b/action.yml @@ -32,4 +32,4 @@ outputs: runs: using: 'docker' - image: 'Dockerfile' + image: 'docker://ghcr.io/paritytech/review-bot/action:2.4.0' diff --git a/package.json b/package.json index 0672e8b..43cd0c8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "review-bot", - "version": "2.3.1", + "version": "2.4.0", "description": "Have custom review rules for PRs with auto assignment", "main": "src/index.ts", "scripts": { @@ -39,7 +39,7 @@ "@actions/github": "^6.0.0", "@eng-automation/js": "^1.0.2", "@polkadot/api": "^10.11.2", - "joi": "^17.12.0", + "joi": "^17.6.4", "yaml": "^2.3.4" } } diff --git a/src/failures/fellowsRules.ts b/src/failures/fellowsRules.ts index 623d399..52412c9 100644 --- a/src/failures/fellowsRules.ts +++ b/src/failures/fellowsRules.ts @@ -42,3 +42,64 @@ export class FellowMissingRankFailure extends ReviewFailure { return { users: [], teams: [] }; } } + +export class FellowMissingScoreFailure extends ReviewFailure { + public readonly currentScore: number; + constructor( + ruleInfo: Omit, + public readonly requiredScore: number, + approvalsWithScores: [string, number][], + missingFellowsWithScore: [string, number][], + ) { + const unifyFellowWithScore = ([handle, score]: [string, number]) => `${handle} -> ${score}`; + super({ + ...ruleInfo, + countingReviews: approvalsWithScores.map(unifyFellowWithScore), + missingUsers: missingFellowsWithScore.map(unifyFellowWithScore), + missingReviews: 1, + }); + + this.currentScore = approvalsWithScores.reduce((n, [_, score]) => n + score, 0); + } + + generateSummary(): typeof summary { + let text = summary + .emptyBuffer() + .addHeading(this.name, 2) + .addHeading("Missing minimum required score from Fellows", 4) + .addDetails( + "Rule explanation", + "Rule 'Fellows' gives every fellow a score based on their rank, and required that the sum of all the scores is greater than the required score." + + "\n\n" + + "For more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)", + ); + + text = text + .addHeading(`Missing a score of ${this.requiredScore}`, 3) + .addEOL() + .addRaw(`Missing ${this.requiredScore - this.currentScore} in the required score.`) + .addEOL() + .addRaw(`Current score is ${this.currentScore}/${this.requiredScore}`); + if (this.missingUsers.length > 0) + text = text.addDetails( + "GitHub users whose approval counts", + `This is a list of all the Fellows that have not reviewed with their current scores:\n\n - ${this.missingUsers + .map(toHandle) + .join("\n - ")}`, + ); + + if (this.countingReviews.length > 0) { + text = text + .addHeading("Users approvals that counted towards this rule with their scores", 3) + .addEOL() + .addList(this.countingReviews.map(toHandle)) + .addEOL(); + } + + return text; + } + + getRequestLogins(): { users: string[]; teams: string[] } { + return { users: [], teams: [] }; + } +} diff --git a/src/polkadot/fellows.ts b/src/polkadot/fellows.ts index 81b5fd7..c17f270 100644 --- a/src/polkadot/fellows.ts +++ b/src/polkadot/fellows.ts @@ -10,7 +10,7 @@ export class PolkadotFellows implements TeamApi { constructor(private readonly logger: ActionLogger) {} - async fetchAllFellows(): Promise> { + private async fetchAllFellows(): Promise> { let api: ApiPromise; this.logger.debug("Connecting to collective parachain"); // we connect to the collective rpc node @@ -91,6 +91,18 @@ export class PolkadotFellows implements TeamApi { } } + /** Returns all the fellows with their rankings */ + async listFellows(): Promise<[string, number][]> { + this.logger.info("Fetching all fellows with their ranks"); + + if (this.fellowsCache.size < 1) { + this.logger.debug("Cache not found. Fetching fellows."); + this.fellowsCache = await this.fetchAllFellows(); + } + + return Array.from(this.fellowsCache.entries()); + } + async getTeamMembers(ranking: string): Promise { const requiredRank = Number(ranking); this.logger.info(`Fetching members of rank '${requiredRank}' or higher`); diff --git a/src/rules/types.ts b/src/rules/types.ts index a69cfb6..e9d133f 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -38,6 +38,19 @@ export interface FellowsRule extends Rule { type: RuleTypes.Fellows; minRank: number; minApprovals: number; + minTotalScore?: number; +} + +export interface FellowsScore { + dan1: number; + dan2: number; + dan3: number; + dan4: number; + dan5: number; + dan6: number; + dan7: number; + dan8: number; + dan9: number; } export interface ConfigurationFile { @@ -49,4 +62,5 @@ export interface ConfigurationFile { teams?: string[]; users?: string[]; }; + score?: FellowsScore; } diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 59ad233..cdc38ce 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, FellowsRule, Reviewers, Rule, RuleTypes } from "./types"; +import { AndRule, BasicRule, ConfigurationFile, FellowsRule, FellowsScore, 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 @@ -30,6 +30,19 @@ const ruleSchema = Joi.object().keys({ .required(), }); +/** Schema for ensuring that all the dan ranks are set properly */ +export const fellowScoreSchema = Joi.object().keys({ + dan1: Joi.number().default(0), + dan2: Joi.number().default(0), + dan3: Joi.number().default(0), + dan4: Joi.number().default(0), + dan5: Joi.number().default(0), + dan6: Joi.number().default(0), + dan7: Joi.number().default(0), + dan8: Joi.number().default(0), + dan9: Joi.number().default(0), +}); + /** General Configuration schema. * Evaluates all the upper level field plus the generic rules fields. * Remember to evaluate the rules with their custom rules @@ -37,6 +50,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"), + score: fellowScoreSchema, }); /** Basic rule schema @@ -59,6 +73,7 @@ export const fellowsRuleSchema = Joi.object().keys({ countAuthor: Joi.boolean().default(false), minRank: Joi.number().required().min(1).empty(null), minApprovals: Joi.number().min(1).default(1), + minTotalScore: Joi.number().min(1).optional(), }); /** diff --git a/src/runner.ts b/src/runner.ts index 387feb9..6c39a7c 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -5,6 +5,7 @@ import { Inputs } from "."; import { CommonRuleFailure, FellowMissingRankFailure, + FellowMissingScoreFailure, RequiredReviewersData, ReviewFailure, RuleFailedReport, @@ -13,9 +14,18 @@ import { 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 { PolkadotFellows } from "./polkadot/fellows"; +import { + AndDistinctRule, + ConfigurationFile, + FellowsRule, + FellowsScore, + Reviewers, + Rule, + RuleTypes, +} from "./rules/types"; import { validateConfig, validateRegularExpressions } from "./rules/validator"; -import { concatArraysUniquely } from "./util"; +import { concatArraysUniquely, rankToScore } from "./util"; type BaseRuleReport = RuleFailedReport & RequiredReviewersData; @@ -31,7 +41,7 @@ export class ActionRunner { constructor( private readonly prApi: PullRequestApi, private readonly teamApi: TeamApi, - private readonly polkadotApi: TeamApi, + private readonly polkadotApi: PolkadotFellows, private readonly checks: GitHubChecksApi, private readonly logger: ActionLogger, ) {} @@ -61,7 +71,7 @@ export class ActionRunner { * The action evaluates if the rules requirements are meet for a PR * @returns an array of error reports for each failed rule. An empty array means no errors */ - async validatePullRequest({ rules }: ConfigurationFile): Promise { + async validatePullRequest({ rules, score }: ConfigurationFile): Promise { const modifiedFiles = await this.prApi.listModifiedFiles(); const errorReports: ReviewFailure[] = []; @@ -150,7 +160,7 @@ export class ActionRunner { break; } case RuleTypes.Fellows: { - const fellowReviewError = await this.fellowsEvaluation(rule); + const fellowReviewError = await this.fellowsEvaluation(rule, score); if (fellowReviewError) { this.logger.error(`Missing the reviews from ${JSON.stringify(fellowReviewError.missingReviews)}`); // errorReports.push({ ...missingData, name: rule.name, type: rule.type }); @@ -440,7 +450,7 @@ export class ActionRunner { } } - async fellowsEvaluation(rule: FellowsRule): Promise { + async fellowsEvaluation(rule: FellowsRule, scores?: FellowsScore): 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()); @@ -472,9 +482,10 @@ export class ActionRunner { } } + const author = this.prApi.getAuthor(); + // 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 @@ -487,11 +498,54 @@ export class ActionRunner { }, rule.minRank, ); - } else { - this.logger.info("Rule requirements fulfilled"); - // If we don't have any missing reviews, we return no error - return null; + // Then we verify if we need to have a minimum score + } else if (rule.minTotalScore && scores) { + this.logger.debug("Validating required minimum score"); + // We get all the fellows with their ranks and convert them to their score + const fellows: [string, number][] = (await this.polkadotApi.listFellows()).map(([handle, rank]) => [ + handle, + rankToScore(rank, scores), + ]); + + const maximumScore = fellows.reduce((a, [_, score]) => a + score, 0); + if (rule.minTotalScore > maximumScore) { + throw new Error( + `Minimum score of ${rule.minTotalScore} is higher that the obtainable score of ${maximumScore}!`, + ); + } + + let score = 0; + + const countingFellows: [string, number][] = []; + + // We iterate over all the approvals and convert their rank to their score + for (const [handle, fellowScore] of fellows) { + // We filter fellows whose score is 0 + if (approvals.indexOf(handle) > -1 && fellowScore > 0) { + score += fellowScore; + countingFellows.push([handle, fellowScore]); + } + } + + this.logger.debug(`Current score is ${score} and the minimum required score is ${rule.minTotalScore}`); + + if (rule.minTotalScore > score) { + const missingUsers = fellows + // Remove all the fellows who score is worth 0 + .filter(([_, fellowScore]) => fellowScore > 0) + // Remove the author + .filter(([handle]) => handle != author) + // Remove the approvals + .filter(([handle]) => approvals.indexOf(handle) < 0); + + this.logger.warn(`Missing score of ${rule.minTotalScore} by ${score - rule.minTotalScore}`); + + return new FellowMissingScoreFailure(rule, rule.minTotalScore, countingFellows, missingUsers); + } } + this.logger.info("Rule requirements fulfilled"); + // If we don't have any missing reviews, we return no error + return null; } /** Using the include and exclude condition, it returns a list of all the files in a PR that matches the criteria */ diff --git a/src/test/fellows.test.ts b/src/test/fellows.test.ts index ffbc2a0..b9bb1a8 100644 --- a/src/test/fellows.test.ts +++ b/src/test/fellows.test.ts @@ -4,7 +4,7 @@ import { mock, mockClear, MockProxy } from "jest-mock-extended"; import { ActionLogger, TeamApi } from "../github/types"; import { PolkadotFellows } from "../polkadot/fellows"; -const timeout = 15_000; +const timeout = 25_000; describe("CAPI test", () => { let fellows: TeamApi; diff --git a/src/test/index.test.ts b/src/test/index.test.ts index 304e8ab..9ee95c5 100644 --- a/src/test/index.test.ts +++ b/src/test/index.test.ts @@ -9,6 +9,7 @@ import { GitHubChecksApi } from "../github/check"; import { PullRequestApi } from "../github/pullRequest"; import { GitHubTeamsApi } from "../github/teams"; import { ActionLogger, GitHubClient, TeamApi } from "../github/types"; +import { PolkadotFellows } from "../polkadot/fellows"; import { ActionRunner } from "../runner"; type ReportName = @@ -83,7 +84,7 @@ describe("Integration testing", () => { api = new PullRequestApi(client, pr, logger); teams = new GitHubTeamsApi(client, "org", logger); checks = new GitHubChecksApi(client, pr, logger, "example"); - runner = new ActionRunner(api, teams, mock(), checks, logger); + runner = new ActionRunner(api, teams, mock(), checks, logger); // @ts-ignore problem with the type being mocked client.rest.repos.getContent.mockResolvedValue({ data: { content: Buffer.from(config, "utf-8") } }); diff --git a/src/test/rules/andDistinct.test.ts b/src/test/rules/andDistinct.test.ts index 9e3fb72..2339e9d 100644 --- a/src/test/rules/andDistinct.test.ts +++ b/src/test/rules/andDistinct.test.ts @@ -5,17 +5,18 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; -import { ActionLogger, TeamApi } from "../../github/types"; +import { ActionLogger } from "../../github/types"; +import { PolkadotFellows } from "../../polkadot/fellows"; import { AndDistinctRule } from "../../rules/types"; import { ActionRunner } from "../../runner"; describe("'AndDistinctRule' rule parsing", () => { let api: MockProxy; let runner: ActionRunner; - let teamsApi: MockProxy; + let teamsApi: MockProxy; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); + runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/rules/andRule.test.ts b/src/test/rules/andRule.test.ts index 03bf5bf..c9c92e4 100644 --- a/src/test/rules/andRule.test.ts +++ b/src/test/rules/andRule.test.ts @@ -5,17 +5,18 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; -import { ActionLogger, TeamApi } from "../../github/types"; +import { ActionLogger } from "../../github/types"; +import { PolkadotFellows } from "../../polkadot/fellows"; import { AndRule } from "../../rules/types"; import { ActionRunner } from "../../runner"; describe("'And' rule parsing", () => { let api: MockProxy; let runner: ActionRunner; - let teamsApi: MockProxy; + let teamsApi: MockProxy; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); + runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/rules/basicRule.test.ts b/src/test/rules/basicRule.test.ts index b2408e3..0c8fedc 100644 --- a/src/test/rules/basicRule.test.ts +++ b/src/test/rules/basicRule.test.ts @@ -5,17 +5,18 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; -import { ActionLogger, TeamApi } from "../../github/types"; +import { ActionLogger } from "../../github/types"; +import { PolkadotFellows } from "../../polkadot/fellows"; import { BasicRule } from "../../rules/types"; import { ActionRunner } from "../../runner"; describe("Basic rule parsing", () => { let api: MockProxy; let runner: ActionRunner; - let teamsApi: MockProxy; + let teamsApi: MockProxy; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); + runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index f4d372a..123092a 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -6,6 +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 { PolkadotFellows } from "../../polkadot/fellows"; import { RuleTypes } from "../../rules/types"; import { ActionRunner } from "../../runner"; @@ -17,7 +18,7 @@ describe("Config Parsing", () => { beforeEach(() => { logger = mock(); api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock(), logger); + runner = new ActionRunner(api, teamsApi, mock(), mock(), logger); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/rules/fellows.test.ts b/src/test/rules/fellows.test.ts index 7e34f4c..5408726 100644 --- a/src/test/rules/fellows.test.ts +++ b/src/test/rules/fellows.test.ts @@ -6,6 +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 { PolkadotFellows } from "../../polkadot/fellows"; import { FellowsRule } from "../../rules/types"; import { ActionRunner } from "../../runner"; @@ -14,7 +15,13 @@ describe("Fellows rule parsing", () => { let runner: ActionRunner; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, mock(), mock(), mock(), mock()); + runner = new ActionRunner( + api, + mock(), + mock(), + mock(), + mock(), + ); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/rules/or.test.ts b/src/test/rules/or.test.ts index 8fabba5..451dfd0 100644 --- a/src/test/rules/or.test.ts +++ b/src/test/rules/or.test.ts @@ -6,6 +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 { PolkadotFellows } from "../../polkadot/fellows"; import { OrRule } from "../../rules/types"; import { ActionRunner } from "../../runner"; @@ -15,7 +16,7 @@ describe("'Or' rule parsing", () => { let teamsApi: MockProxy; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); + runner = new ActionRunner(api, teamsApi, mock(), mock(), mock()); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/runner/conditions.test.ts b/src/test/runner/conditions.test.ts index d9a2a10..200a144 100644 --- a/src/test/runner/conditions.test.ts +++ b/src/test/runner/conditions.test.ts @@ -3,18 +3,19 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../github/types"; +import { PolkadotFellows } from "../../polkadot/fellows"; import { ActionRunner } from "../../runner"; describe("evaluateCondition tests", () => { let api: MockProxy; let teamsApi: MockProxy; - let fellowsApi: MockProxy; + let fellowsApi: MockProxy; let runner: ActionRunner; beforeEach(() => { api = mock(); teamsApi = mock(); - fellowsApi = mock(); + fellowsApi = mock(); runner = new ActionRunner(api, teamsApi, fellowsApi, mock(), mock()); }); diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index e1dca23..d6d18d8 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -5,20 +5,21 @@ import { ReviewFailure } from "../../failures"; import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../github/types"; +import { PolkadotFellows } from "../../polkadot/fellows"; import { ConfigurationFile, Rule, RuleTypes } from "../../rules/types"; import { ActionRunner } from "../../runner"; describe("Shared validations", () => { let api: MockProxy; let teamsApi: MockProxy; - let fellowsApi: MockProxy; + let fellowsApi: MockProxy; let logger: MockProxy; let runner: ActionRunner; beforeEach(() => { api = mock(); logger = mock(); teamsApi = mock(); - fellowsApi = mock(); + fellowsApi = mock(); runner = new ActionRunner(api, teamsApi, fellowsApi, mock(), logger); }); diff --git a/src/test/runner/validation/and.test.ts b/src/test/runner/validation/and.test.ts index 8f4c77b..808a87f 100644 --- a/src/test/runner/validation/and.test.ts +++ b/src/test/runner/validation/and.test.ts @@ -3,19 +3,20 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../../github/check"; import { PullRequestApi } from "../../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../../github/types"; +import { PolkadotFellows } from "../../../polkadot/fellows"; import { ConfigurationFile, RuleTypes } from "../../../rules/types"; import { ActionRunner } from "../../../runner"; describe("'And' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; - let fellowsApi: MockProxy; + let fellowsApi: MockProxy; let runner: ActionRunner; const users = ["user-1", "user-2", "user-3"]; beforeEach(() => { api = mock(); teamsApi = mock(); - fellowsApi = mock(); + fellowsApi = mock(); teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); diff --git a/src/test/runner/validation/andDistinct.test.ts b/src/test/runner/validation/andDistinct.test.ts index 4eb278b..7804c5c 100644 --- a/src/test/runner/validation/andDistinct.test.ts +++ b/src/test/runner/validation/andDistinct.test.ts @@ -4,13 +4,14 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../../github/check"; import { PullRequestApi } from "../../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../../github/types"; +import { PolkadotFellows } from "../../../polkadot/fellows"; import { AndDistinctRule, RuleTypes } from "../../../rules/types"; import { ActionRunner } from "../../../runner"; describe("'And distinct' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; - let fellowsApi: MockProxy; + let fellowsApi: MockProxy; let runner: ActionRunner; let logger: MockProxy; const users = ["user-1", "user-2", "user-3"]; @@ -18,7 +19,7 @@ describe("'And distinct' rule validation", () => { logger = mock(); api = mock(); teamsApi = mock(); - fellowsApi = mock(); + fellowsApi = mock(); teamsApi.getTeamMembers.calledWith("team-abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); diff --git a/src/test/runner/validation/fellows.test.ts b/src/test/runner/validation/fellows.test.ts index 26355c2..25c6b51 100644 --- a/src/test/runner/validation/fellows.test.ts +++ b/src/test/runner/validation/fellows.test.ts @@ -1,22 +1,25 @@ +import { validate } from "@eng-automation/js"; import { mock, MockProxy } from "jest-mock-extended"; -import { FellowMissingRankFailure } from "../../../failures"; +import { FellowMissingRankFailure, FellowMissingScoreFailure } from "../../../failures"; import { GitHubChecksApi } from "../../../github/check"; import { PullRequestApi } from "../../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../../github/types"; -import { ConfigurationFile, RuleTypes } from "../../../rules/types"; +import { PolkadotFellows } from "../../../polkadot/fellows"; +import { ConfigurationFile, FellowsScore, RuleTypes } from "../../../rules/types"; +import { fellowScoreSchema } from "../../../rules/validator"; import { ActionRunner } from "../../../runner"; describe("'Fellows' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; - let fellowsApi: MockProxy; + let fellowsApi: MockProxy; let runner: ActionRunner; const users = ["user-1", "user-2", "user-3"]; beforeEach(() => { api = mock(); teamsApi = mock(); - fellowsApi = mock(); + fellowsApi = mock(); teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); @@ -102,7 +105,6 @@ describe("'Fellows' rule validation", () => { test("should throw error if not enough fellows of a given rank are found to fulfill minApprovals requirement", async () => { fellowsApi.getTeamMembers.mockResolvedValue([users[2]]); - fellowsApi.getTeamMembers.calledWith("4").mockResolvedValue(users); const config: ConfigurationFile = { rules: [ { @@ -120,4 +122,100 @@ describe("'Fellows' rule validation", () => { ); }); }); + + describe("Score Validation", () => { + beforeEach(() => { + fellowsApi.getTeamMembers.mockResolvedValue([users[2]]); + api.listApprovedReviewsAuthors.mockResolvedValue(users); + }); + + const generateSchemaWithScore = (minTotalScore: number): ConfigurationFile => { + return { + rules: [ + { + name: "Score rule", + type: RuleTypes.Fellows, + condition: { include: ["review-bot.yml"] }, + minRank: 1, + minApprovals: 1, + minTotalScore, + }, + ], + score: { + dan1: 1, + dan2: 2, + dan3: 3, + dan4: 4, + dan5: 5, + dan6: 6, + dan7: 7, + dan8: 8, + dan9: 9, + }, + }; + }; + + describe("Schema test", () => { + test("should not report errors with a valid schema", () => { + const score = {}; + validate(score, fellowScoreSchema); + }); + + test("should assign correct values", () => { + const score = { dan1: 3, dan3: 5 }; + const validation: FellowsScore = validate(score, fellowScoreSchema); + expect(validation.dan1).toBe(3); + expect(validation.dan3).toBe(5); + }); + + test("should default unassigned values as 0", () => { + const score = { dan1: 3 }; + const validation: FellowsScore = validate(score, fellowScoreSchema); + expect(validation.dan2).toBe(0); + expect(validation.dan5).toBe(0); + }); + + test("should fail when a score is not a number", () => { + const score = { dan1: "one" }; + expect(() => validate(score, fellowScoreSchema)).toThrowError('"dan1" must be a number'); + }); + }); + + test("should work with enough score", async () => { + fellowsApi.listFellows.mockResolvedValue([[users[2], 4]]); + + const { reports } = await runner.validatePullRequest(generateSchemaWithScore(1)); + expect(reports).toHaveLength(0); + }); + + test("should fail without enough score", async () => { + fellowsApi.listFellows.mockResolvedValue([ + [users[2], 4], + ["example", 3], + ["user", 2], + ]); + + const { reports } = await runner.validatePullRequest(generateSchemaWithScore(5)); + const error = reports[0] as FellowMissingScoreFailure; + expect(error.currentScore).toBeLessThan(5); + }); + + test("should throw when score is impossible", async () => { + fellowsApi.listFellows.mockResolvedValue([[users[2], 4]]); + + await expect(runner.validatePullRequest(generateSchemaWithScore(10))).rejects.toThrow( + "Minimum score of 10 is higher that the obtainable score of 4!", + ); + }); + + test("should allow a combination of scores", async () => { + fellowsApi.listFellows.mockResolvedValue([ + [users[0], 4], + [users[1], 1], + ]); + + const { reports } = await runner.validatePullRequest(generateSchemaWithScore(1)); + expect(reports).toHaveLength(0); + }); + }); }); diff --git a/src/test/runner/validation/or.test.ts b/src/test/runner/validation/or.test.ts index da919ba..43b6d6d 100644 --- a/src/test/runner/validation/or.test.ts +++ b/src/test/runner/validation/or.test.ts @@ -3,19 +3,20 @@ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../../github/check"; import { PullRequestApi } from "../../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../../github/types"; +import { PolkadotFellows } from "../../../polkadot/fellows"; import { ConfigurationFile, RuleTypes } from "../../../rules/types"; import { ActionRunner } from "../../../runner"; describe("'Or' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; - let fellowsApi: MockProxy; + let fellowsApi: MockProxy; let runner: ActionRunner; const users = ["user-1", "user-2", "user-3"]; beforeEach(() => { api = mock(); teamsApi = mock(); - fellowsApi = mock(); + fellowsApi = mock(); teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); diff --git a/src/util.ts b/src/util.ts index 54be919..98f195c 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1,6 +1,7 @@ import { debug, error, info, warning } from "@actions/core"; import { ActionLogger } from "./github/types"; +import { FellowsScore } from "./rules/types"; export function generateCoreLogger(): ActionLogger { return { info, debug, warn: warning, error }; @@ -26,3 +27,29 @@ export function caseInsensitiveEqual(a: T, b: T): boolean { * @param handle The username */ export const toHandle = (handle: string): string => `@${handle}`; + +/** Converts a rank into its value inside the score configuration */ +export function rankToScore(rank: number, scores: FellowsScore): number { + switch (rank) { + case 1: + return scores.dan1; + case 2: + return scores.dan2; + case 3: + return scores.dan3; + case 4: + return scores.dan4; + case 5: + return scores.dan5; + case 6: + return scores.dan6; + case 7: + return scores.dan7; + case 8: + return scores.dan8; + case 9: + return scores.dan9; + default: + throw new Error(`Rank ${rank} is out of bounds. Ranks are between I and IX`); + } +} diff --git a/yarn.lock b/yarn.lock index 18d26af..b1ad8eb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -474,12 +474,12 @@ resolved "https://registry.yarnpkg.com/@fastify/busboy/-/busboy-2.0.0.tgz#f22824caff3ae506b18207bad4126dbc6ccdb6b8" integrity sha512-JUFJad5lv7jxj926GPgymrWQxxjPYuJNiNjNMzqT+HiuP6Vl3dk5xzG+8sTX96np0ZAluvaMzPsjhHZ5rNuNQQ== -"@hapi/hoek@^9.0.0", "@hapi/hoek@^9.3.0": +"@hapi/hoek@^9.0.0": version "9.3.0" resolved "https://registry.yarnpkg.com/@hapi/hoek/-/hoek-9.3.0.tgz#8368869dcb735be2e7f5cb7647de78e167a251fb" integrity sha512-/c6rf4UJlmHlC9b5BaNvzAcFv7HZ2QHaV0D4/HNlBdvFnvQq8RI4kYdhyPCl7Xj+oWvTWQ8ujhqS53LIgAe6KQ== -"@hapi/topo@^5.0.0", "@hapi/topo@^5.1.0": +"@hapi/topo@^5.0.0": version "5.1.0" resolved "https://registry.yarnpkg.com/@hapi/topo/-/topo-5.1.0.tgz#dc448e332c6c6e37a4dc02fd84ba8d44b9afb012" integrity sha512-foQZKJig7Ob0BMAYBfcJk8d77QtOe7Wo4ox7ff1lQYoNNAb6jwcY1ncdoy2e9wQZzvNy7ODZCYJkK8kzmcAnAg== @@ -1243,7 +1243,7 @@ resolved "https://registry.yarnpkg.com/@scure/base/-/base-1.1.5.tgz#1d85d17269fe97694b9c592552dd9e5e33552157" integrity sha512-Brj9FiG2W1MRQSTB212YVPRrcbjkv48FoZi/u4l/zds/ieRrqsh7aUf6CLwkAq61oKXr/ZlTzlY66gLIj3TFTQ== -"@sideway/address@^4.1.3", "@sideway/address@^4.1.4": +"@sideway/address@^4.1.3": version "4.1.4" resolved "https://registry.yarnpkg.com/@sideway/address/-/address-4.1.4.tgz#03dccebc6ea47fdc226f7d3d1ad512955d4783f0" integrity sha512-7vwq+rOHVWjyXxVlR76Agnvhy8I9rpzjosTESvmhNeXOXdZZB15Fl+TI9x1SiHZH5Jv2wTGduSxFDIaq0m3DUw== @@ -3456,17 +3456,6 @@ joi-to-typescript@^4.0.5: resolved "https://registry.yarnpkg.com/joi-to-typescript/-/joi-to-typescript-4.4.1.tgz#9f84e94bba78c8b3dd3dfade9408ca2616f6d678" integrity sha512-5RBSFooYrnZCSrisA2g1yFlbTnBGLs724V8BfjtlcxYMyNEQbCfsJOIcCenXo+01h5UBHUbD+42fHGOJo5nK/w== -joi@^17.12.0: - version "17.12.0" - resolved "https://registry.yarnpkg.com/joi/-/joi-17.12.0.tgz#a3fb5715f198beb0471cd551dd26792089c308d5" - integrity sha512-HSLsmSmXz+PV9PYoi3p7cgIbj06WnEBNT28n+bbBNcPZXZFqCzzvGqpTBPujx/Z0nh1+KNQPDrNgdmQ8dq0qYw== - dependencies: - "@hapi/hoek" "^9.3.0" - "@hapi/topo" "^5.1.0" - "@sideway/address" "^4.1.4" - "@sideway/formula" "^3.0.1" - "@sideway/pinpoint" "^2.0.0" - joi@^17.6.4: version "17.9.2" resolved "https://registry.yarnpkg.com/joi/-/joi-17.9.2.tgz#8b2e4724188369f55451aebd1d0b1d9482470690"