mocha_test example

[alexeagle]

This allows a mocha_test macro to be neatly written - otherwise it got inputs like wksp_name/./file.js

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature (please, look at the "Scope of the project" section in the README.md file)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[fredrikredflag]

@alexeagle Is this just and example on how to run mocha_test or an implmentation as well?

Based on this I looked in mocha's github repo and couldn't find any Bazel rules:
load("@npm//mocha:index.bzl", "mocha_test")

Just curious :)

[alexeagle]

@fredrikredflag the new model for rules_nodejs is that we automatically generate rules for any npm package containing a bin entry.

So the @npm//mocha:index.bzl is a generated file that exposes the mocha bin as a mocha_test rule, by using a macro to wrap a nodejs_binary. If you were to bazel query --output=build you would see how that macro is expanded.

So yeah this PR is an example and also illustrates that no implementation is required.

(however we have to figure out what to do about Windows, this currently relies on Bazel's runfiles symlink tree. Asking @meteorcloudy for some advice there)

[fredrikredflag]

@alexeagle excellent, thanks! Then I can throw out our own mocha build rule hopefully.

Do you know if mocha will honor test_timeout if it's passed to the bazel command? Thinking for debugging. Right now we have issues with tests timing out when using node inspect and attach to it with interllij. Way out of scope for this issue maybe.

[alexeagle]

in this model, we run vanilla mocha, so it will behave the same as if you don't run it under Bazel.
Bazel will kill the nodejs process when the test_timeout expires which is approx. what you want, though the test framework won't log the useful things it might if it were triggering a timeout itself.

Timing out more gracefully should go on my list of reasons one might want to wrap a test runner with bazel logic.

[meteorcloudy]

@alexeagle I think you mentioned this problem in our last meeting, right? For Windows, is there a way to teach the binary about runfiles library? Can you give a simple example to reproduce this issue?

[alexeagle]

@meteorcloudy this example is about as simple as it gets. cd examples/webapp; bazel test :test_sourcemaps

We can't teach the binary about runfiles, because there are hundreds of binaries and we want them all to work.

Let's think about how this work work outside of Bazel. I have my sources, in some cases I want to execute against them directly, in some cases I run a transformation like the typescript compiler and want to execute against the output directory.

I think we can make this the same under Bazel by changing the working directory before running node, to either the directory containing the package input, or the output. Of course users will have to copy untransformed input files to the output tree, as you won't have a "union view" of them. But this matches how these tools work outside of Bazel.

If we were to take that approach, we should think about whether every node binary run under Bazel should have the working dir changed in this way.

[meteorcloudy]

@alexeagle That could be the solution.

I think we can make this the same under Bazel by changing the working directory before running node,

This could be achieved by a wrapper script?

Of course users will have to copy untransformed input files to the output tree, as you won't have a "union view" of them.

Maybe we can think of this copy as an no-op "transform", so we can implement it through a genrule or an Starlark action, then everything will be in the output directory. Question is how many files we need to copy? Is it going to be a performance issue?

[alexeagle]

Yes the node_launcher.sh should continue to adapt Bazel's environment to host node programs in the way they expect. I think setting the pwd is a reasonable thing for it to do, if we understand what is the right pwd to make node programs run the way they expect.

Copying files to the output dir will take time, but I'm less concerned about that since it's required outside of Bazel. I'm more concerned that Bazel makes it difficult to copy the file to the output directory under the same path. This is something we should discuss independently of this issue, for here let's assume I can always make an output dir with the right files (typically by creating a new subdirectory as the rule output and re-root files there)

[meteorcloudy]

@alexeagle When I was trying to reproduce at this PR I got this error:

pcloudy@pcloudy0-w MSYS ~/workspace/rules_nodejs/examples/webapp
$ bazel test :test_sourcemaps
INFO: Invocation ID: 8556510a-ce99-4c99-acf4-f777decf2f0e
INFO: Reading 'startup' options from c:\users\pcloudy\.bazelrc: --output_user_root=C:/src/tmp
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=320
INFO: Reading rc options for 'test' from c:\tools\msys64\home\pcloudy\workspace\rules_nodejs\common.bazelrc:
  Inherited 'common' options: --experimental_allow_incremental_repository_updates
INFO: Options provided by the client:
  Inherited 'build' options: --python_path=C:/Python36/python.exe
INFO: Reading rc options for 'test' from c:\tools\msys64\home\pcloudy\workspace\rules_nodejs\common.bazelrc:
  Inherited 'build' options: --symlink_prefix=dist/ --nolegacy_external_runfiles --incompatible_strict_action_env
INFO: Reading rc options for 'test' from c:\users\pcloudy\.bazelrc:
  Inherited 'build' options: --curses=yes --color=yes --verbose_failures --announce_rc --disk_cache=C:/src/tmp/bazel_disk_cache
INFO: Reading rc options for 'test' from c:\tools\msys64\home\pcloudy\workspace\rules_nodejs\common.bazelrc:
  'test' options: --test_output=errors
INFO: Reading rc options for 'test' from c:\users\pcloudy\.bazelrc:
  'test' options: -k --test_output=errors --build_tests_only --test_tag_filters -noci,-manual,-no_windows
ERROR: C:/tools/msys64/home/pcloudy/workspace/rules_nodejs/examples/webapp/BUILD.bazel:2:1: file '@npm//mocha:index.bzl' does not contain symbol 'mocha_test'
ERROR: C:/tools/msys64/home/pcloudy/workspace/rules_nodejs/examples/webapp/BUILD.bazel:34:1: name 'mocha_test' is not defined (did you mean 'java_test'?)
ERROR: Skipping ':test_sourcemaps': no such target '//:test_sourcemaps': target 'test_sourcemaps' not declared in package '' defined by C:/tools/msys64/home/pcloudy/workspace/rules_nodejs/examples/webapp/BUILD.bazel
WARNING: Target pattern parsing failed.
INFO: Analyzed 0 targets (0 packages loaded, 0 targets configured).
INFO: Found 0 test targets...
ERROR: command succeeded, but there were errors parsing the target pattern
INFO: Elapsed time: 0.695s, Critical Path: 0.01s
INFO: 0 processes.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully

[alexeagle]

@meteorcloudy sorry - there's a change here to create the *_test rules, so you have to run bazel test examples:examples_webapp so that the dependency is built. (the WORKSPACE file in that directory hard-codes the prior release of rules_nodejs)

[alexeagle]

I'm going to tag the mocha_test with no-bazelci-windows to get this green. That will make it easier to repro on Windows as well, after the next release. Figuring out how to avoid runfiles is a general problem.

[gregmagolan]

🥇

[meteorcloudy]

@alexeagle Thanks! I successfully reproduced the failure. And when I add --enable_runfiles, the test passed. So it's indeed the runfiles problem. Can you give me an example of how this works outside of Bazel?

[alexeagle]

Outside of Bazel a developer would do something like
case 1) source dir

cat my.js my.spec.js
mocha *.spec.js

which suggests we wanted the test to have a working dir of WORKSPACE/package rather than $TEST_SRCDIR/workspace

case 2) dist or build dir

tsc --outDir=dist my.ts my.spec.ts
mocha dist/*.spec.js

this is true for a usecase like bazel run :server where the server target is something like

load("@npm//http-server:index.bzl", "http_server")
http_server(name = "server", templated_args=[package_name()])

again you either wanted the working dir to be in the sources or in the dist folder.

In short, there is no concept of a "union view" over sources and outputs, you run in either one of the two directories. If you have a dist folder, you must copy all needed sources there.

I'll document --enable_runfiles for now and merge this, which will get us some part of the way there. I wonder if we have any recent data about how many developers don't have the version of Windows and the permissions required to have file symlinks.

Let's follow-up in another place.