javascript: Test goal with package manager installed test runner support #18554

tobni · 2023-03-22T19:08:43Z

Add support for running package manager installed test runners, and javascript_test targets.

There's multiple flavors of how coverage reports are configured (extra args, separate script) depending on test runner.
To stay unopinionated I implemented support for both. If one has to go, extra args is the one.

This PR includes:

Introduce a new scripts mapper build symbol node_test_script, where users can augment default behaviour, which is:
To run (npm|pnpm|yarn) run test, the "standard" way to execute test runners.
Coverage reports per test support. I made no effort to merge the coverage reports.
No batch support (yet). I think it would be good to OOTB assume that npm run test <all-files-for-workspace> is preferred to current behaviour of 1 run per file. See e.g https://jestjs.io/docs/cli#--maxworkersnumstring.

Fixes #18524.

tobni · 2023-03-22T19:10:42Z

I've added js projects to testprojects, not sure if that's allowed, but I figured it is nice to have something easy to reach for when inspecting outputs that are more visual like e.g coverage reports are.

tobni · 2023-03-22T20:38:32Z

src/python/pants/backend/javascript/goals/test.py

+        SourceFilesRequest(
+            (tgt.get(SourcesField) for tgt in transitive_tgts.closure),
+            enable_codegen=True,
+            for_sources_types=[JSSourceField, AssetSourceField],


The reason for hydrating AssetSourceFields instead of only ResourceSourceField is that I think the easiest way to support the many different configuration files that can play a part in tooling for a node package. I.e let this

files(name="configs", sources=["*.rc", "*.json", "*.config.js"]) package_json( ... dependencies=[":configs"], )

be the way to have them be part of the sandboxes.

If a test config is only to affect test caching, I think using __defaults__ to configure javascript_test:s dependencies instead could work?

Supporting both files and resources makes sense I think, independent of the reason why. But:

The primary reason that those are separated into two separate types though is if an ecosystem should use separate APIs to load files which are "embedded" in a release binary/package (resources) than they do to load files "next to" their binary/package (files).

Does the JS ecosystem distinguish between those cases? If so, then the primary thing that you want to watch out for is alignment between pants test time and pants package time, so that if you've loaded a resource one way during a test run, then you can also load it that way in production. The same does not necessarily apply to files: depending on the deployment format, files may in the cwd... or there may be no such concept as a cwd.

Hmm... I'm not really following the files and cwd bit fully, but:

I think in pants lingo, any file is considered a resource to the package.json: All files that will be included in a package .tar are expected to be at or below the directory containing the package.json, when "packaging" (npm pack). But the pipeline is scriptable, and the scripts themselves have no limitations to where they can copy files from. I think it is not uncommon to e.g copy a repository README.md from the repo root into a <package-dir>/docs folder that is .gitignored and referenced in package.json.

The package.json has fields that references where files to include in packages are found, but either silently ignores ../ or errors, depending on package manager. The default is to implicitly include all files next to and below the package.json, with a .gitignore-esque blocklist.

In source code, files are either loaded via import "./path/to/file.json" or via file reading api:s similar to pythons open (of course...). Same thing here is that the import cannot "escape" the package.json directory, whilst open could.

So I guess if a package_json happens to have a dependency on a resource that isn't at or below its containing directory, pants could error or warn?

I think file is the same, strictly speaking? But that is disregarding scriptability. Users could have scripts in package.json to e.g copy files from outside into the package.json dir that executes as part of npm pack. Pants has facilities for this as well though in relocated_files, so maybe encouraging using that is better? 🤔

In source code, files are either loaded via import "./path/to/file.json" or via file reading api:s similar to pythons open (of course...). Same thing here is that the import cannot "escape" the package.json directory, whilst open could.

Are these cases always completely equivalent in JS?

The distinction in Python is: either "imported" (resources) or loaded with open (files). The distinction matters there, because if your Python code is packaged in certain ways (as a zipapp, or using Pyoxidizer, for example), then resources embedded in your binary must be loaded using the resource APIs, because they don't exist as loose files on disk.

So I guess if a package_json happens to have a dependency on a resource that isn't at or below its containing directory, pants could error or warn?

That would probably be a good idea until support for source/workspace dependencies is available.

This relates to the other thread: if there is a CLASSPATH/PYTHONPATH concept in JS (probably NODE_PATH, according to my chatgpt-ing), then that allows multiple source roots to be added to the module resolution at once. Otherwise, you'd have to strip source roots (which could cause file collisions if you have two resources at project1/src/something.json and project2/src/something.json).

That would probably be a good idea until support for source/workspace dependencies is available.

I think that already works, package manager workspaces deals with the module resolution, and the dep inference already picks up the source dependency. A workspace package dependee ends up as a symlink in the /node_modules of the the dependent install time.

tobni · 2023-03-22T20:51:17Z

src/python/pants/backend/javascript/nodejs_project_environment.py

+            yield os.path.join(
+                os.path.relpath(os.path.curdir, self.relative_workspace_directory()), "node_modules"
+            )


So this was very broken before. The NodeJSProjectEnvironment assumes that

CWD should always be at the project root dir, but

that all output files and output directories will be referred to relative to the workspace

Point 2 Is true for "goal facing" stuff like lockfile generation, package goals and now tests. It is very convenient there. But I had borked it here, and made it so this method produce paths relative to 1 instead of 2.

Hm. Yea, it's slightly odd to try and have this both ways.

Do JS runners have a concept like source roots (aka the PYTHONPATH, aka the CLASSPATH), where a prefix is stripped from a directory? If so, it might be good to make lockfile generation and test running consistently use strategy (2) using that facility.

That would also ease eventually allowing source dependencies between JS projects in a monorepo, IIUC.

Indeed, that is how NODE_PATH works, but I'm fairly sure NODE_PATH usage is discouraged in favor of the standard algorithm. Convention rules here.

Monorepos are directly supported via "workspaces", I'm not sure it is a worthwhile to re-invent that wheel?

So

That would also ease eventually allowing source dependencies between JS projects in a monorepo, IIUC.

You get this for free with the "workspaces" feature of the package managers already, I think. If a package.json contains a package.json#dependencies entry that points to a package whose name is a workspace in the current project, the package manager will symlink that package (the one in package.json#dependencies) to the node_modules of the package.json dependee, install time.

I suppose pants could attempt to support a completely separated package.json as well, but I'm afraid there's a lot of work there that I don't think will give a lot of value.

Ok, it sounds like unlike in Python (and similar to the JVM), there is an install step for dependency projects that means that they can't collide with source dependencies.

tobni · 2023-03-22T21:07:16Z

src/python/pants/backend/javascript/package_json.py

@@ -707,7 +707,7 @@ async def generate_node_package_targets(
    package_target = NodePackageTarget(
        {
            **request.template,
-            NodePackageNameField.alias: pkg_json.name.replace("@", "__"),
+            NodePackageNameField.alias: pkg_json.name,


Another oopsie, this has to actually be the correct string w.r.t what is in package.json#name field.

stuhood

Looks good! Two questions related to the relativization of files in the sandbox.

stuhood · 2023-03-23T21:56:05Z

src/python/pants/backend/javascript/goals/test.py

+        SourceFilesRequest(
+            (tgt.get(SourcesField) for tgt in transitive_tgts.closure),
+            enable_codegen=True,
+            for_sources_types=[JSSourceField, AssetSourceField],


Supporting both files and resources makes sense I think, independent of the reason why. But:

The primary reason that those are separated into two separate types though is if an ecosystem should use separate APIs to load files which are "embedded" in a release binary/package (resources) than they do to load files "next to" their binary/package (files).

Does the JS ecosystem distinguish between those cases? If so, then the primary thing that you want to watch out for is alignment between pants test time and pants package time, so that if you've loaded a resource one way during a test run, then you can also load it that way in production. The same does not necessarily apply to files: depending on the deployment format, files may in the cwd... or there may be no such concept as a cwd.

src/python/pants/backend/javascript/goals/test.py

stuhood · 2023-03-23T22:12:25Z

src/python/pants/backend/javascript/nodejs_project_environment.py

+            yield os.path.join(
+                os.path.relpath(os.path.curdir, self.relative_workspace_directory()), "node_modules"
+            )


Hm. Yea, it's slightly odd to try and have this both ways.

Do JS runners have a concept like source roots (aka the PYTHONPATH, aka the CLASSPATH), where a prefix is stripped from a directory? If so, it might be good to make lockfile generation and test running consistently use strategy (2) using that facility.

That would also ease eventually allowing source dependencies between JS projects in a monorepo, IIUC.

src/python/pants/backend/javascript/subsystems/nodejstest.py

stuhood

It sounds like the only question to resolve is the resources vs files one: let me know if you plan to make further changes in that area, or whether you're ready to merge.

Thanks!

tobni · 2023-03-29T09:00:26Z

It sounds like the only question to resolve is the resources vs files one: let me know if you plan to make further changes in that area, or whether you're ready to merge.

I intend to address all of the comments, I just haven't gotten around to it!

tobni · 2023-03-29T20:51:04Z

@stuhood Done!

stuhood · 2023-03-29T23:21:05Z

It looks like your merge-base for this one has become too old? I'll rebase it in about 30 minutes unless you do first.

The aliases were wrong, and comments are evil

Also remove redundant Sources from JSTestTargets.

./pants test testprojects/src/js/src/test/index.test.js \ --test-use-coverage \ --test-open-coverage Works!

Implement support for `nyc` style coverage reporting Also touch up the relationship between output dirs and output files. Also added testprojects

tobni mentioned this pull request Mar 22, 2023

javascript_test target, and support running npm test <file> via pants test #18524

Closed

tobni commented Mar 22, 2023

View reviewed changes

tobni force-pushed the add/support-javascript-test branch from fb8f8ec to 0526a11 Compare March 22, 2023 21:03

tobni commented Mar 22, 2023

View reviewed changes

stuhood requested review from stuhood and sureshjoshi March 23, 2023 21:34

stuhood added the category:new feature label Mar 23, 2023

stuhood approved these changes Mar 23, 2023

View reviewed changes

stuhood approved these changes Mar 28, 2023

View reviewed changes

stuhood approved these changes Mar 29, 2023

View reviewed changes

stuhood enabled auto-merge (squash) March 29, 2023 23:11

tobni added 14 commits March 29, 2023 16:47

doc: Remove incorrect comment

cba5961

The aliases were wrong, and comments are evil

feat: Add target types for javascript testing

954fa77

feat: Add tailor support for javascript_tests

0fdbf73

feat: Add support for overrides in js sources

964870d

Also remove redundant Sources from JSTestTargets.

feat: Add boilerplate test fields

d459000

fix: Inherit JSTarget fields to enable dep inference for tests

d44fcd5

feat: Support test goal in javascript backend

4a9171b

feat: Allow custom specifications for test script

248ddf8

feat: Honor the flag

043cc9a

feat: Support coverage reporting

184e6b9

./pants test testprojects/src/js/src/test/index.test.js \ --test-use-coverage \ --test-open-coverage Works!

feat: Coverage reporting as separate script

86f998b

Implement support for `nyc` style coverage reporting Also touch up the relationship between output dirs and output files. Also added testprojects

fix: Ensure root node_modules is part of digest

210fdcd

fix: Correct coverage handling in npm workspaces

08d864a

fix: Do not normalize the package name

a64f89d

tobni added 11 commits March 29, 2023 16:47

test: Added test project with workspace setup

5c7bb43

test: Add missing future import

d3d99fe

doc: Correct blatant copy paste

cc9cf9f

chore: Add annotations import in more places

f363d85

fix: Do not use tuple in get_type_hints path

d420b91

refactor: Replace map with generator

e6f5094

feat: Use faster relpath

d366112

refactor: Do not name unused variables

939677f

refactor: Expect rule callers to relativize user input

ff6abe6

test: Assert the textual output first for easier debugging

792b178

doc: Less pluralization

c021f22

stuhood force-pushed the add/support-javascript-test branch from d651a4d to c021f22 Compare March 29, 2023 23:47

stuhood merged commit 37b3001 into pantsbuild:main Mar 30, 2023
17 checks passed

cczona added the backend: JavaScript JavaScript backend-related issues label May 4, 2023

tobni deleted the add/support-javascript-test branch May 16, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

javascript: Test goal with package manager installed test runner support #18554

javascript: Test goal with package manager installed test runner support #18554

tobni commented Mar 22, 2023 •

edited by stuhood

tobni commented Mar 22, 2023 •

edited

tobni Mar 22, 2023 •

edited

tobni Mar 22, 2023 •

edited

stuhood Mar 23, 2023

tobni Mar 28, 2023 •

edited

stuhood Mar 28, 2023 •

edited

tobni Mar 28, 2023 •

edited

tobni Mar 22, 2023 •

edited

stuhood Mar 23, 2023

tobni Mar 28, 2023 •

edited

stuhood Mar 28, 2023

tobni Mar 22, 2023

stuhood left a comment

stuhood Mar 23, 2023

stuhood Mar 23, 2023

stuhood left a comment

tobni commented Mar 29, 2023

tobni commented Mar 29, 2023

stuhood commented Mar 29, 2023

javascript: Test goal with package manager installed test runner support #18554

javascript: Test goal with package manager installed test runner support #18554

Conversation

tobni commented Mar 22, 2023 • edited by stuhood

tobni commented Mar 22, 2023 • edited

tobni Mar 22, 2023 • edited

Choose a reason for hiding this comment

tobni Mar 22, 2023 • edited

Choose a reason for hiding this comment

stuhood Mar 23, 2023

Choose a reason for hiding this comment

tobni Mar 28, 2023 • edited

Choose a reason for hiding this comment

stuhood Mar 28, 2023 • edited

Choose a reason for hiding this comment

tobni Mar 28, 2023 • edited

Choose a reason for hiding this comment

tobni Mar 22, 2023 • edited

Choose a reason for hiding this comment

stuhood Mar 23, 2023

Choose a reason for hiding this comment

tobni Mar 28, 2023 • edited

Choose a reason for hiding this comment

stuhood Mar 28, 2023

Choose a reason for hiding this comment

tobni Mar 22, 2023

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood Mar 23, 2023

Choose a reason for hiding this comment

stuhood Mar 23, 2023

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

tobni commented Mar 29, 2023

tobni commented Mar 29, 2023

stuhood commented Mar 29, 2023

tobni commented Mar 22, 2023 •

edited by stuhood

tobni commented Mar 22, 2023 •

edited

tobni Mar 22, 2023 •

edited

tobni Mar 22, 2023 •

edited

tobni Mar 28, 2023 •

edited

stuhood Mar 28, 2023 •

edited

tobni Mar 28, 2023 •

edited

tobni Mar 22, 2023 •

edited

tobni Mar 28, 2023 •

edited