-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
JavaScript: Add runnable tests #12118
JavaScript: Add runnable tests #12118
Conversation
c123a95
to
8e0a514
Compare
What if the user uses another framework instead of jest Does the task/runnable in zed take this in consideration currently? |
@sigmaSd We could also add support for Deno tests, I thought most people use Jest for testing. We could pair on it to make it work if you want to? because I never used Deno so don't know the in and outs of it. |
Thanks for this PR, it will be very usefull ! There is also Bun tests (bun:test) and Node tests (node:test). This will be supported in the future ? |
@coratgerl You can make that possible by your self, when this pull request is merged by adding the following task to your [
{
"label": "bun test $ZED_SYMBOL",
"command": "bun test",
"args": ["\"$ZED_SYMBOL\" $ZED_FILE"],
"tags": ["jest-test"]
},
...
] But we might want to rename the |
Thanks but I can't currently hack on zed , but here is what I tried #11586 What I'm really wondering is can this in the future be offloaded to extensions as in I can author a deno extension that can provide deno tests without clashing with the default runnable that this pr adds |
Otherwise we can put all the combinations possible inside this default runnable |
Ahh Deno has their own lsp and stuff, so I think the Deno extension just should contain their own runnables instead of adding support for them inside the JavaScript/Typescript extensions. |
Renamed {
"tags": ["js-test", "ts-test"],
...
} |
@RemcoSmitsDev something that came up in other PRs/issues (#12003 (comment) #12215) is that we might want to allow users themselves to fill in some environmental variables used by tasks. Imagine that user could have the following in their settings (in pseudocode):
which you could then inspect when creating a task context on extension side. |
Yeah, great idea! That would be nice if we could give the users some more easy control for switching between runners. So we and them don't have to duplicate the tasks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's give it some time on nightly and merge it after todays Preview is out.
It appears this PR did not enable runnable tests for "tsx" files. Is that intentionally left out of this scope, or could I create a new issue for this? |
I think that's just omission (so a new issue would be most apt); any thoughts on that @RemcoSmitsDev? |
Yeah didn't left it out on purpose, I thought this was included inside the Typescript language. So totally misted the JSX language. |
Thanks @RemcoSmitsDev! I created #12884 to track implementation for TSX. |
Some of the runnables added in #12118 don't work for tests (or code) that contain spaces. In other words, the runnable for a test like ```js it('does the thing', () => ...) ``` would end up w/ something like `npx jest does the thing /path/to/file.spec.js`, but what we really want is `npx jest --testNamePattern "does the thing" /path/to/file.spec.js`. A similar thing was happening for the "node execute selection" runnable: selecting `let foo = 1` would run `node -e let foo = 1`, not `node -e "let foo = 1"`. In my (somewhat limited?) experience, it's very common for tests like these to include spaces, and of course a code selection is almost certain to contain whitespace. Not covered: - this just blindly wraps quotes around the symbol/code; in the future it may make sense to try to figure out *what type of quote* to use. (eg `it('does the "thing"', () => ...)` is a valid test name, but `--testNamePattern "does the "thing""` would not work. Note the doubled quotes.) - I did not wrap the filenames in quotes to escape those for the shell, nor did I test if that's actually an issue. In my experience, I've not seen many (any?) test files that contain spaces in the name, but I suspect that it would be an issue if a containing dir includes spaces. (eg `npx jest ... /path/to/My Documents/Code/file.spec.js` /cc @RemcoSmitsDev Release Notes: - Fixed some runnables in Javascript/Typescript
Some of the runnables added in zed-industries#12118 don't work for tests (or code) that contain spaces. In other words, the runnable for a test like ```js it('does the thing', () => ...) ``` would end up w/ something like `npx jest does the thing /path/to/file.spec.js`, but what we really want is `npx jest --testNamePattern "does the thing" /path/to/file.spec.js`. A similar thing was happening for the "node execute selection" runnable: selecting `let foo = 1` would run `node -e let foo = 1`, not `node -e "let foo = 1"`. In my (somewhat limited?) experience, it's very common for tests like these to include spaces, and of course a code selection is almost certain to contain whitespace. Not covered: - this just blindly wraps quotes around the symbol/code; in the future it may make sense to try to figure out *what type of quote* to use. (eg `it('does the "thing"', () => ...)` is a valid test name, but `--testNamePattern "does the "thing""` would not work. Note the doubled quotes.) - I did not wrap the filenames in quotes to escape those for the shell, nor did I test if that's actually an issue. In my experience, I've not seen many (any?) test files that contain spaces in the name, but I suspect that it would be an issue if a containing dir includes spaces. (eg `npx jest ... /path/to/My Documents/Code/file.spec.js` /cc @RemcoSmitsDev Release Notes: - Fixed some runnables in Javascript/Typescript
Some of the runnables added in zed-industries#12118 don't work for tests (or code) that contain spaces. In other words, the runnable for a test like ```js it('does the thing', () => ...) ``` would end up w/ something like `npx jest does the thing /path/to/file.spec.js`, but what we really want is `npx jest --testNamePattern "does the thing" /path/to/file.spec.js`. A similar thing was happening for the "node execute selection" runnable: selecting `let foo = 1` would run `node -e let foo = 1`, not `node -e "let foo = 1"`. In my (somewhat limited?) experience, it's very common for tests like these to include spaces, and of course a code selection is almost certain to contain whitespace. Not covered: - this just blindly wraps quotes around the symbol/code; in the future it may make sense to try to figure out *what type of quote* to use. (eg `it('does the "thing"', () => ...)` is a valid test name, but `--testNamePattern "does the "thing""` would not work. Note the doubled quotes.) - I did not wrap the filenames in quotes to escape those for the shell, nor did I test if that's actually an issue. In my experience, I've not seen many (any?) test files that contain spaces in the name, but I suspect that it would be an issue if a containing dir includes spaces. (eg `npx jest ... /path/to/My Documents/Code/file.spec.js` /cc @RemcoSmitsDev Release Notes: - Fixed some runnables in Javascript/Typescript
Context: @bennetbo spotted a regression in handling of `cargo run` task in zed repo following a merge of #13658. We've started invoking `cargo run` from the folder of an active file whereas previously we did it from the workspace root. We brainstormed few solutions that involved adding a separate task that gets invoked at a workspace level, but I realized that a cleaner solution may be to finally add user-configured task variables. This way, we can choose which crate to run by default at a workspace level. This has been originally brought up in the context of javascript tasks in #12118 (comment) Note that this is intended for internal use only for the time being. Release notes: - N/A
Context: @bennetbo spotted a regression in handling of `cargo run` task in zed repo following a merge of #13658. We've started invoking `cargo run` from the folder of an active file whereas previously we did it from the workspace root. We brainstormed few solutions that involved adding a separate task that gets invoked at a workspace level, but I realized that a cleaner solution may be to finally add user-configured task variables. This way, we can choose which crate to run by default at a workspace level. This has been originally brought up in the context of javascript tasks in #12118 (comment) Note that this is intended for internal use only for the time being. /cc @RemcoSmitsDev we should be unblocked on having runner-dependant tasks now. Release notes: - N/A
Screen.Recording.2024-05-22.at.13.57.33.mov
Release Notes: