feat: add default DEBUG and VERBOSE_LOGS configuration_env_vars to no…

…dejs_binary

- all rules updated to use DEBUG and VERBOSE_LOGS environment variables
- added golden_debug attribute golden_file_test to support the case where a rule has different output if DEBUG is set; for example
```
golden_file_test(
    name = "test",
    actual = "out.min.js",
    golden = "output.golden.js_",
    golden_debug = "output.debug.golden.js_",
)
```

.bazelci/presubmit.yml

@@ -106,6 +106,16 @@ tasks:
     - "--test_tag_filters=-e2e,-examples,-fix-bazelci-ubuntu"
     test_targets:
     - "//..."
+  ubuntu1804_debug:
+    name: ubuntu1804_debug
+    platform: ubuntu1804
+    test_flags:
+    - "--define=VERBOSE_LOGS=1"
+    - "--define=DEBUG=1"
+    - "--test_tag_filters=-e2e,-examples,-fix-bazelci-ubuntu"
+    test_targets:
+    - "//..."
+    - "//packages/terser/test/debug:test_define_DEBUG"
   ubuntu1804_e2e:
     name: ubuntu1804_e2e
     platform: ubuntu1804

.circleci/config.yml

@@ -152,7 +152,7 @@ jobs:
       - *init_bazel
       - *hide_node_and_yarn_local_binaries
 
-      - run: bazel test ... --test_tag_filters=-e2e,-examples
+      - run: bazel test --test_tag_filters=-e2e,-examples ...
 
       - store_artifacts:
           path: dist/bin/release.tar.gz

DEVELOPING.md

@@ -24,6 +24,42 @@ To do a full clean run:
 yarn clean_all
 ```
 
+## Debugging
+
+Adding the `--config=debug` command line option will set a number of flags that are useful for debugging.
+See the `Support for debugging NodeJS tests` section of `common.bazelrc` for the full list of flags that are set with `--config=debug`.
+
+Use  `--config=debug` with `bazel test` as follow,
+
+```
+bazel test --config=debug //test:...
+```
+
+or with `bazel run`,
+
+```
+bazel run --config=debug //test:test1
+```
+
+to also turn on the NodeJS inspector agent which will break before any user code starts. You should then see,
+
+```
+Executing tests from //test:test1
+-----------------------------------------------------------------------------
+Debugger listening on ws://127.0.0.1:9229/3f20777a-242c-4d18-b88b-5ed4b3fed61c
+For help, see: https://nodejs.org/en/docs/inspector
+```
+
+when the test is run.
+
+To inspect with Chrome DevTools 55+, open `chrome://inspect` in a Chromium-based browser and attach to the waiting process.
+A Chrome DevTools window should open and you should see `Debugger attached.` in the console.
+
+See https://nodejs.org/en/docs/guides/debugging-getting-started/ for more details.
+
+If developing outside of this repo, add the options in the `Support for debugging NodeJS tests` section of `common.bazelrc` in
+this repo to your project's `.bazelrc` file to add the above support for debugging NodeJS programs.
+
 ## Releasing
 
 Start from a clean checkout at master/HEAD.

common.bazelrc

@@ -26,7 +26,30 @@ test --test_output=errors
 
 # Support for debugging NodeJS tests
 # Add the Bazel option `--config=debug` to enable this
