Script runner perf optimization (#473)

* Fix buidler runScript() '--inspect' flag so it can properly work with the debugger.

* Script runner omit re-compiling with ts-node, unless requesting a TS script.

    * This way the run task will run much faster,
    the same goes for tests that otherwise can even timeout.

* Fix script runner failing test, stop skipping on Windows, remove unneeded timeout.

* Parallelize runScript() calls in scripts-runner tests for a faster execution time.

* Add JSDocs explaining why enforce '--inspect' VS '--inspect-brk=' in runScript().

Author: tmilar

Diff for: packages/buidler-core/src/internal/util/scripts-runner.ts

@@ -16,9 +16,11 @@ export async function runScript(
   const { fork } = await import("child_process");
 
   return new Promise((resolve, reject) => {
+    const processExecArgv = withFixedInspectArg(process.execArgv);
+
     const nodeArgs = [
-      ...process.execArgv,
-      ...getTsNodeArgsIfNeeded(),
+      ...processExecArgv,
+      ...getTsNodeArgsIfNeeded(scriptPath),
       ...extraNodeArgs
     ];
 
@@ -46,26 +48,79 @@ export async function runScriptWithBuidler(
 ): Promise<number> {
   log(`Creating Buidler subprocess to run ${scriptPath}`);
 
+  const buidlerRegisterPath = resolveBuidlerRegisterPath();
+
   return runScript(
     scriptPath,
     scriptArgs,
-    [
-      ...extraNodeArgs,
-      "--require",
-      path.join(__dirname, "..", "..", "register")
-    ],
+    [...extraNodeArgs, "--require", buidlerRegisterPath],
     {
       ...getEnvVariablesMap(buidlerArguments),
       ...extraEnvVars
     }
   );
 }
 
-function getTsNodeArgsIfNeeded() {
+/**
+ * Fix debugger "inspect" arg from process.argv, if present.
+ *
+ * When running this process with a debugger, a debugger port
+ * is specified via the "--inspect-brk=" arg param in some IDEs/setups.
+ *
+ * This normally works, but if we do a fork afterwards, we'll get an error stating
+ * that the port is already in use (since the fork would also use the same args,
+ * therefore the same port number). To prevent this issue, we could replace the port number with
+ * a different free one, or simply use the port-agnostic --inspect" flag, and leave the debugger
+ * port selection to the Node process itself, which will pick an empty AND valid one.
+ *
+ * This way, we can properly use the debugger for this process AND for the executed
+ * script itself - even if it's compiled using ts-node.
+ */
+function withFixedInspectArg(argv: string[]) {
+  const fixIfInspectArg = (arg: string) => {
+    if (arg.toLowerCase().includes("--inspect-brk=")) {
+      return "--inspect";
+    }
+    return arg;
+  };
+  return argv.map(fixIfInspectArg);
+}
+
+/**
+ * Ensure buidler/register source file path is resolved to compiled JS file
+ * instead of TS source file, so we don't need to run ts-node unnecessarily.
+ */
+export function resolveBuidlerRegisterPath() {
+  const executionMode = getExecutionMode();
+  const isCompiledInstallation = [
+    ExecutionMode.EXECUTION_MODE_LOCAL_INSTALLATION,
+    ExecutionMode.EXECUTION_MODE_GLOBAL_INSTALLATION,
+    ExecutionMode.EXECUTION_MODE_LINKED
+  ].includes(executionMode);
+
+  const buidlerCoreBaseDir = path.join(__dirname, "..", "..");
+
+  const buidlerCoreCompiledDir = isCompiledInstallation
+    ? buidlerCoreBaseDir
+    : path.join(buidlerCoreBaseDir, "..");
+
+  const buidlerCoreRegisterCompiledPath = path.join(
+    buidlerCoreCompiledDir,
+    "register"
+  );
+
+  return buidlerCoreRegisterCompiledPath;
+}
+
+function getTsNodeArgsIfNeeded(scriptPath: string) {
   if (getExecutionMode() !== ExecutionMode.EXECUTION_MODE_TS_NODE_TESTS) {
     return [];
   }
 
+  if (!/\.tsx?$/i.test(scriptPath)) {
+    return [];
+  }
+
   const extraNodeArgs = [];
 
   if (!process.execArgv.includes("ts-node/register")) {

Diff for: packages/buidler-core/test/internal/util/scripts-runner.ts

@@ -1,8 +1,7 @@
 import { assert } from "chai";
-import os from "os";
-import path from "path";
 
 import {
+  resolveBuidlerRegisterPath,
   runScript,
   runScriptWithBuidler
 } from "../../../src/internal/util/scripts-runner";
@@ -13,12 +12,18 @@ describe("Scripts runner", function() {
   useFixtureProject("project-with-scripts");
 
   it("Should pass params to the script", async function() {
-    const statusCode = await runScript("./params-script.js", ["a", "b", "c"]);
-    assert.equal(statusCode, 0);
+    const [
+      statusCodeWithScriptParams,
+      statusCodeWithNoParams
+    ] = await Promise.all([
+      runScript("./params-script.js", ["a", "b", "c"]),
+      runScript("./params-script.js")
+    ]);
+
+    assert.equal(statusCodeWithScriptParams, 0);
 
     // We check here that the script is correctly testing this:
-    const statusCode2 = await runScript("./params-script.js");
-    assert.notEqual(statusCode2, 0);
+    assert.notEqual(statusCodeWithNoParams, 0);
   });
 
   it("Should run the script to completion", async function() {
@@ -30,54 +35,90 @@ describe("Scripts runner", function() {
   });
 
   it("Should resolve to the status code of the script run", async function() {
-    this.timeout(35000);
-
-    if (os.type() === "Windows_NT") {
-      this.skip();
-    }
-
-    const statusCode1 = await runScript(
-      "./async-script.js",
-      [],
-      ["--require", path.join(__dirname, "..", "..", "..", "src", "register")]
+    const buidlerRegisterPath = resolveBuidlerRegisterPath();
+
+    const extraNodeArgs = ["--require", buidlerRegisterPath];
+    const scriptArgs: string[] = [];
+
+    const runScriptCases = [
+      {
+        scriptPath: "./async-script.js",
+        extraNodeArgs,
+        expectedStatusCode: 0
+      },
+      {
+        scriptPath: "./failing-script.js",
+        expectedStatusCode: 123
+      },
+      {
+        scriptPath: "./successful-script.js",
+        extraNodeArgs,
+        expectedStatusCode: 0
+      }
+    ];
+
+    const runScriptTestResults = await Promise.all(
+      runScriptCases.map(
+        async ({ scriptPath, extraNodeArgs: _extraNodeArgs }) => {
+          const statusCode =
+            _extraNodeArgs === undefined
+              ? await runScript(scriptPath)
+              : await runScript(scriptPath, scriptArgs, _extraNodeArgs);
+          return { scriptPath, statusCode };
+        }
+      )
     );
-    assert.equal(statusCode1, 0);
-
-    const statusCode2 = await runScript("./failing-script.js");
-    assert.equal(statusCode2, 123);
 
-    const statusCode3 = await runScript(
-      "./successful-script.js",
-      [],
-      ["--require", path.join(__dirname, "..", "..", "..", "src", "register")]
+    const expectedResults = runScriptCases.map(
+      ({ expectedStatusCode, scriptPath }) => ({
+        scriptPath,
+        statusCode: expectedStatusCode
+      })
     );
-    assert.equal(statusCode3, 0);
+
+    assert.deepEqual(runScriptTestResults, expectedResults);
   });
 
   it("Should pass env variables to the script", async function() {
-    const statusCode = await runScript("./env-var-script.js", [], [], {
-      TEST_ENV_VAR: "test"
-    });
-    assert.equal(statusCode, 0);
+    const [statusCodeWithEnvVars, statusCodeWithNoEnvArgs] = await Promise.all([
+      runScript("./env-var-script.js", [], [], {
+        TEST_ENV_VAR: "test"
+      }),
+      runScript("./env-var-script.js")
+    ]);
+
+    assert.equal(
+      statusCodeWithEnvVars,
+      0,
+      "Status code with env vars should be 0"
+    );
 
-    // We check here that the script is correctly testing this:
-    const statusCode2 = await runScript("./env-var-script.js");
-    assert.notEqual(statusCode2, 0);
+    assert.notEqual(
+      statusCodeWithNoEnvArgs,
+      0,
+      "Status code with no env vars should not be 0"
+    );
   });
 
   describe("runWithBuidler", function() {
     useEnvironment();
 
     it("Should load buidler/register successfully", async function() {
-      const statusCode = await runScriptWithBuidler(
-        this.env.buidlerArguments,
-        "./successful-script.js"
-      );
-      assert.equal(statusCode, 0);
+      const [
+        statusCodeWithBuidler,
+        statusCodeWithoutBuidler
+      ] = await Promise.all([
+        runScriptWithBuidler(
+          this.env.buidlerArguments,
+          "./successful-script.js"
+        ),
+        runScript("./successful-script.js")
+      ]);
+
+      assert.equal(statusCodeWithBuidler, 0);
 
       // We check here that the script is correctly testing this:
-      const statusCode2 = await runScript("./successful-script.js");
-      assert.notEqual(statusCode2, 0);
+      assert.notEqual(statusCodeWithoutBuidler, 0);
     });
 
     it("Should forward all the buidler arguments", async function() {