Skip to content

Commit

Permalink
Added better rank report and testing (#82)
Browse files Browse the repository at this point in the history 
Enhanced the check report, adding the information of the missing rank.
This resolves #80

Added tests that evaluate the ranking system, and how it applies the
rules. This resolves #81

For the `or` rule, we request the lowest required rank, and for the
`and` rule we request the highest rank.

In the case of `and-distinct` we request the highest rank.
  • Loading branch information
@Bullrich
Bullrich authored Sep 16, 2023
1 parent 29b16b5 commit 088b8b9
Show file tree
Hide file tree
Showing 5 changed files with 219 additions and 12 deletions.
36 changes: 28 additions & 8 deletions src/runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ type ReviewReport = {
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;
};

export type RuleReport = { name: string } & ReviewReport;
Expand Down Expand Up @@ -129,10 +131,13 @@ export class ActionRunner {
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);
Expand Down Expand Up @@ -234,6 +239,9 @@ export class ActionRunner {
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 of rank ${report.missingRank} or above`, 3);
}

check.output.text += text.stringify() + "\n";
}
Expand All @@ -251,14 +259,8 @@ export class ActionRunner {
const requirements: { users: string[]; requiredApprovals: number }[] = [];
// We get all the users belonging to each 'and distinct' review condition
for (const reviewers of rule.reviewers) {
let usersToAdd: string[] = reviewers.users ?? [];
if (reviewers.teams) {
for (const team of reviewers.teams) {
const members = await this.teamApi.getTeamMembers(team);
usersToAdd = [...new Set([...usersToAdd, ...members])];
}
}
requirements.push({ users: usersToAdd, requiredApprovals: reviewers.min_approvals });
const users = await this.fetchAllUsers(reviewers);
requirements.push({ users, requiredApprovals: reviewers.min_approvals });
}

// We count how many reviews are needed in total
Expand All @@ -271,13 +273,17 @@ 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 {
missingReviews: requiredAmountOfReviews,
missingUsers: filterMissingUsers(requirements),
teamsToRequest: rule.reviewers.flatMap((r) => r.teams ?? []),
usersToRequest: filterMissingUsers(rule.reviewers),
missingRank,
};
};

Expand Down Expand Up @@ -421,6 +427,7 @@ 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 {
Expand Down Expand Up @@ -507,12 +514,25 @@ export class ActionRunner {
}
}

const getRequiredRanks = (reports: Pick<ReviewReport, "missingRank">[]): 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): RuleReport => {
const ranks = getRequiredRanks(reports);
const missingRank = ranks ? Math.max(...(ranks as number[])) : undefined;

return {
missingReviews: reports.reduce((a, b) => a + b.missingReviews, 0),
missingUsers: [...new Set(reports.flatMap((r) => r.missingUsers))],
teamsToRequest: [...new Set(reports.flatMap((r) => r.teamsToRequest ?? []))],
usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))],
name,
missingRank,
};
};
32 changes: 31 additions & 1 deletion src/test/runner/conditions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@ import { ActionRunner } from "../../runner";
describe("evaluateCondition tests", () => {
let api: MockProxy<PullRequestApi>;
let teamsApi: MockProxy<TeamApi>;
let fellowsApi: MockProxy<TeamApi>;

let runner: ActionRunner;
beforeEach(() => {
api = mock<PullRequestApi>();
teamsApi = mock<TeamApi>();
runner = new ActionRunner(api, teamsApi, mock<TeamApi>(), mock<GitHubChecksApi>(), mock<ActionLogger>());
fellowsApi = mock<TeamApi>();
runner = new ActionRunner(api, teamsApi, fellowsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
});

test("should throw if no teams or users were set", async () => {
Expand Down Expand Up @@ -148,4 +151,31 @@ 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();
});
});
});
72 changes: 71 additions & 1 deletion src/test/runner/validation/and.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@ import { ActionRunner } from "../../../runner";
describe("'And' rule validation", () => {
let api: MockProxy<PullRequestApi>;
let teamsApi: MockProxy<TeamApi>;
let fellowsApi: MockProxy<TeamApi>;
let runner: ActionRunner;
const users = ["user-1", "user-2", "user-3"];
beforeEach(() => {
api = mock<PullRequestApi>();
teamsApi = mock<TeamApi>();
fellowsApi = mock<TeamApi>();
teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users);
api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]);
api.listApprovedReviewsAuthors.mockResolvedValue([]);
runner = new ActionRunner(api, teamsApi, mock<TeamApi>(), mock<GitHubChecksApi>(), mock<ActionLogger>());
runner = new ActionRunner(api, teamsApi, fellowsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
});

describe("approvals", () => {
Expand Down Expand Up @@ -170,4 +172,72 @@ 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);
});
});
});
45 changes: 44 additions & 1 deletion src/test/runner/validation/andDistinct.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,19 @@ import { ActionRunner } from "../../../runner";
describe("'And distinct' rule validation", () => {
let api: MockProxy<PullRequestApi>;
let teamsApi: MockProxy<TeamApi>;
let fellowsApi: MockProxy<TeamApi>;
let runner: ActionRunner;
let logger: MockProxy<ActionLogger>;
const users = ["user-1", "user-2", "user-3"];
beforeEach(() => {
logger = mock<ActionLogger>();
api = mock<PullRequestApi>();
teamsApi = mock<TeamApi>();
fellowsApi = mock<TeamApi>();
teamsApi.getTeamMembers.calledWith("team-abc").mockResolvedValue(users);
api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]);
api.listApprovedReviewsAuthors.mockResolvedValue([]);
runner = new ActionRunner(api, teamsApi, mock<TeamApi>(), mock<GitHubChecksApi>(), logger);
runner = new ActionRunner(api, teamsApi, fellowsApi, mock<GitHubChecksApi>(), logger);
});

describe("Fail scenarios", () => {
Expand Down Expand Up @@ -149,6 +151,47 @@ 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", () => {
Expand Down
46 changes: 45 additions & 1 deletion src/test/runner/validation/or.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@ import { ActionRunner } from "../../../runner";
describe("'Or' rule validation", () => {
let api: MockProxy<PullRequestApi>;
let teamsApi: MockProxy<TeamApi>;
let fellowsApi: MockProxy<TeamApi>;
let runner: ActionRunner;
const users = ["user-1", "user-2", "user-3"];
beforeEach(() => {
api = mock<PullRequestApi>();
teamsApi = mock<TeamApi>();
fellowsApi = mock<TeamApi>();
teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users);
api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]);
api.listApprovedReviewsAuthors.mockResolvedValue([]);
runner = new ActionRunner(api, teamsApi, mock<TeamApi>(), mock<GitHubChecksApi>(), mock<ActionLogger>());
runner = new ActionRunner(api, teamsApi, fellowsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
});

describe("approvals", () => {
Expand Down Expand Up @@ -75,7 +77,28 @@ describe("'Or' rule validation", () => {
const { reports } = await runner.validatePullRequest(config);
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]];
Expand Down Expand Up @@ -123,5 +146,26 @@ describe("'Or' rule validation", () => {
const [result] = reports;
expect(result.missingReviews).toEqual(1);
});

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 },
],
},
],
});
const [result] = reports;
expect(result.missingReviews).toEqual(1);
expect(result.missingUsers).toEqual([users[2]]);
expect(result.missingRank).toEqual(1);
});
});
});

0 comments on commit 088b8b9

Please sign in to comment.