fix(jasmine): can not reference runner when exports_directories_only=… (#3293)

thesayyn · alexeagle · web-flow · commit 0be0eeb12bd5 · 2022-01-27T06:09:38.000-08:00
* fix(jasmine): can not reference runner when exports_directories_only=True

* chore: code review comments

Co-authored-by: Alex Eagle &lt;alex@aspect.dev&gt;
diff --git a/docs/Jasmine.md b/docs/Jasmine.md
@@ -98,14 +98,17 @@ Defaults to `None`
 <h4 id="jasmine_node_test-jasmine">jasmine</h4>
 
 A label providing the `@bazel/jasmine` npm dependency.
+Intended for internal use only.
 
-Defaults to `"@npm//@bazel/jasmine"`
+Defaults to `None`
 
 <h4 id="jasmine_node_test-jasmine_entry_point">jasmine_entry_point</h4>
 
 A label providing the `@bazel/jasmine` entry point.
+This is a custom wrapper which adds features like sharding and ibazel support.
+Intended for internal use only.
 
-Defaults to `Label("@npm//@bazel/jasmine:jasmine_runner.js")`
+Defaults to `None`
 
 <h4 id="jasmine_node_test-kwargs">kwargs</h4>
 
diff --git a/examples/nestjs/WORKSPACE b/examples/nestjs/WORKSPACE
@@ -30,7 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
-    exports_directories_only = False,
+    exports_directories_only = True,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -386,7 +386,8 @@ async function generatePackageBuildFiles(pkg: Dep) {
   // If there's an index.bzl in the package then copy all the package's files
   // other than the BUILD file which we'll write below.
   // (maybe we shouldn't copy .js though, since it belongs under node_modules?)
-  if (pkg._files.includes('index.bzl')) {
+  const hasIndexBzl = pkg._files.includes('index.bzl')
+  if (hasIndexBzl) {
     await pkg._files.filter(f => f !== 'BUILD' && f !== 'BUILD.bazel').reduce(async (prev, file) => {
       if (/^node_modules[/\\]/.test(file)) {
         // don't copy over nested node_modules
@@ -406,16 +407,23 @@ async function generatePackageBuildFiles(pkg: Dep) {
       await mkdirp(path.dirname(destFile));
       await fs.copyFile(src, destFile);
     }, Promise.resolve());
-  } else {
+  } 
+
+
     const indexFile = printIndexBzl(pkg);
     if (indexFile.length) {
-      await writeFile(path.posix.join(pkg._dir, 'index.bzl'), indexFile);
-      buildFile += `
+      await writeFile(path.posix.join(pkg._dir, hasIndexBzl ? 'private' : '', 'index.bzl'), indexFile);
+      const buildContent = `
 # For integration testing
 exports_files(["index.bzl"])
 `;
+      if (hasIndexBzl) {
+        await writeFile(path.posix.join(pkg._dir, 'private', 'BUILD.bazel'), buildContent);
+      } else {
+        buildFile += buildContent;
+      }
     }
-  }
+  
 
   if (!symlinkBuildFile) {
     await writeFile(
diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js
@@ -249,7 +249,8 @@ async function generatePackageBuildFiles(pkg) {
             await writeFile(path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), generateBuildFileHeader(visibility) + binBuildFile);
         }
     }
-    if (pkg._files.includes('index.bzl')) {
+    const hasIndexBzl = pkg._files.includes('index.bzl');
+    if (hasIndexBzl) {
         await pkg._files.filter(f => f !== 'BUILD' && f !== 'BUILD.bazel').reduce(async (prev, file) => {
             if (/^node_modules[/\\]/.test(file)) {
                 return;
@@ -266,14 +267,18 @@ async function generatePackageBuildFiles(pkg) {
             await fs_1.promises.copyFile(src, destFile);
         }, Promise.resolve());
     }
-    else {
-        const indexFile = printIndexBzl(pkg);
-        if (indexFile.length) {
-            await writeFile(path.posix.join(pkg._dir, 'index.bzl'), indexFile);
-            buildFile += `
+    const indexFile = printIndexBzl(pkg);
+    if (indexFile.length) {
+        await writeFile(path.posix.join(pkg._dir, hasIndexBzl ? 'private' : '', 'index.bzl'), indexFile);
+        const buildContent = `
 # For integration testing
 exports_files(["index.bzl"])
 `;
+        if (hasIndexBzl) {
+            await writeFile(path.posix.join(pkg._dir, 'private', 'BUILD.bazel'), buildContent);
+        }
+        else {
+            buildFile += buildContent;
         }
     }
     if (!symlinkBuildFile) {
diff --git a/packages/jasmine/BUILD.bazel b/packages/jasmine/BUILD.bazel
@@ -39,7 +39,10 @@ stardoc(
     out = "README.md",
     input = "index.bzl",
     tags = ["fix-windows"],
-    deps = [":bzl"],
+    deps = [
+        ":bzl",
+        "//packages/jasmine/private:bzl",
+    ],
 )
 
 nodejs_test(
@@ -80,9 +83,6 @@ pkg_npm(
         "package.json",
     ],
     build_file_content = "",
-    substitutions = {
-        "//packages/jasmine:jasmine_runner.js": "//:node_modules/@bazel/jasmine/jasmine_runner.js",
-    },
     deps = [
         ":README.md",
         ":npm_version_check",
diff --git a/packages/jasmine/jasmine_node_test.bzl b/packages/jasmine/jasmine_node_test.bzl
@@ -19,6 +19,7 @@ than launching a test in Karma, for example.
 """
 
 load("@rules_nodejs//nodejs:providers.bzl", "JSModuleInfo")
+load("//packages/jasmine/private:index.bzl", "bazel_jasmine_runner_test")
 load("@build_bazel_rules_nodejs//internal/node:node.bzl", nodejs_test = "nodejs_test_macro")
 
 def _js_sources_impl(ctx):
@@ -66,10 +67,9 @@ def jasmine_node_test(
         tags = [],
         config_file = None,
         use_direct_specs = None,
-        # Replaced by pkg_npm with jasmine = "//@bazel/jasmine",
-        jasmine = "//packages/jasmine",
-        # Replaced by pkg_npm with jasmine_entry_point = "//:node_modules/@bazel/jasmine/jasmine_runner.js",
-        jasmine_entry_point = Label("//packages/jasmine:jasmine_runner.js"),
+        # TODO(6.0): remove these two attributes, users should never interact with them
+        jasmine = None,
+        jasmine_entry_point = None,
         **kwargs):
     """Runs tests in NodeJS using the Jasmine test runner.
 
@@ -103,7 +103,12 @@ def jasmine_node_test(
         More info: https://github.com/bazelbuild/rules_nodejs/pull/2576
 
       jasmine: A label providing the `@bazel/jasmine` npm dependency.
+        Intended for internal use only.
+
       jasmine_entry_point: A label providing the `@bazel/jasmine` entry point.
+        This is a custom wrapper which adds features like sharding and ibazel support.
+        Intended for internal use only.
+
       **kwargs: Remaining arguments are passed to the test rule
     """
     if kwargs.pop("coverage", False):
@@ -117,7 +122,10 @@ def jasmine_node_test(
         use_direct_specs = use_direct_specs,
     )
 
-    all_data = data + srcs + deps + [Label(jasmine)]
+    all_data = data + srcs + deps
+
+    if jasmine != None:
+        all_data.append(jasmine)
 
     # BEGIN-INTERNAL
     # Only used when running tests in the rules_nodejs repo.
@@ -142,13 +150,20 @@ def jasmine_node_test(
         pkg = Label("%s//%s:__pkg__" % (native.repository_name(), native.package_name()))
         all_data.append(pkg.relative(config_file))
 
-    nodejs_test(
+    kwargs = dict(
         name = name,
         data = all_data,
-        entry_point = jasmine_entry_point,
         templated_args = templated_args,
         testonly = 1,
         expected_exit_code = expected_exit_code,
         tags = tags,
         **kwargs
     )
+
+    if jasmine_entry_point:
+        nodejs_test(
+            entry_point = jasmine_entry_point,
+            **kwargs
+        )
+    else:
+        bazel_jasmine_runner_test(**kwargs)
diff --git a/packages/jasmine/package.json b/packages/jasmine/package.json
@@ -15,6 +15,9 @@
         "jasmine",
         "bazel"
     ],
+    "bin": {
+        "bazel-jasmine-runner": "jasmine_runner.js"
+    },
     "main": "index.js",
     "dependencies": {
         "jasmine-reporters": "~2.5.0",
diff --git a/packages/jasmine/private/BUILD.bazel b/packages/jasmine/private/BUILD.bazel
@@ -0,0 +1,10 @@
+load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
+
+bzl_library(
+    name = "bzl",
+    srcs = glob(["*.bzl"]),
+    visibility = ["//packages/jasmine:__pkg__"],
+    deps = [
+        "@build_bazel_rules_nodejs//internal/node:bzl",
+    ],
+)
diff --git a/packages/jasmine/private/index.bzl b/packages/jasmine/private/index.bzl
@@ -0,0 +1,13 @@
+"""
+This file mimics what we would get when we install a npm package with bin entries. Only used when jasmine_node_test is used directly
+from rnj sources and should not be published.
+"""
+
+load("@build_bazel_rules_nodejs//internal/node:node.bzl", nodejs_test = "nodejs_test_macro")
+
+def bazel_jasmine_runner_test(**kwargs):
+    nodejs_test(
+        entry_point = "//packages/jasmine:jasmine_runner.js",
+        data = ["//packages/jasmine"] + kwargs.pop("data", []),
+        **kwargs
+    )

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")`
`30`	`30`
`31`	`31`	`yarn_install(`
`32`	`32`	`name = "npm",`
`33`		`- exports_directories_only = False,`
	`33`	`+ exports_directories_only = True,`
`34`	`34`	`package_json = "//:package.json",`
`35`	`35`	`yarn_lock = "//:yarn.lock",`
`36`	`36`	`)`