From 67e3ecc7e31a8bb8a48a614deabda9dd5932f919 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 23 Jul 2020 15:53:46 -0700 Subject: [PATCH] fix(dev-infra): Ensure conditions with groups do not fail verification (#37798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are a few changes in this PR to ensure conditions that are based on groups (i.e. `- groups.pending.length == 0`) do not fail the verify task: * Remove the warning when a condition is encountered that depends on the `groups` state. The warning will otherwise be printed once for every file that triggers the execution of the condition (400,000+ times) * Add an `unverifiable` flag to `GroupCondition` interface and set it to true when an error is encountered due to attempting to get the state of `groups` in a condition * Ignore any unverifiable conditions when gathering unmatched conditions. These should not be considered `unmatched` for verification purposes. * Print the unverifiable conditions by group in the results Sample output: ``` ┌──────────────────────────────────────────────────────────────────────────────┐ │ PullApprove results by group │ └──────────────────────────────────────────────────────────────────────────────┘ Groups skipped (4 groups) Matched conditions by Group (37 groups) Unmatched conditions by Group (0 groups) Unverifiable conditions by Group (3 groups) [public-api] len(groups.pending.exclude("required-minimum-review")... len(groups.rejected.exclude("required-minimum-review")... [size-tracking] len(groups.pending.exclude("required-minimum-review")... len(groups.rejected.exclude("required-minimum-review")... [circular-dependencies] len(groups.pending.exclude("required-minimum-review")... len(groups.rejected.exclude("required-minimum-review")... ``` PR Close #37798 --- dev-infra/pullapprove/group.ts | 27 ++++++++++++++++----------- dev-infra/pullapprove/logging.ts | 19 ++++++++++++++----- dev-infra/pullapprove/verify.ts | 9 +++++++-- 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/dev-infra/pullapprove/group.ts b/dev-infra/pullapprove/group.ts index 45de9eac23..3aaeaf1cf6 100644 --- a/dev-infra/pullapprove/group.ts +++ b/dev-infra/pullapprove/group.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {error, warn} from '../utils/console'; +import {error} from '../utils/console'; import {convertConditionToFunction} from './condition_evaluator'; import {PullApproveGroupConfig} from './parse-yaml'; import {PullApproveGroupStateDependencyError} from './pullapprove_arrays'; @@ -16,6 +16,7 @@ interface GroupCondition { expression: string; checkFn: (files: string[], groups: PullApproveGroup[]) => boolean; matchedFiles: Set; + unverifiable: boolean; } /** Result of testing files against the group. */ @@ -25,6 +26,7 @@ export interface PullApproveGroupResult { matchedCount: number; unmatchedConditions: GroupCondition[]; unmatchedCount: number; + unverifiableConditions: GroupCondition[]; } // Regular expression that matches conditions for the global approval. @@ -61,6 +63,7 @@ export class PullApproveGroup { expression, checkFn: convertConditionToFunction(expression), matchedFiles: new Set(), + unverifiable: false, }); } catch (e) { error(`Could not parse condition in group: ${this.groupName}`); @@ -79,7 +82,8 @@ export class PullApproveGroup { * the pull approve group's conditions. */ testFile(filePath: string): boolean { - return this.conditions.every(({matchedFiles, checkFn, expression}) => { + return this.conditions.every((condition) => { + const {matchedFiles, checkFn, expression} = condition; try { const matchesFile = checkFn([filePath], this.precedingGroups); if (matchesFile) { @@ -87,16 +91,14 @@ export class PullApproveGroup { } return matchesFile; } catch (e) { - // In the case of a condition that depends on the state of groups we want to just - // warn that the verification can't accurately evaluate the condition and then + // In the case of a condition that depends on the state of groups we want to + // ignore that the verification can't accurately evaluate the condition and then // continue processing. Other types of errors fail the verification, as conditions // should otherwise be able to execute without throwing. if (e instanceof PullApproveGroupStateDependencyError) { - const errMessage = `Condition could not be evaluated: \n` + - `${e.message}\n` + - `From the [${this.groupName}] group:\n` + - ` - ${expression}`; - warn(errMessage); + condition.unverifiable = true; + // Return true so that `this.conditions.every` can continue evaluating. + return true; } else { const errMessage = `Condition could not be evaluated: \n\n` + `From the [${this.groupName}] group:\n` + @@ -111,13 +113,16 @@ export class PullApproveGroup { /** Retrieve the results for the Group, all matched and unmatched conditions. */ getResults(): PullApproveGroupResult { - const matchedConditions = this.conditions.filter(c => !!c.matchedFiles.size); - const unmatchedConditions = this.conditions.filter(c => !c.matchedFiles.size); + const matchedConditions = this.conditions.filter(c => c.matchedFiles.size > 0); + const unmatchedConditions = + this.conditions.filter(c => c.matchedFiles.size === 0 && !c.unverifiable); + const unverifiableConditions = this.conditions.filter(c => c.unverifiable); return { matchedConditions, matchedCount: matchedConditions.length, unmatchedConditions, unmatchedCount: unmatchedConditions.length, + unverifiableConditions, groupName: this.groupName, }; } diff --git a/dev-infra/pullapprove/logging.ts b/dev-infra/pullapprove/logging.ts index ea289b7739..573516e23e 100644 --- a/dev-infra/pullapprove/logging.ts +++ b/dev-infra/pullapprove/logging.ts @@ -9,14 +9,23 @@ import {info} from '../utils/console'; import {PullApproveGroupResult} from './group'; +type ConditionGrouping = keyof Pick< + PullApproveGroupResult, 'matchedConditions'|'unmatchedConditions'|'unverifiableConditions'>; + /** Create logs for each pullapprove group result. */ -export function logGroup(group: PullApproveGroupResult, matched = true, printMessageFn = info) { - const conditions = matched ? group.matchedConditions : group.unmatchedConditions; +export function logGroup( + group: PullApproveGroupResult, conditionsToPrint: ConditionGrouping, printMessageFn = info) { + const conditions = group[conditionsToPrint]; printMessageFn.group(`[${group.groupName}]`); if (conditions.length) { - conditions.forEach(matcher => { - const count = matcher.matchedFiles.size; - printMessageFn(`${count} ${count === 1 ? 'match' : 'matches'} - ${matcher.expression}`); + conditions.forEach(groupCondition => { + const count = groupCondition.matchedFiles.size; + if (conditionsToPrint === 'unverifiableConditions') { + printMessageFn(`${groupCondition.expression}`); + } else { + printMessageFn( + `${count} ${count === 1 ? 'match' : 'matches'} - ${groupCondition.expression}`); + } }); printMessageFn.groupEnd(); } diff --git a/dev-infra/pullapprove/verify.ts b/dev-infra/pullapprove/verify.ts index d8302c2caa..1674083383 100644 --- a/dev-infra/pullapprove/verify.ts +++ b/dev-infra/pullapprove/verify.ts @@ -84,11 +84,16 @@ export function verify() { info.groupEnd(); const matchedGroups = resultsByGroup.filter(group => !group.unmatchedCount); info.group(`Matched conditions by Group (${matchedGroups.length} groups)`); - matchedGroups.forEach(group => logGroup(group, true, debug)); + matchedGroups.forEach(group => logGroup(group, 'matchedConditions', debug)); info.groupEnd(); const unmatchedGroups = resultsByGroup.filter(group => group.unmatchedCount); info.group(`Unmatched conditions by Group (${unmatchedGroups.length} groups)`); - unmatchedGroups.forEach(group => logGroup(group, false)); + unmatchedGroups.forEach(group => logGroup(group, 'unmatchedConditions')); + info.groupEnd(); + const unverifiableConditionsInGroups = + resultsByGroup.filter(group => group.unverifiableConditions.length > 0); + info.group(`Unverifiable conditions by Group (${unverifiableConditionsInGroups.length} groups)`); + unverifiableConditionsInGroups.forEach(group => logGroup(group, 'unverifiableConditions')); info.groupEnd(); // Provide correct exit code based on verification success.