refactor(dev-infra): Add support for groups in the conditions evaluator (#38164)

Conditions can refer to the groups array that is a list of the preceding groups.
This commit adds support to the verification for those conditions.

This commit also adds some tests to the parsing and condition matching
to ensure everything works as expected.

PR Close #38164

dev-infra/pullapprove/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@npm_bazel_typescript//:index.bzl", "ts_library")
+load("//tools:defaults.bzl", "jasmine_node_test")
 
 ts_library(
     name = "pullapprove",
@@ -8,6 +9,8 @@ ts_library(
         "group.ts",
         "logging.ts",
         "parse-yaml.ts",
+        "pullapprove_arrays.ts",
+        "utils.ts",
         "verify.ts",
     ],
     module_name = "@angular/dev-infra-private/pullapprove",
@@ -25,3 +28,24 @@ ts_library(
         "@npm//yargs",
     ],
 )
+
+ts_library(
+    name = "pullapprove_test_lib",
+    testonly = True,
+    srcs = glob(
+        ["*.spec.ts"],
+    ),
+    visibility = ["//visibility:private"],
+    deps = [
+        ":pullapprove",
+        "@npm//@types/jasmine",
+        "@npm//@types/node",
+        "@npm//typescript",
+    ],
+)
+
+jasmine_node_test(
+    name = "pullapprove_test",
+    srcs = [":pullapprove_test_lib"],
+    visibility = ["//visibility:private"],
+)

dev-infra/pullapprove/condition_evaluator.ts

@@ -6,10 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {IMinimatch, Minimatch} from 'minimatch';
-
-/** Map that holds patterns and their corresponding Minimatch globs. */
-const patternCache = new Map<string, IMinimatch>();
+import {PullApproveGroup} from './group';
+import {PullApproveGroupArray, PullApproveStringArray} from './pullapprove_arrays';
+import {getOrCreateGlob} from './utils';
 
 /**
  * Context that is provided to conditions. Conditions can use various helpers
@@ -18,30 +17,34 @@ const patternCache = new Map<string, IMinimatch>();
  */
 const conditionContext = {
   'len': (value: any[]) => value.length,
-  'contains_any_globs': (files: PullApproveArray, patterns: string[]) => {
+  'contains_any_globs': (files: PullApproveStringArray, patterns: string[]) => {
     // Note: Do not always create globs for the same pattern again. This method
     // could be called for each source file. Creating glob's is expensive.
     return files.some(f => patterns.some(pattern => getOrCreateGlob(pattern).match(f)));
-  }
+  },
 };
 
 /**
  * Converts a given condition to a function that accepts a set of files. The returned
  * function can be called to check if the set of files matches the condition.
  */
-export function convertConditionToFunction(expr: string): (files: string[]) => boolean {
-  // Creates a dynamic function with the specified expression. The first parameter will
-  // be `files` as that corresponds to the supported `files` variable that can be accessed
-  // in PullApprove condition expressions. The followed parameters correspond to other
-  // context variables provided by PullApprove for conditions.
-  const evaluateFn = new Function('files', ...Object.keys(conditionContext), `
+export function convertConditionToFunction(expr: string): (
+    files: string[], groups: PullApproveGroup[]) => boolean {
+  // Creates a dynamic function with the specified expression.
+  // The first parameter will be `files` as that corresponds to the supported `files` variable that
+  // can be accessed in PullApprove condition expressions. The second parameter is the list of
+  // PullApproveGroups that are accessible in the condition expressions. The followed parameters
+  // correspond to other context variables provided by PullApprove for conditions.
+  const evaluateFn = new Function('files', 'groups', ...Object.keys(conditionContext), `
     return (${transformExpressionToJs(expr)});
   `);
 
   // Create a function that calls the dynamically constructed function which mimics
   // the condition expression that is usually evaluated with Python in PullApprove.
-  return files => {
-    const result = evaluateFn(new PullApproveArray(...files), ...Object.values(conditionContext));
+  return (files, groups) => {
+    const result = evaluateFn(
+        new PullApproveStringArray(...files), new PullApproveGroupArray(...groups),
+        ...Object.values(conditionContext));
     // If an array is returned, we consider the condition as active if the array is not
     // empty. This matches PullApprove's condition evaluation that is based on Python.
     if (Array.isArray(result)) {
@@ -59,41 +62,3 @@ export function convertConditionToFunction(expr: string): (files: string[]) => b
 function transformExpressionToJs(expression: string): string {
   return expression.replace(/not\s+/g, '!');
 }
-
-/**
- * Superset of a native array. The superset provides methods which mimic the
- * list data structure used in PullApprove for files in conditions.
- */
-class PullApproveArray extends Array<string> {
-  constructor(...elements: string[]) {
-    super(...elements);
-
-    // Set the prototype explicitly because in ES5, the prototype is accidentally
-    // lost due to a limitation in down-leveling.
-    // https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work.
-    Object.setPrototypeOf(this, PullApproveArray.prototype);
-  }
-
-  /** Returns a new array which only includes files that match the given pattern. */
-  include(pattern: string): PullApproveArray {
-    return new PullApproveArray(...this.filter(s => getOrCreateGlob(pattern).match(s)));
-  }
-
-  /** Returns a new array which only includes files that did not match the given pattern. */
-  exclude(pattern: string): PullApproveArray {
-    return new PullApproveArray(...this.filter(s => !getOrCreateGlob(pattern).match(s)));
-  }
-}
-
-/**
- * Gets a glob for the given pattern. The cached glob will be returned
- * if available. Otherwise a new glob will be created and cached.
- */
-function getOrCreateGlob(pattern: string) {
-  if (patternCache.has(pattern)) {
-    return patternCache.get(pattern)!;
-  }
-  const glob = new Minimatch(pattern, {dot: true});
-  patternCache.set(pattern, glob);
-  return glob;
-}

dev-infra/pullapprove/group.ts

@@ -6,14 +6,15 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {error} from '../utils/console';
+import {error, warn} from '../utils/console';
 import {convertConditionToFunction} from './condition_evaluator';
 import {PullApproveGroupConfig} from './parse-yaml';
+import {PullApproveGroupStateDependencyError} from './pullapprove_arrays';
 
 /** A condition for a group. */
 interface GroupCondition {
   expression: string;
-  checkFn: (files: string[]) => boolean;
+  checkFn: (files: string[], groups: PullApproveGroup[]) => boolean;
   matchedFiles: Set<string>;
 }
 
@@ -39,7 +40,9 @@ export class PullApproveGroup {
   /** List of conditions for the group. */
   conditions: GroupCondition[] = [];
 
-  constructor(public groupName: string, config: PullApproveGroupConfig) {
+  constructor(
+      public groupName: string, config: PullApproveGroupConfig,
+      readonly precedingGroups: PullApproveGroup[] = []) {
     this._captureConditions(config);
   }
 
@@ -78,12 +81,23 @@ export class PullApproveGroup {
   testFile(filePath: string): boolean {
     return this.conditions.every(({matchedFiles, checkFn, expression}) => {
       try {
-        const matchesFile = checkFn([filePath]);
+        const matchesFile = checkFn([filePath], this.precedingGroups);
         if (matchesFile) {
           matchedFiles.add(filePath);
         }
         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
+        // 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);
+        } else {
           const errMessage = `Condition could not be evaluated: \n\n` +
               `From the [${this.groupName}] group:\n` +
               ` - ${expression}` +
@@ -91,6 +105,7 @@ export class PullApproveGroup {
           error(errMessage);
           process.exit(1);
         }
+      }
     });
   }
 

dev-infra/pullapprove/parse-yaml.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {parse as parseYaml} from 'yaml';
+import {PullApproveGroup} from './group';
 
 export interface PullApproveGroupConfig {
   conditions?: string[];
@@ -33,3 +34,12 @@ export interface PullApproveConfig {
 export function parsePullApproveYaml(rawYaml: string): PullApproveConfig {
   return parseYaml(rawYaml, {merge: true}) as PullApproveConfig;
 }
+
+/** Parses all of the groups defined in the pullapprove yaml. */
+export function getGroupsFromYaml(pullApproveYamlRaw: string): PullApproveGroup[] {
+  /** JSON representation of the pullapprove yaml file. */
+  const pullApprove = parsePullApproveYaml(pullApproveYamlRaw);
+  return Object.entries(pullApprove.groups).reduce((groups, [groupName, group]) => {
+    return groups.concat(new PullApproveGroup(groupName, group, groups));
+  }, [] as PullApproveGroup[]);
+}

dev-infra/pullapprove/pullapprove_arrays.ts

@@ -0,0 +1,89 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+import {PullApproveGroup} from './group';
+import {getOrCreateGlob} from './utils';
+
+export class PullApproveGroupStateDependencyError extends Error {
+  constructor(message?: string) {
+    super(message);
+    // Set the prototype explicitly because in ES5, the prototype is accidentally
+    // lost due to a limitation in down-leveling.
+    // https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work.
+    Object.setPrototypeOf(this, PullApproveGroupStateDependencyError.prototype);
+    // Error names are displayed in their stack but can't be set in the constructor.
+    this.name = PullApproveGroupStateDependencyError.name;
+  }
+}
+
+/**
+ * Superset of a native array. The superset provides methods which mimic the
+ * list data structure used in PullApprove for files in conditions.
+ */
+export class PullApproveStringArray extends Array<string> {
+  constructor(...elements: string[]) {
+    super(...elements);
+
+    // Set the prototype explicitly because in ES5, the prototype is accidentally
+    // lost due to a limitation in down-leveling.
+    // https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work.
+    Object.setPrototypeOf(this, PullApproveStringArray.prototype);
+  }
+  /** Returns a new array which only includes files that match the given pattern. */
+  include(pattern: string): PullApproveStringArray {
+    return new PullApproveStringArray(...this.filter(s => getOrCreateGlob(pattern).match(s)));
+  }
+
+  /** Returns a new array which only includes files that did not match the given pattern. */
+  exclude(pattern: string): PullApproveStringArray {
+    return new PullApproveStringArray(...this.filter(s => !getOrCreateGlob(pattern).match(s)));
+  }
+}
+
+/**
+ * Superset of a native array. The superset provides methods which mimic the
+ * list data structure used in PullApprove for groups in conditions.
+ */
+export class PullApproveGroupArray extends Array<PullApproveGroup> {
+  constructor(...elements: PullApproveGroup[]) {
+    super(...elements);
+
+    // Set the prototype explicitly because in ES5, the prototype is accidentally
+    // lost due to a limitation in down-leveling.
+    // https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work.
+    Object.setPrototypeOf(this, PullApproveGroupArray.prototype);
+  }
+
+  include(pattern: string): PullApproveGroupArray {
+    return new PullApproveGroupArray(...this.filter(s => s.groupName.match(pattern)));
+  }
+
+  /** Returns a new array which only includes files that did not match the given pattern. */
+  exclude(pattern: string): PullApproveGroupArray {
+    return new PullApproveGroupArray(...this.filter(s => s.groupName.match(pattern)));
+  }
+
+  get pending() {
+    throw new PullApproveGroupStateDependencyError();
+  }
+
+  get active() {
+    throw new PullApproveGroupStateDependencyError();
+  }
+
+  get inactive() {
+    throw new PullApproveGroupStateDependencyError();
+  }
+
+  get rejected() {
+    throw new PullApproveGroupStateDependencyError();
+  }
+
+  get names() {
+    return this.map(g => g.groupName);
+  }
+}

dev-infra/pullapprove/utils.ts

@@ -0,0 +1,24 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+import {IMinimatch, Minimatch} from 'minimatch';
+
+/** Map that holds patterns and their corresponding Minimatch globs. */
+const patternCache = new Map<string, IMinimatch>();
+
+/**
+ * Gets a glob for the given pattern. The cached glob will be returned
+ * if available. Otherwise a new glob will be created and cached.
+ */
+export function getOrCreateGlob(pattern: string) {
+  if (patternCache.has(pattern)) {
+    return patternCache.get(pattern)!;
+  }
+  const glob = new Minimatch(pattern, {dot: true});
+  patternCache.set(pattern, glob);
+  return glob;
+}

dev-infra/pullapprove/verify.spec.ts

@@ -0,0 +1,88 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+import {PullApproveGroup} from './group';
+import {getGroupsFromYaml} from './parse-yaml';
+
+describe('group parsing', () => {
+  it('gets group name', () => {
+    const groupName = 'fw-migrations';
+    const groups = getGroupsFromYaml(`
+      groups:
+        ${groupName}:
+          type: optional
+      `);
+    expect(groups[0].groupName).toBe(groupName);
+  });
+
+  it('gets correct number of groups', () => {
+    const groups = getGroupsFromYaml(`
+      groups:
+        fw-migrations:
+          type: optional
+        fw-core:
+          type: optional
+      `);
+    expect(groups.length).toBe(2);
+  });
+
+  it('gets preceding groups', () => {
+    const groups = getGroupsFromYaml(`
+      groups:
+        fw-migrations:
+          type: optional
+        fw-core:
+          type: optional
+        dev-infra:
+          type: optional
+      `);
+    const fwMigrations = getGroupByName(groups, 'fw-migrations')!;
+    const fwCore = getGroupByName(groups, 'fw-core')!;
+    const devInfra = getGroupByName(groups, 'dev-infra')!;
+    expect(getGroupNames(fwMigrations.precedingGroups)).toEqual([]);
+    expect(getGroupNames(fwCore.precedingGroups)).toEqual([fwMigrations.groupName]);
+    expect(getGroupNames(devInfra.precedingGroups)).toEqual([
+      fwMigrations.groupName, fwCore.groupName
+    ]);
+  });
+
+  it('matches file conditions', () => {
+    const groups = getGroupsFromYaml(`
+      groups:
+        fw-core:
+          conditions:
+            - contains_any_globs(files, ['packages/core/**'])
+      `);
+    const fwCore = getGroupByName(groups, 'fw-core')!;
+    expect(fwCore.testFile('packages/core/test.ts')).toBe(true);
+    expect(fwCore.testFile('some/other/location/test.ts')).toBe(false);
+  });
+
+  it('allows conditions based on groups', () => {
+    const groups = getGroupsFromYaml(`
+      groups:
+        fw-migrations:
+          conditions:
+            - len(groups) > 0
+        fw-core:
+          conditions:
+            - len(groups.active) > 0
+      `);
+    const fwMigrations = getGroupByName(groups, 'fw-migrations')!;
+    expect(() => fwMigrations.testFile('any')).not.toThrow();
+    const fwCore = getGroupByName(groups, 'fw-core')!;
+    expect(() => fwCore.testFile('any')).not.toThrow();
+  });
+});
+
+function getGroupByName(groups: PullApproveGroup[], name: string): PullApproveGroup|undefined {
+  return groups.find(g => g.groupName === name);
+}
+
+function getGroupNames(groups: PullApproveGroup[]) {
+  return groups.map(g => g.groupName);
+}

dev-infra/pullapprove/verify.ts

@@ -11,10 +11,8 @@ import {resolve} from 'path';
 import {getRepoBaseDir} from '../utils/config';
 import {debug, info} from '../utils/console';
 import {allFiles} from '../utils/repo-files';
-
-import {PullApproveGroup} from './group';
 import {logGroup, logHeader} from './logging';
-import {parsePullApproveYaml} from './parse-yaml';
+import {getGroupsFromYaml} from './parse-yaml';
 
 export function verify() {
   /** Full path to PullApprove config file */
@@ -23,12 +21,8 @@ export function verify() {
   const REPO_FILES = allFiles();
   /** The pull approve config file. */
   const pullApproveYamlRaw = readFileSync(PULL_APPROVE_YAML_PATH, 'utf8');
-  /** JSON representation of the pullapprove yaml file. */
-  const pullApprove = parsePullApproveYaml(pullApproveYamlRaw);
   /** All of the groups defined in the pullapprove yaml. */
-  const groups = Object.entries(pullApprove.groups).map(([groupName, group]) => {
-    return new PullApproveGroup(groupName, group);
-  });
+  const groups = getGroupsFromYaml(pullApproveYamlRaw);
   /**
    * PullApprove groups without conditions. These are skipped in the verification
    * as those would always be active and cause zero unmatched files.

dev-infra/utils/BUILD.bazel

@@ -16,6 +16,7 @@ ts_library(
         "@npm//@types/node",
         "@npm//@types/shelljs",
         "@npm//chalk",
+        "@npm//inquirer",
         "@npm//shelljs",
         "@npm//tslib",
         "@npm//typed-graphqlify",