Regression in 2.0.0 autogenerated test rules

[WesleyYue]

🐞 bug report

Is this a regression?

Yes, this works in 1.6.1

Description

It looks like this bug regressed in the 2.0.0 release candidates.

🔬 Minimal Reproduction

Repo to repro here.

🔥 Exception or Error

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //:test
-----------------------------------------------------------------------------
FAIL ./lib.test.js
  ● Test suite failed to run

    Cannot find module 'jest_repro/lib' from 'lib.test.js'

      at Resolver.resolveModule (../../../../../node_modules/jest-resolve/build/index.js:259:17)
      at ../../../lib.test.ts:1:1
      at lib.test.js:3:13
      at Object. (lib.test.js:8:3)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        8.091s
Ran all test suites within paths "./lib.test.js".

🌍 Your Environment

Operating System:

  
Ubuntu 18.04.4 LTS
  

Output of bazel version:

  
Build label: 3.1.0- (@non-git)
Build target: bazel-out/aarch64-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Jul 7 00:47:49 2020 (1594082869)
Build timestamp: 1594082869
Build timestamp as int: 1594082869
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
2.0.0-rc.1
  

Anything else relevant?
I'm building on an aarch64 platform.

[alexeagle]

We removed the test that had earlier been added to prevent such a regression
5f0df3a#diff-a3a949474a86b529e37ebf2ad918ff65

[gregmagolan]

That test was intentionally removed for 2.0 as it relied on the workspace name being implicitly linkable for absolute imports such as workspace_name/path/to/thing.

For 2.0, you now have to explicitly make a pkg_npm in the root BUILD file with a package_name that matches the workspace name and add the things you want importable.

To get the rhttps://github.com/WesleyYue/ts-jest-repro above working here is the change:

ts-jest-repro gregmagolan$ git diff
diff --git a/BUILD.bazel b/BUILD.bazel
index 2af2cfb..e3ae61e 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@npm//@bazel/typescript:index.bzl", "ts_library")
+load("@build_bazel_rules_nodejs//:index.bzl", "pkg_npm")
 load("//:ts_jest_test.bzl", "ts_jest_test")
 
 ts_library(
@@ -10,6 +11,14 @@ ts_library(
     ],
 )
 
+pkg_npm(
+    # Declare that the root folder is an npm package named "jest_repro"
+    # so it can be imported as such
+    name = "jest_repro",
+    package_name = "jest_repro",
+    deps = [":lib"],
+)
+
 ts_jest_test(
     name = "test",
     srcs = [
@@ -19,4 +28,7 @@ ts_jest_test(
     deps = [
         ":lib",
     ],
+    data = [
+        ":jest_repro",
+    ]
 )

with that it works

ts-jest-repro gregmagolan$ bazel test ...
DEBUG: /private/var/tmp/_bazel_gregmagolan/9151342612d902873da5c6f77a0c760b/external/npm/install_bazel_dependencies.bzl:5:9: 
NOTICE: install_bazel_dependencies is no longer needed,
since @bazel/* npm packages can be load()ed without copying to another repository.
See https://github.com/bazelbuild/rules_nodejs/issues/1877

install_bazel_dependencies is harmful because it causes npm_install/yarn_install to run even
if the requested output artifacts for the build don't require nodejs, making multi-language monorepo
use cases slower.

You should be able to remove install_bazel_workspaces from your WORKSPACE file unless you depend
on a package that exposes a separate repository, like @angular/bazel exposes @npm_angular_bazel//:index.bzl

You can suppress this message by passing "suppress_warning = True" to install_bazel_dependencies()
INFO: Analyzed 9 targets (1 packages loaded, 9 targets configured).
INFO: Found 8 targets and 1 test target...
INFO: Elapsed time: 0.848s, Critical Path: 0.03s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
//:test                                                         (cached) PASSED in 1.8s

Executed 0 out of 1 test: 1 test passes.
INFO: Build completed successfully, 1 total action

[gregmagolan]

We updated the 2.0 upgrade wiki to add a little more guidance on this: https://github.com/bazelbuild/rules_nodejs/wiki#no-longer-link-the-workspace-root

[WesleyYue]

Is there a better pattern to use if :lib and :test are deep inside a monorepo? It seems silly to explicitly enumerate every dependency that I want to include under every ts_jest_test in my monorepo at the root BUILD npm_pkg rule and make them publicly visible when the ts_jest_test rules (:test) just need to depend on the neighboring ts_library rules (:lib).

[alexeagle]

Why do you need more than one entry in the root build file? I think I'm missing something about the shape of the graph. Could you make a little repro where there are multiple things that have to be registered?

[WesleyYue]

Could you make a little repro where there are multiple things that have to be registered?

Doesn't that defeat the point of using Bazel and a monorepo paradigm? Or am I misunderstanding what you mean? Imagine I have an org-wide monorepo with typescript tests that are unrelated from different teams, now these teams would need to mix and add the dependencies for all of their unrelated tests to the same root npm_pkg.

[alexeagle]

Yes, the repro I was asking for is one that exhibits the problem you're describing. I agree there should not be a central point where all the deps in a monorepo need to get registered.

I think this was fixed in 2.2.0 with

To fix the hardest migration problem with the 2.0 release, we added back a way to link the workspace root into your node_modules, and added it to nodejs_binary, npm_package_bin, rollup_bundle, ts_project & ts_library (4dcb37f)