-test:debug --test_arg=--node_options=--inspect-brk --test_output=streamed --test_strategy=exclusive --test_timeout=9999 --nocache_test_results
+# --test_output=streamed
+#     Stream stdout/stderr output from each test in real-time.
+#     See https://docs.bazel.build/versions/master/user-manual.html#flag--test_output for more details.
+# --test_strategy=exclusive
+#     Run one test at a time.
+# --test_timeout=9999
+#     Prevent long running tests from timing out
+#     See https://docs.bazel.build/versions/master/user-manual.html#flag--test_timeout for more details.
+# --nocache_test_results
+#     Always run tests
+# --test_arg=--node_options=--inspect-brk
+#     Pass the --inspect-brk option to all tests which enables the node inspector agent.
+#     See https://nodejs.org/de/docs/guides/debugging-getting-started/#command-line-options for more details.
+# --define=VERBOSE_LOGS=1
+#     Rules will output verbose logs if the VERBOSE_LOGS environment variable is set.
+# --define=DEBUG=1
+#     Rules may change their build outputs if the DEBUG environment variable is set. For example,
+#     mininfiers such as terser may make their output more human readable when this is set.
+test:debug --test_output=streamed --test_strategy=exclusive --test_timeout=9999 --nocache_test_results --define=VERBOSE_LOGS=1
+# Use bazel run with `--config=debug` to turn on the NodeJS inspector agent.
+# The node process will break before user code starts and wait for the debugger to connect.
+run:debug --define=VERBOSE_LOGS=1 -- --node_options=--inspect-brk
+# The following option will change the build output of certain rules such as terser and may not be desirable in all cases
+build:debug --define=DEBUG=1
 
 # Turn off legacy external runfiles
 # This prevents accidentally depending on this feature, which Bazel will remove.

docs/Terser.md

@@ -87,7 +87,7 @@ Bazel will make a copy of your config file, treating it as a template.
 
 If you use the magic strings `"bazel_debug"` or `"bazel_no_debug"`, these will be
 replaced with `true` and `false` respecting the value of the `debug` attribute
-or the `--define=DEBUG=true` bazel flag.
+or the `--define=DEBUG=1` bazel flag.
 
 For example,
 
@@ -107,7 +107,7 @@ If `config_file` isn't supplied, Bazel will use a default config file.
 (*Boolean*): Configure terser to produce more readable output.
 
 Instead of setting this attribute, consider setting the DEBUG variable instead
-bazel build --define=DEBUG=true //my/terser:target
+bazel build --define=DEBUG=1 //my/terser:target
 so that it only affects the current build.
 
 

internal/bazel_integration_test/test_runner.js

@@ -15,8 +15,6 @@
  * limitations under the License.
  */
 
-const DEBUG = false;
-
 // Set TEST_MANIFEST to true and use `bazel run` to excersize the MANIFEST
 // file code path on Linux and OSX
 const TEST_MANIFEST = false;
@@ -26,17 +24,28 @@ const fs = require('fs');
 const path = require('path');
 const tmp = require('tmp');
 
+const DEBUG = !!process.env['DEBUG'];
+const VERBOSE_LOGS = !!process.env['VERBOSE_LOGS'];
+
+function log(...m) {
+  console.error('[test_runner.js]', ...m);
+}
+
+function log_verbose(...m) {
+  if (VERBOSE_LOGS) console.error('[test_runner.js]', ...m);
+}
+
 const config = require(process.argv[2]);
-if (DEBUG) console.log(`config: ${JSON.stringify(config, null, 2)}`);
+log_verbose(`config: ${JSON.stringify(config, null, 2)}`);
 
 const testArgs = process.argv.slice(3);
