Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: enable test file crawling for jest example #1506

Merged
merged 1 commit into from Jan 30, 2020
Merged

feat: enable test file crawling for jest example #1506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions .vscode/settings.json
View file
Open in desktop
@@ -1,4 +1,3 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean to check in this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it shouldn't be part of this change, but in general I have to do this to rules_nodejs everytime I try to edit the repo in vscode. It breaks the bazel vscode extensions ability to format build files

"bazel.buildifierExecutable": "${workspaceFolder}/node_modules/.bin/buildifier",
"bazel.buildifierFixOnFormat": true
}
}
6 changes: 3 additions & 3 deletions examples/jest/BUILD.bazel
View file
Open in desktop
Expand Up @@ -3,6 +3,9 @@ load(":jest.bzl", "jest_test")
jest_test(
name = "test",
srcs = [
"babel.config.js",
"extra.js",
"index.js",
"index.test.js",
"index2.test.js",
],
Expand All @@ -16,9 +19,6 @@ jest_test(
"no-bazelci-mac",
],
deps = [
"babel.config.js",
"extra.js",
"index.js",
"@npm//@babel/core",
"@npm//@babel/preset-env",
"@npm//@jest/transform",
Expand Down
3 changes: 3 additions & 0 deletions examples/jest/WORKSPACE
View file
Open in desktop
Expand Up @@ -29,6 +29,9 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")

yarn_install(
name = "npm",
data = [
"//:patches/jest-haste-map+24.9.0.patch",
],
package_json = "//:package.json",
quiet = False,
yarn_lock = "//:yarn.lock",
Expand Down
2 changes: 0 additions & 2 deletions examples/jest/jest.bzl
View file
Open in desktop
Expand Up @@ -10,8 +10,6 @@ def jest_test(name, srcs, deps, jest_config, **kwargs):
"--ci",
]
args.extend(["--config", "$(location %s)" % jest_config])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting to see a glob pattern being added here or in the place jest_test is called

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The glob pattern was already present within jest.config.js (that's the idiomatic way of specifying tests to jest). Do you have a different API you would prefer?

for src in srcs:
args.extend(["--runTestsByPath", "$(location %s)" % src])

_jest_test(
name = name,
Expand Down
8 changes: 3 additions & 5 deletions examples/jest/package.json
View file
Open in desktop
Expand Up @@ -9,13 +9,11 @@
"@jest/transform": "24.7.1",
"babel-jest": "24.7.1",
"babel-plugin-istanbul": "5.1.2",
"jest-cli": "24.7.1",
"jest-config": "24.7.1",
"jest-haste-map": "24.7.1",
"jest-resolve": "24.7.1",
"jest-runtime": "24.7.1"
"jest-cli": "24.9.0",
"patch-package": "^6.2.0"
},
"scripts": {
"postinstall": "patch-package",
"test": "bazel test ..."
}
}
16 changes: 16 additions & 0 deletions examples/jest/patches/jest-haste-map+24.9.0.patch
View file
Open in desktop
@@ -0,0 +1,16 @@
diff --git a/node_modules/jest-haste-map/build/crawlers/node.js b/node_modules/jest-haste-map/build/crawlers/node.js
index 23985ae..a728a9a 100644
--- a/node_modules/jest-haste-map/build/crawlers/node.js
+++ b/node_modules/jest-haste-map/build/crawlers/node.js
@@ -166,7 +166,11 @@ function find(roots, extensions, ignore, callback) {

function findNative(roots, extensions, ignore, callback) {
const args = Array.from(roots);
+ args.push('(');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an issue or PR open on the upstream repo so there's a chance of unwinding this patch later?
if so add a comment somewhere pointing to it, so we'll know when it's time to unpatch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own reference, he's the PR I've started with Jest: jestjs/jest#9351

args.push('-type', 'f');
+ args.push('-o');
+ args.push('-type', 'l');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand where the symlinks are appearing. The node-patches should make it so the source files appear to be in the runfiles tree, not symlinks out of the runfiles. Are you sure the fix belongs in jest and not in the node-patches? Can you show where the symlink appears and where it points?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symlinks appear within the runfiles tree of the testable target. nodejs_binary creates a runfiles directory which is filled with symlinks to the original files.

+ args.push(')');

if (extensions.length) {
args.push('(');