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.

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

feat(testing): support for vscode-jest integration. #1118

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

Domratchev
Copy link

@Domratchev Domratchev commented Feb 28, 2019

Testing in VSCode is possible using vscode-jest extension with the following configuration in launch.json:
{
"name": "vscode-jest-tests",
"type": "node",
"request": "launch",
"program": "${workspaceFolder}/node_modules/@angular/cli/bin/ng",
"args": ["test", "--run-in-band", "--no-code-coverage", "--path-to-file-to-test='${fileDirname}'", "--file-to-test"],
"cwd": "${workspaceFolder}",
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen",
"trace": "all"
}

Might as well run individual tests from the command line using ng test command with --file-to-test and --path-to-file-to-test.

The --path-to-file-to-test might appear redundant, but it's really helpful with vscode-jest integration, as that extension passes in only file name, without the path.

Fixes #1190

@Domratchev
Copy link
Author

Domratchev commented Feb 28, 2019

If you suggest different option names, e.g. --test-file & --test-dir, I might as well do that.

@Domratchev
Copy link
Author

Domratchev commented Mar 1, 2019

With new options the launch.json config is this:
{
"name": "vscode-jest-tests",
"type": "node",
"request": "launch",
"program": "${workspaceFolder}/node_modules/@angular/cli/bin/ng",
"args": ["test", "--pass-with-no-tests", "--run-in-band", "--no-code-coverage", "--test-file"],
"cwd": "${fileDirname}",
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen",
"trace": "all"
}

@Domratchev Domratchev changed the title Added support for all Jest parameters. Added support for file-to-test to facilitate vscode-jest integration. feat(Jest): Added support for all Jest parameters. Added support for --test-file to facilitate vscode-jest integration. Mar 1, 2019
@FrozenPandaz
Copy link
Collaborator

Hi can we approach this in a different way?

Can we keep the options we currently have but then have a way to pass additional options to jest?

We are open to any new options you want but let's do them 1 at a time so we can consider each one.

Something like --file is a good start I think that one would be useful for sure.

@Domratchev
Copy link
Author