-if (DEBUG) console.log(`testArgs: ${JSON.stringify(testArgs, null, 2)}`);
+log_verbose(`testArgs: ${JSON.stringify(testArgs, null, 2)}`);
 
 /**
- * Helper function to log out the contents of a file.
+ * Helper function to debug log out the contents of a file.
  */
 function logFileContents(desc, contents) {
-  console.log(`\n\n${
+  log_verbose(`${
       desc}\n========================================================================================\n${
       contents}\n^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n`);
 }
@@ -82,7 +91,7 @@ function copyFolderSync(from, to) {
     if (fs.statSync(src).isFile()) {
       mkdirp(path.dirname(dest));
       fs.copyFileSync(src, dest);
-      if (DEBUG) console.log(`copying ${src} -> ${dest}`);
+      log_verbose(`copying ${src} -> ${dest}`);
     } else {
       copyFolderSync(src, dest);
     }
@@ -130,7 +139,7 @@ function copyWorkspace(workspace) {
         const element = key.slice(start.length);
         const dest = path.posix.join(to, element);
         mkdirp(path.dirname(dest));
-        if (DEBUG) console.log(`copying (MANIFEST) ${RUNFILES_MANIFEST[key]} -> ${dest}`);
+        log_verbose(`copying (MANIFEST) ${RUNFILES_MANIFEST[key]} -> ${dest}`);
         fs.copyFileSync(RUNFILES_MANIFEST[key], dest);
       }
     }
@@ -158,7 +167,7 @@ function copyNpmPackage(packagePath) {
   return to;
 }
 
-if (DEBUG) console.log(`\n\ncopying workspace under test ${config.workspaceUnderTest} to tmp`);
+log_verbose(`copying workspace under test ${config.workspaceUnderTest} to tmp`);
 const workspaceRoot = copyWorkspace(config.workspaceUnderTest);
 
 // Handle .bazelrc import replacements
@@ -172,7 +181,7 @@ if (bazelrcImportsKeys.length && isFile(bazelrcFile)) {
     bazelrcContents = bazelrcContents.replace(importKey, importContents);
   }
   fs.writeFileSync(bazelrcFile, bazelrcContents);
-  if (DEBUG) logFileContents('.bazelrc file with replacements:', bazelrcContents);
+  logFileContents('.bazelrc file with replacements:', bazelrcContents);
 }
 
 // Handle appending to .bazelrc
@@ -182,7 +191,7 @@ if (config.bazelrcAppend) {
   bazelrcContents += '\n\n# Appended by bazel_integration_test\n';
   bazelrcContents += config.bazelrcAppend;
   fs.writeFileSync(bazelrcFile, bazelrcContents);
-  if (DEBUG) logFileContents('.bazelrc file after appending:', bazelrcContents);
+  logFileContents('.bazelrc file after appending:', bazelrcContents);
 }
 
 // Handle WORKSPACE replacements
@@ -206,7 +215,7 @@ if (config.bazelrcAppend) {
     }
   }
   fs.writeFileSync(workspaceFile, workspaceContents);
-  if (DEBUG) logFileContents('WORKSPACE file with replacements:', workspaceContents);
+  logFileContents('WORKSPACE file with replacements:', workspaceContents);
 }
 
 // Handle package.json replacements
@@ -217,7 +226,7 @@ if (isFile(packageJsonFile)) {
   const npmPackageKeys = Object.keys(config.npmPackages);
   if (npmPackageKeys.length) {
     for (const packageJsonKey of npmPackageKeys) {
-      if (DEBUG) console.log(`\n\ncopying npm package ${packageJsonKey} to tmp`);
+      log_verbose(`copying npm package ${packageJsonKey} to tmp`);
       const packagePath = copyNpmPackage(config.npmPackages[packageJsonKey]).replace(/\\/g, '/');
       const regex = new RegExp(`\"${packageJsonKey}\"\\s*\:\\s*\"[^"]+`)
       const replacement = `"${packageJsonKey}": "file:${packagePath}`;
@@ -255,21 +264,21 @@ if (isFile(packageJsonFile)) {
     }
   }
 
-  if (DEBUG) logFileContents('package.json file with replacements:', packageJsonContents);
+  logFileContents('package.json file with replacements:', packageJsonContents);
 }
 
 const isWindows = process.platform === 'win32';
 const bazelBinary =
     require.resolve(`${config.bazelBinaryWorkspace}/bazel${isWindows ? '.exe' : ''}`);
 
-console.log(`\n\nRunning 'bazel version'`);
+log(`running 'bazel version'`);
 let spawnedProcess = spawnSync(bazelBinary, ['version'], {cwd: workspaceRoot, stdio: 'inherit'});
 if (spawnedProcess.status) {
   process.exit(spawnedProcess.status);
 }
 
