fix(dev-infra): Ensure conditions with groups do not fail verification (#37798)

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
This commit is contained in:
Andrew Scott 2020-07-23 15:53:46 -07:00 committed by Misko Hevery
parent 9821443150
commit 67e3ecc7e3
3 changed files with 37 additions and 18 deletions

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license * 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 {convertConditionToFunction} from './condition_evaluator';
import {PullApproveGroupConfig} from './parse-yaml'; import {PullApproveGroupConfig} from './parse-yaml';
import {PullApproveGroupStateDependencyError} from './pullapprove_arrays'; import {PullApproveGroupStateDependencyError} from './pullapprove_arrays';
@ -16,6 +16,7 @@ interface GroupCondition {
expression: string; expression: string;
checkFn: (files: string[], groups: PullApproveGroup[]) => boolean; checkFn: (files: string[], groups: PullApproveGroup[]) => boolean;
matchedFiles: Set<string>; matchedFiles: Set<string>;
unverifiable: boolean;
} }
/** Result of testing files against the group. */ /** Result of testing files against the group. */
@ -25,6 +26,7 @@ export interface PullApproveGroupResult {
matchedCount: number; matchedCount: number;
unmatchedConditions: GroupCondition[]; unmatchedConditions: GroupCondition[];
unmatchedCount: number; unmatchedCount: number;
unverifiableConditions: GroupCondition[];
} }
// Regular expression that matches conditions for the global approval. // Regular expression that matches conditions for the global approval.
@ -61,6 +63,7 @@ export class PullApproveGroup {
expression, expression,
checkFn: convertConditionToFunction(expression), checkFn: convertConditionToFunction(expression),
matchedFiles: new Set(), matchedFiles: new Set(),
unverifiable: false,
}); });
} catch (e) { } catch (e) {
error(`Could not parse condition in group: ${this.groupName}`); error(`Could not parse condition in group: ${this.groupName}`);
@ -79,7 +82,8 @@ export class PullApproveGroup {
* the pull approve group's conditions. * the pull approve group's conditions.
*/ */
testFile(filePath: string): boolean { testFile(filePath: string): boolean {
return this.conditions.every(({matchedFiles, checkFn, expression}) => { return this.conditions.every((condition) => {
const {matchedFiles, checkFn, expression} = condition;
try { try {
const matchesFile = checkFn([filePath], this.precedingGroups); const matchesFile = checkFn([filePath], this.precedingGroups);
if (matchesFile) { if (matchesFile) {
@ -87,16 +91,14 @@ export class PullApproveGroup {
} }
return matchesFile; return matchesFile;
} catch (e) { } catch (e) {
// In the case of a condition that depends on the state of groups we want to just // In the case of a condition that depends on the state of groups we want to
// warn that the verification can't accurately evaluate the condition and then // ignore that the verification can't accurately evaluate the condition and then
// continue processing. Other types of errors fail the verification, as conditions // continue processing. Other types of errors fail the verification, as conditions
// should otherwise be able to execute without throwing. // should otherwise be able to execute without throwing.
if (e instanceof PullApproveGroupStateDependencyError) { if (e instanceof PullApproveGroupStateDependencyError) {
const errMessage = `Condition could not be evaluated: \n` + condition.unverifiable = true;
`${e.message}\n` + // Return true so that `this.conditions.every` can continue evaluating.
`From the [${this.groupName}] group:\n` + return true;
` - ${expression}`;
warn(errMessage);
} else { } else {
const errMessage = `Condition could not be evaluated: \n\n` + const errMessage = `Condition could not be evaluated: \n\n` +
`From the [${this.groupName}] group:\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. */ /** Retrieve the results for the Group, all matched and unmatched conditions. */
getResults(): PullApproveGroupResult { getResults(): PullApproveGroupResult {
const matchedConditions = 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); const unmatchedConditions =
this.conditions.filter(c => c.matchedFiles.size === 0 && !c.unverifiable);
const unverifiableConditions = this.conditions.filter(c => c.unverifiable);
return { return {
matchedConditions, matchedConditions,
matchedCount: matchedConditions.length, matchedCount: matchedConditions.length,
unmatchedConditions, unmatchedConditions,
unmatchedCount: unmatchedConditions.length, unmatchedCount: unmatchedConditions.length,
unverifiableConditions,
groupName: this.groupName, groupName: this.groupName,
}; };
} }

View File

@ -9,14 +9,23 @@
import {info} from '../utils/console'; import {info} from '../utils/console';
import {PullApproveGroupResult} from './group'; import {PullApproveGroupResult} from './group';
type ConditionGrouping = keyof Pick<
PullApproveGroupResult, 'matchedConditions'|'unmatchedConditions'|'unverifiableConditions'>;
/** Create logs for each pullapprove group result. */ /** Create logs for each pullapprove group result. */
export function logGroup(group: PullApproveGroupResult, matched = true, printMessageFn = info) { export function logGroup(
const conditions = matched ? group.matchedConditions : group.unmatchedConditions; group: PullApproveGroupResult, conditionsToPrint: ConditionGrouping, printMessageFn = info) {
const conditions = group[conditionsToPrint];
printMessageFn.group(`[${group.groupName}]`); printMessageFn.group(`[${group.groupName}]`);
if (conditions.length) { if (conditions.length) {
conditions.forEach(matcher => { conditions.forEach(groupCondition => {
const count = matcher.matchedFiles.size; const count = groupCondition.matchedFiles.size;
printMessageFn(`${count} ${count === 1 ? 'match' : 'matches'} - ${matcher.expression}`); if (conditionsToPrint === 'unverifiableConditions') {
printMessageFn(`${groupCondition.expression}`);
} else {
printMessageFn(
`${count} ${count === 1 ? 'match' : 'matches'} - ${groupCondition.expression}`);
}
}); });
printMessageFn.groupEnd(); printMessageFn.groupEnd();
} }

View File

@ -84,11 +84,16 @@ export function verify() {
info.groupEnd(); info.groupEnd();
const matchedGroups = resultsByGroup.filter(group => !group.unmatchedCount); const matchedGroups = resultsByGroup.filter(group => !group.unmatchedCount);
info.group(`Matched conditions by Group (${matchedGroups.length} groups)`); 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(); info.groupEnd();
const unmatchedGroups = resultsByGroup.filter(group => group.unmatchedCount); const unmatchedGroups = resultsByGroup.filter(group => group.unmatchedCount);
info.group(`Unmatched conditions by Group (${unmatchedGroups.length} groups)`); 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(); info.groupEnd();
// Provide correct exit code based on verification success. // Provide correct exit code based on verification success.