Hi Jason, thanks for the suggestion.
Definitely can do, but conceptually, why would you want to interfere in the Jest builder with what Jest itself wants/understands? Having JSON schema around is sufficient for options validation, albeit already overly intrusive (e.g. I could have implemented defaults in it, but deliberately chosen not to, 'cause Jest would do the same).
Again, if you are certain that you want to be fully in control of all the options, I can do it quickly.
At the very minimum these options need to be supported for vscode-jest integration:
"--run-in-band", "--code-coverage" (this on is optional), "--test-name-pattern", "--test-directory" and "--test-file".
Thanks,
Alexei

@Domratchev
Copy link
Author

@FrozenPandaz Done.

@Domratchev Domratchev changed the title feat(Jest): Added support for all Jest parameters. Added support for --test-file to facilitate vscode-jest integration. feat(Jest): support with vscode-jest integration. Mar 4, 2019
@Domratchev Domratchev changed the title feat(Jest): support with vscode-jest integration. feat(Jest): support for vscode-jest integration. Mar 4, 2019
@jasedwards
Copy link

Is there a separate issue for adding other options? My company uses subversion instead of git which means watch does not work for us. I would still like to keep the session live which is why I would need watchAll. Was not sure if we should open a new issue for each option needed.

Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes @Domratchev. This PR is looking much better.

@@ -4,11 +4,17 @@ jest.mock('jest');
const { runCLI } = require('jest');
import * as path from 'path';

class TestJestBuilder extends JestBuilder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this so you can spy? Can you cast to any instead please?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

testNamePattern?: string;
updateSnapshot?: boolean;
useStderr?: boolean;

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

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

I was initially wrong about the minimum scope of changes required for integration.
All the added properties are used by vscode-jest in different scenarios (some - in debug mode, some - in the background).

const project = this.getProjectForFile(filePath);

if (project && project.root !== builderConfig.root) {
return of({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary? and If the specified file is not in the project, should it be a failure?

Copy link
Author

@Domratchev Domratchev Mar 13, 2019

Choose a reason for hiding this comment

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

The fist-file has to be tested in a context of its own project. This is the only way I could come up with to find out what project it is.

jestConfig: string;
testDirectory?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

vscode-jest passes in the file name only, without the full path.
To find out which file exactly that is, and to which project it belongs we need to know the directory.

@FrozenPandaz
Copy link
Collaborator

@jasedwards That is unfortunate, can you please create a separate issue?

@Domratchev Domratchev changed the title feat(Jest): support for vscode-jest integration. feat(testing): support for vscode-jest integration. Mar 15, 2019
@jasedwards
Copy link

@FrozenPandaz It appears after looking at last commit that watchAll will be available. can you confirm? if I am mistaken then I will create a separate issue

@Domratchev
Copy link
Author

Domratchev commented Mar 15, 2019

@jasedwards It was my private initiative, hope @FrozenPandaz doesn't mind.

@Domratchev
Copy link
Author

Domratchev commented Mar 15, 2019

Also it is very tempting to add passWithNoTests=true by default.
Otherwise the testing of multiple projects stops when a project with no tests is discovered.

Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

Thank you for your persistence here as we work out the best solution. I am sorry it is taking so long.

Re: watchAll I am completely open to adding new options which will improve the tool. However, please do not try to "sneak" changes in. I would have preferred it have it's own issue and it's own PR. It also shows up nicer in the Changelog so other users have a better idea of what changed.


if (filePath && !this._isInFolder(builderConfig.root, filePath)) {
return of({
success: true
Copy link
Collaborator

@FrozenPandaz FrozenPandaz Mar 18, 2019

Choose a reason for hiding this comment

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

This seems like an error to me. If we look at this out of the context of vscode-jest then running the following as a command will pass without errors.

> ng test app1 --test-file apps/app2/src/app/app.component.spec.ts
Tests Passed?

For VSCode-Jest debugging, it's hard to get the project which is being tested, thus you are testing all of them with your config. Let's remove this check and consider the following solutions:

  1. Compute which project needs to be tested (i don't think VSCode debugging has support for this :()
  2. Pass the testFile to all projects like you are doing now but also pass passWithNoTests so when it doesn't match any files, it will still pass 🤷‍♀️. This solves your use case semantically without causing a false positive for others.
  3. Prompt the user for the Project via https://code.visualstudio.com/docs/editor/variables-reference#_input-variables
  4. Add configuration to https://github.com/jest-community/vscode-jest to adjust it's logic if the user is using @nrwl/builders:jest similar to how it has a specific treatment for create-react-app projects.

Copy link
Author

@Domratchev Domratchev Mar 18, 2019

Choose a reason for hiding this comment

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

@FrozenPandaz Jason, my intent for --test-file was rather specific to vscode-jest integration (or a similar scenario). I did not want to start testing other projects than the one holding the test-file. This way the terminal output is clear. I have also added --pass-with-no-tests to launch.json (see my edited comment above).
Using ng test with --test-file and non-matching project name is not a supported scenario and produces no results (as expected). The whole --test-directory affair is also very much VSCode specific, being the easiest way to provide full path to the file when needed and ignore it when --test-file is empty.

@Domratchev
Copy link
Author

@FrozenPandaz I do not really like the removal of the project skipping logic. Even with --test-file specified and cwd set it still attempts to find the file in every single project in the workspace. And if the file name is not unique it would find it. I think that logic needs to be there. See my Terminal window output:
cetmac08:formatters adomratchev$ cd /Users/adomratchev/Projects/FARApps/libs/common/src/lib/speech/formatters ; env CI=vscode-jest-tests /usr/local/bin/node --inspect-brk=13240 ../../../../../../node_modules/@angular/cli/bin/ng test --pass-with-no-tests --run-in-band --no-code-coverage --test-file number-formatter.spec.ts --testNamePattern "should recognize plain numbers and ignore spaces"
Debugger listening on ws://127.0.0.1:13240/71c75eb9-f482-45f6-b376-7d4e57bcc0fb
For help, see: https://nodejs.org/en/docs/inspector
Debugger attached.
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/apps/count/count
138 files checked.
testMatch: /Users/adomratchev/Projects/FARApps/apps/count/count/apps/count/count//*.spec.ts, /Users/adomratchev/Projects/FARApps/apps/count/count/libs/services/.spec.ts, /Users/adomratchev/Projects/FARApps/apps/count/count/libs/alert/.spec.ts, /Users/adomratchev/Projects/FARApps/apps/count/count/libs/utils/.spec.ts, /Users/adomratchev/Projects/FARApps/apps/count/count/libs/cet-shared/.spec.ts - 0 matches
testPathIgnorePatterns: /node_modules/ - 138 matches
testRegex: - 138 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/apps/inspect
198 files checked.
testMatch: /Users/adomratchev/Projects/FARApps/apps/inspect/apps/inspect/
/.spec.ts - 0 matches
testPathIgnorePatterns: /node_modules/ - 198 matches
testRegex: - 198 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/apps/speech-test
25 files checked.
testMatch: /Users/adomratchev/Projects/FARApps/apps/speech-test/apps/speech-test/**/
.spec.ts - 0 matches
testPathIgnorePatterns: /node_modules/ - 25 matches
testRegex: - 25 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/libs/auth
28 files checked.
testMatch: **/+(.)+(spec|test).+(ts|js)?(x) - 6 matches
testPathIgnorePatterns: /node_modules/ - 28 matches
testRegex: - 28 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/libs/translate
6 files checked.
testMatch: **/+(
.)+(spec|test).+(ts|js)?(x) - 1 match
testPathIgnorePatterns: /node_modules/ - 6 matches
testRegex: - 6 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/libs/cet-core
8 files checked.
testMatch: **/+(.)+(spec|test).+(ts|js)?(x) - 2 matches
testPathIgnorePatterns: /node_modules/ - 8 matches
testRegex: - 8 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/libs/alert
10 files checked.
testMatch: **/+(
.)+(spec|test).+(ts|js)?(x) - 3 matches
testPathIgnorePatterns: /node_modules/ - 10 matches
testRegex: - 10 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/apps/fix/fix
406 files checked.
testMatch: **/+(.)+(spec|test).+(ts|js)?(x) - 124 matches
testPathIgnorePatterns: /node_modules/ - 406 matches
testRegex: - 406 matches
Pattern: number-formatter.spec.ts - 0 matches
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/libs/cet-shared
33 files checked.
testMatch: **/+(
.)+(spec|test).+(ts|js)?(x) - 12 matches
testPathIgnorePatterns: /node_modules/ - 33 matches
testRegex: - 33 matches
Pattern: number-formatter.spec.ts - 0 matches
PASS ./number-formatter.spec.ts (187.671s)
cetNumberFormatter
✓ should recognize plain numbers and ignore spaces (185615ms)
○ skipped 29 tests

Test Suites: 1 passed, 1 total
Tests: 29 skipped, 1 passed, 30 total
Snapshots: 0 total
Time: 188.786s
Ran all test suites matching /number-formatter.spec.ts/i with tests matching "should recognize plain numbers and ignore spaces".
No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/adomratchev/Projects/FARApps/libs/core
23 files checked.
testMatch: **/+(*.)+(spec|test).+(ts|js)?(x) - 6 matches
testPathIgnorePatterns: /node_modules/ - 23 matches
testRegex: - 23 matches
Pattern: number-formatter.spec.ts - 0 matches
Waiting for the debugger to disconnect...
Killed: 9
cetmac08:formatters adomratchev$

@Domratchev
Copy link
Author

Domratchev commented Mar 21, 2019

@FrozenPandaz By the way, there is a bug in Jest. Its message about "No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0" is misleading. That option IS passed in, and Jest knows it, 'cause it proceeds on with testing of subsequent projects, hence the response code IS 0, it just prints the same message regardless.

Created a new issue there: jestjs/jest#8187

@FrozenPandaz
Copy link
Collaborator

Can you fix the commits? Maybe squash them all into a single commit 🤷‍♂️

By the way, there is a bug in Jest. Its message about "No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0" is misleading.

Good Catch 👍

Even with --test-file specified and cwd set it still attempts to find the file in every single project in the workspace.

This is the nature of using ng test. Ideally, it would know to only run the project whose root is the closest parent directory of process.cwd() as it does when calling ng generate. 🤔 I reached out to the Angular CLI. It's not the cleanest right now, but we can keep looking for other solutions.

@Domratchev
Copy link
Author

Domratchev commented Mar 22, 2019

@FrozenPandaz

Can you fix the commits? Maybe squash them all into a single commit 🤷‍♂️

Done. Have a look, please.

@FrozenPandaz
Copy link
Collaborator

@Domratchev Can you fix the failing tests please?

@vsavkin vsavkin merged commit c1aaed5 into nrwl:master Apr 3, 2019
@Karql
Copy link

Karql commented Apr 9, 2019

Hi @FrozenPandaz @Domratchev

This dose not work on windows:
options.jestConfig = path.resolve( this.context.workspace.root, options.jestConfig );

image

this.context.workspace.root already contains drive letter.
After resolve path is: C:\C\MDM\nwrltest\ws\apps\todos\jest.config.js and could not be find

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] New project instructions don't work
5 participants