-if (DEBUG) {
-  console.log(`\n\nRunning 'bazel info'`);
+if (VERBOSE_LOGS) {
+  log_verbose(`running 'bazel info'`);
   spawnedProcess = spawnSync(bazelBinary, ['info'], {cwd: workspaceRoot, stdio: 'inherit'});
   if (spawnedProcess.status) {
     process.exit(spawnedProcess.status);
@@ -286,7 +295,7 @@ for (const bazelCommand of config.bazelCommands) {
   } else {
     bazelArgs.push(...testArgs);
   }
-  console.log(`\n\nRunning 'bazel ${bazelArgs.join(' ')}'`);
+  log(`running 'bazel ${bazelArgs.join(' ')}'`);
   spawnedProcess = spawnSync(bazelBinary, bazelArgs, {cwd: workspaceRoot, stdio: 'inherit'});
   if (spawnedProcess.status) {
     process.exit(spawnedProcess.status);

internal/golden_file_test/bin.js

@@ -3,7 +3,9 @@ const path = require('path');
 const unidiff = require('unidiff');
 
 function main(args) {
-  const [mode, golden, actual] = args;
+  const [mode, golden_no_debug, golden_debug, actual] = args;
+  const debug = !!process.env['DEBUG'];
+  const golden = debug ? golden_debug : golden_no_debug;
   const actualContents = fs.readFileSync(require.resolve(actual), 'utf-8').replace(/\r\n/g, '\n');
   const goldenContents = fs.readFileSync(require.resolve(golden), 'utf-8').replace(/\r\n/g, '\n');
 
@@ -22,7 +24,8 @@ ${prettyDiff}
       
 Update the golden file:
 
-            bazel run ${process.env['BAZEL_TARGET'].replace(/_bin$/, '')}.accept
+            bazel run ${debug ? '--define=DEBUG=1 ' : ''}${
+          process.env['BAZEL_TARGET'].replace(/_bin$/, '')}.accept
 `);
     } else {
       throw new Error('unknown mode', mode);

internal/golden_file_test/golden_file_test.bzl

@@ -3,13 +3,23 @@
 load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary", "nodejs_test")
 
 def golden_file_test(name, golden, actual, **kwargs):
+    """Tests an actual output against a golden output.
+
+Use `golden_debug` if the actual output changes when DEBUG is set.
+"""
     data = [golden, actual, "@npm//unidiff"]
 
+    golden_debug = kwargs.pop("golden_debug", [])
+    if golden_debug:
+        data.extend([golden_debug])
+    else:
+        golden_debug = golden
+
     loc = "$(location %s)"
     nodejs_test(
         name = name,
         entry_point = "@build_bazel_rules_nodejs//internal/golden_file_test:bin.js",
-        templated_args = ["--verify", loc % golden, loc % actual],
+        templated_args = ["--verify", loc % golden, loc % golden_debug, loc % actual],
         data = data,
         **kwargs
     )
@@ -18,7 +28,7 @@ def golden_file_test(name, golden, actual, **kwargs):
         name = name + ".accept",
         testonly = True,
         entry_point = "@build_bazel_rules_nodejs//internal/golden_file_test:bin.js",
-        templated_args = ["--out", loc % golden, loc % actual],
+        templated_args = ["--out", loc % golden, loc % golden_debug, loc % actual],
         data = data,
         **kwargs
     )

internal/node/node.bzl

@@ -156,7 +156,7 @@ def _nodejs_binary_impl(ctx):
             ctx.outputs.loader.short_path,
         ])
     env_vars = "export BAZEL_TARGET=%s\n" % ctx.label
-    for k in ctx.attr.configuration_env_vars:
+    for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars:
         if k in ctx.var.keys():
             env_vars += "export %s=\"%s\"\n" % (k, ctx.var[k])
 
@@ -269,6 +269,19 @@ _NODEJS_EXECUTABLE_ATTRS = {
         allow_files = True,
         aspects = [sources_aspect, module_mappings_runtime_aspect, collect_node_modules_aspect],
     ),
+    "default_env_vars": attr.string_list(
+        doc = """Default environment variables that are added to `configuration_env_vars`.
+
+This is separate from the default of `configuration_env_vars` so that a user can set `configuration_env_vars`
+without losing the defaults that should be set in most cases.
+
+The set of default  environment variables is:
+
+`DEBUG`: rules use this environment variable to turn on debug information in their output artifacts
+`VERBOSE_LOGS`: rules use this environment variable to turn on debug output in their logs
+""",
+        default = ["DEBUG", "VERBOSE_LOGS"],
+    ),
     "entry_point": attr.label(
         doc = """The script which should be executed first, usually containing a main function.