Merge pull request #174 from github/nickfyson/error_wrapper

add error-catching wrapper around calls to toolrunner

src/codeql.ts

@@ -11,8 +11,10 @@ import uuidV4 from "uuid/v4";
 
 import * as api from "./api-client";
 import * as defaults from "./defaults.json"; // Referenced from codeql-action-sync-tool!
+import { errorMatchers } from "./error-matcher";
 import { Language } from "./languages";
 import { Logger } from "./logging";
+import { toolrunnerErrorCatcher } from "./toolrunner-error-catcher";
 import * as util from "./util";
 
 type Options = Array<string | number | boolean>;
@@ -505,22 +507,30 @@ function getCodeQLForCmd(cmd: string): CodeQL {
       );
 
       // Run trace command
-      await new toolrunnner.ToolRunner(cmd, [
-        "database",
-        "trace-command",
-        ...getExtraOptionsFromEnv(["database", "trace-command"]),
-        databasePath,
-        "--",
-        traceCommand,
-      ]).exec();
+      await toolrunnerErrorCatcher(
+        cmd,
+        [
+          "database",
+          "trace-command",
+          ...getExtraOptionsFromEnv(["database", "trace-command"]),
+          databasePath,
+          "--",
+          traceCommand,
+        ],
+        errorMatchers
+      );
     },
     async finalizeDatabase(databasePath: string) {
-      await new toolrunnner.ToolRunner(cmd, [
-        "database",
-        "finalize",
-        ...getExtraOptionsFromEnv(["database", "finalize"]),
-        databasePath,
-      ]).exec();
+      await toolrunnerErrorCatcher(
+        cmd,
+        [
+          "database",
+          "finalize",
+          ...getExtraOptionsFromEnv(["database", "finalize"]),
+          databasePath,
+        ],
+        errorMatchers
+      );
     },
     async resolveQueries(
       queries: string[],

src/error-matcher.test.ts

@@ -0,0 +1,32 @@
+import test from "ava";
+
+import { namedMatchersForTesting } from "./error-matcher";
+
+/*
+NB We test the regexes for all the matchers against example log output snippets.
+*/
+
+test("noSourceCodeFound matches against example javascript output", async (t) => {
+  t.assert(
+    testErrorMatcher(
+      "noSourceCodeFound",
+      `
+  2020-09-07T17:39:53.9050522Z [2020-09-07 17:39:53] [build] Done extracting /opt/hostedtoolcache/CodeQL/0.0.0-20200630/x64/codeql/javascript/tools/data/externs/web/ie_vml.js (3 ms)
+  2020-09-07T17:39:53.9051849Z [2020-09-07 17:39:53] [build-err] No JavaScript or TypeScript code found.
+  2020-09-07T17:39:53.9052444Z [2020-09-07 17:39:53] [build-err] No JavaScript or TypeScript code found.
+  2020-09-07T17:39:53.9251124Z [2020-09-07 17:39:53] [ERROR] Spawned process exited abnormally (code 255; tried to run: [/opt/hostedtoolcache/CodeQL/0.0.0-20200630/x64/codeql/javascript/tools/autobuild.sh])
+  `
+    )
+  );
+});
+
+function testErrorMatcher(matcherName: string, logSample: string): boolean {
+  if (!(matcherName in namedMatchersForTesting)) {
+    throw new Error(`Unknown matcher ${matcherName}`);
+  }
+  const regex = namedMatchersForTesting[matcherName].outputRegex;
+  if (regex === undefined) {
+    throw new Error(`Cannot test matcher ${matcherName} with null regex`);
+  }
+  return regex.test(logSample);
+}

src/error-matcher.ts

@@ -0,0 +1,24 @@
+// defines properties to match against the result of executed commands,
+// and a custom error to return when a match is found
+export interface ErrorMatcher {
+  exitCode?: number; // exit code of the run process
+  outputRegex?: RegExp; // pattern to match against either stdout or stderr
+  message: string; // the error message that will be thrown for a matching process
+}
+
+// exported only for testing purposes
+export const namedMatchersForTesting: { [key: string]: ErrorMatcher } = {
+  /*
+  In due course it may be possible to remove the regex, if/when javascript also exits with code 32.
+  */
+  noSourceCodeFound: {
+    exitCode: 32,
+    outputRegex: new RegExp("No JavaScript or TypeScript code found\\."),
+    message:
+      "No code found during the build. Please see:\n" +
+      "https://docs.github.com/en/github/finding-security-vulnerabilities-and-errors-in-your-code/troubleshooting-code-scanning#no-code-found-during-the-build",
+  },
+};
+
+// we collapse the matches into an array for use in execErrorCatcher
+export const errorMatchers = Object.values(namedMatchersForTesting);

src/toolrunner-error-catcher.test.ts

@@ -0,0 +1,209 @@
+import * as exec from "@actions/exec";
+import test from "ava";
+
+import { ErrorMatcher } from "./error-matcher";
+import { setupTests } from "./testing-utils";
+import { toolrunnerErrorCatcher } from "./toolrunner-error-catcher";
+
+setupTests(test);
+
+test("matchers are never applied if non-error exit", async (t) => {
+  const testArgs = buildDummyArgs(
+    "foo bar\\nblort qux",
+    "foo bar\\nblort qux",
+    "",
+    0
+  );
+
+  const matchers: ErrorMatcher[] = [
+    { exitCode: 123, outputRegex: new RegExp("foo bar"), message: "error!!!" },
+  ];
+
+  t.deepEqual(await exec.exec("node", testArgs), 0);
+
+  t.deepEqual(await toolrunnerErrorCatcher("node", testArgs, matchers), 0);
+});
+
+test("regex matchers are applied to stdout for non-zero exit code", async (t) => {
+  const testArgs = buildDummyArgs("foo bar\\nblort qux", "", "", 1);
+
+  const matchers: ErrorMatcher[] = [
+    { exitCode: 123, outputRegex: new RegExp("foo bar"), message: "🦄" },
+  ];
+
+  await t.throwsAsync(exec.exec("node", testArgs), {
+    instanceOf: Error,
+    message: "The process 'node' failed with exit code 1",
+  });
+
+  await t.throwsAsync(toolrunnerErrorCatcher("node", testArgs, matchers), {
+    instanceOf: Error,
+    message: "🦄",
+  });
+});
+
+test("regex matchers are applied to stderr for non-zero exit code", async (t) => {
+  const testArgs = buildDummyArgs(
+    "non matching string",
+    "foo bar\\nblort qux",
+    "",
+    1
+  );
+
+  const matchers: ErrorMatcher[] = [
+    { exitCode: 123, outputRegex: new RegExp("foo bar"), message: "🦄" },
+  ];
+
+  await t.throwsAsync(exec.exec("node", testArgs), {
+    instanceOf: Error,
+    message: "The process 'node' failed with exit code 1",
+  });
+
+  await t.throwsAsync(toolrunnerErrorCatcher("node", testArgs, matchers), {
+    instanceOf: Error,
+    message: "🦄",
+  });
+});
+
+test("matcher returns correct error message when multiple matchers defined", async (t) => {
+  const testArgs = buildDummyArgs(
+    "non matching string",
+    "foo bar\\nblort qux",
+    "",
+    1
+  );
+
+  const matchers: ErrorMatcher[] = [
+    { exitCode: 456, outputRegex: new RegExp("lorem ipsum"), message: "😩" },
+    { exitCode: 123, outputRegex: new RegExp("foo bar"), message: "🦄" },
+    { exitCode: 789, outputRegex: new RegExp("blah blah"), message: "🤦‍♂️" },
+  ];
+
+  await t.throwsAsync(exec.exec("node", testArgs), {
+    instanceOf: Error,
+    message: "The process 'node' failed with exit code 1",
+  });
+
+  await t.throwsAsync(toolrunnerErrorCatcher("node", testArgs, matchers), {
+    instanceOf: Error,
+    message: "🦄",
+  });
+});
+
+test("matcher returns first match to regex when multiple matches", async (t) => {
+  const testArgs = buildDummyArgs(
+    "non matching string",
+    "foo bar\\nblort qux",
+    "",
+    1
+  );
+
+  const matchers: ErrorMatcher[] = [
+    { exitCode: 123, outputRegex: new RegExp("foo bar"), message: "🦄" },
+    { exitCode: 789, outputRegex: new RegExp("blah blah"), message: "🤦‍♂️" },
+    { exitCode: 987, outputRegex: new RegExp("foo bar"), message: "🚫" },
+  ];
+
+  await t.throwsAsync(exec.exec("node", testArgs), {
+    instanceOf: Error,
+    message: "The process 'node' failed with exit code 1",
+  });
+
+  await t.throwsAsync(toolrunnerErrorCatcher("node", testArgs, matchers), {
+    instanceOf: Error,
+    message: "🦄",
+  });
+});
+
+test("exit code matchers are applied", async (t) => {
+  const testArgs = buildDummyArgs(
+    "non matching string",
+    "foo bar\\nblort qux",
+    "",
+    123
+  );
+
+  const matchers: ErrorMatcher[] = [
+    {
+      exitCode: 123,
+      outputRegex: new RegExp("this will not match"),
+      message: "🦄",
+    },
+  ];
+
+  await t.throwsAsync(exec.exec("node", testArgs), {
+    instanceOf: Error,
+    message: "The process 'node' failed with exit code 123",
+  });
+
+  await t.throwsAsync(toolrunnerErrorCatcher("node", testArgs, matchers), {
+    instanceOf: Error,
+    message: "🦄",
+  });
+});
+
+test("execErrorCatcher respects the ignoreReturnValue option", async (t) => {
+  const testArgs = buildDummyArgs("standard output", "error output", "", 199);
+
+  await t.throwsAsync(
+    toolrunnerErrorCatcher("node", testArgs, [], { ignoreReturnCode: false }),
+    { instanceOf: Error }
+  );
+
+  t.deepEqual(
+    await toolrunnerErrorCatcher("node", testArgs, [], {
+      ignoreReturnCode: true,
+    }),
+    199
+  );
+});
+
+test("execErrorCatcher preserves behavior of provided listeners", async (t) => {
+  const stdoutExpected = "standard output";
+  const stderrExpected = "error output";
+
+  let stdoutActual = "";
+  let stderrActual = "";
+
+  const listeners = {
+    stdout: (data: Buffer) => {
+      stdoutActual += data.toString();
+    },
+    stderr: (data: Buffer) => {
+      stderrActual += data.toString();
+    },
+  };
+
+  const testArgs = buildDummyArgs(stdoutExpected, stderrExpected, "", 0);
+
+  t.deepEqual(
+    await toolrunnerErrorCatcher("node", testArgs, [], {
+      listeners,
+    }),
+    0
+  );
+
+  t.deepEqual(stdoutActual, `${stdoutExpected}\n`);
+  t.deepEqual(stderrActual, `${stderrExpected}\n`);
+});
+
+function buildDummyArgs(
+  stdoutContents: string,
+  stderrContents: string,
+  desiredErrorMessage?: string,
+  desiredExitCode?: number
+): string[] {
+  let command = "";
+
+  if (stdoutContents) command += `console.log("${stdoutContents}");`;
+  if (stderrContents) command += `console.error("${stderrContents}");`;
+
+  if (command.length === 0)
+    throw new Error("Must provide contents for either stdout or stderr");
+
+  if (desiredErrorMessage)
+    command += `throw new Error("${desiredErrorMessage}");`;
+  if (desiredExitCode) command += `process.exitCode = ${desiredExitCode};`;
+
+  return ["-e", command];
+}

src/toolrunner-error-catcher.ts

@@ -0,0 +1,86 @@
+import * as im from "@actions/exec/lib/interfaces";
+import * as toolrunnner from "@actions/exec/lib/toolrunner";
+
+import { ErrorMatcher } from "./error-matcher";
+
+/**
+ * Wrapper for toolrunner.Toolrunner which checks for specific return code and/or regex matches in console output.
+ * Output will be streamed to the live console as well as captured for subsequent processing.
+ * Returns promise with return code
+ *
+ * @param     commandLine        command to execute
+ * @param     args               optional arguments for tool. Escaping is handled by the lib.
+ * @param     matchers           defines specific codes and/or regexes that should lead to return of a custom error
+ * @param     options            optional exec options.  See ExecOptions
+ * @returns   Promise<number>    exit code
+ */
+export async function toolrunnerErrorCatcher(
+  commandLine: string,
+  args?: string[],
+  matchers?: ErrorMatcher[],
+  options?: im.ExecOptions
+): Promise<number> {
+  let stdout = "";
+  let stderr = "";
+
+  const listeners = {
+    stdout: (data: Buffer) => {
+      stdout += data.toString();
+      if (options?.listeners?.stdout !== undefined) {
+        options.listeners.stdout(data);
+      } else {
+        // if no stdout listener was originally defined then we match default behavior of Toolrunner
+        process.stdout.write(data);
+      }
+    },
+    stderr: (data: Buffer) => {
+      stderr += data.toString();
+      if (options?.listeners?.stderr !== undefined) {
+        options.listeners.stderr(data);
+      } else {
+        // if no stderr listener was originally defined then we match default behavior of Toolrunner
+        process.stderr.write(data);
+      }
+    },
+  };
+
+  // we capture the original return code or error so that if no match is found we can duplicate the behavior
+  let returnState: Error | number;
+  try {
+    returnState = await new toolrunnner.ToolRunner(commandLine, args, {
+      ...options, // we want to override the original options, so include them first
+      listeners,
+      ignoreReturnCode: true, // so we can check for specific codes using the matchers
+    }).exec();
+  } catch (e) {
+    returnState = e;
+  }
+
+  // if there is a zero return code then we do not apply the matchers
+  if (returnState === 0) return returnState;
+
+  if (matchers) {
+    for (const matcher of matchers) {
+      if (
+        matcher.exitCode === returnState ||
+        matcher.outputRegex?.test(stderr) ||
+        matcher.outputRegex?.test(stdout)
+      ) {
+        throw new Error(matcher.message);
+      }
+    }
+  }
+
+  if (typeof returnState === "number") {
+    // only if we were instructed to ignore the return code do we ever return it non-zero
+    if (options?.ignoreReturnCode) {
+      return returnState;
+    } else {
+      throw new Error(
+        `The process \'${commandLine}\' failed with exit code ${returnState}`
+      );
+    }
+  } else {
+    throw returnState;
+  }
+}