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

Full parameterized tests support #649

Merged
merged 3 commits into from
Jan 27, 2021

Conversation

connectdotz
Copy link
Collaborator

@connectdotz connectdotz commented Jan 18, 2021

This PR adds full parameterized test support across the extension:

test-matching

  • upgrade to the jest-editor-support@28.1 to pick up the parser fix for full parameterized test syntax support
  • the matching logic is enhanced to handle parameterized describe blocks, in addition to parameterized test blocks. All parameterized test results are exposed to the rest of the system.

decorators

  • all flavor of parameterized tests should show (computed) test status in the gutter
  • all parameterized test errors will be marked with decorators, therefore, a parameterized test block might have multiple error decorators or a single line might have multiple error messages.

diagnosis

  • all parameterized test errors show up in the PROBLEMS area as well as hovering messages. Each with the full test name.

debug

  • If there are multiple debuggable tests for the given test block, DebugCodeLens display the number of debuggable tests and will ask the user to choose which test to debug.
  • run-test command API change (backward compatible)
  • optimization: remove stop/start the internal jest tests process during debugging. (see the open question below)

Open Questions

  • I was surprised to see that we stop and restart jest process in JestExt.runTest. Looks like this logic was there since runTest is introduced (debugger support #172). Could not see any discussion about why that was needed... Concern that it could be a very expensive overhead especially for large projects. I tried without stop/start logic and all seem fine. So I figure unless we knew the explicit use case to justify the overhead, it is better to leave them out...

Notes

  • There is a known issue in jest that parameterized tests have incorrect locations (Location for test.each results is always 42,9 jestjs/jest#10412). The fix (Fix location for test.each jestjs/jest#10413) was merged in jest-26.5.0. Therefore, the minimal jest version for projects with parameterized tests should be >= 26.5.0
  • to eat our own dog food, I have added a few parameterized tests in our own test suites. You can build the extension with this PR to try out.
  • This PR does touch quite a few files, but most are not too complicated. I have tested this for a couple of days, but more tests especially from different platforms will be needed. Hopefully, we can release this soon so more people can help testing it.

fixes #427 (see comments)

@coveralls
Copy link

coveralls commented Jan 19, 2021

Pull Request Test Coverage Report for Build 873

  • 110 of 111 (99.1%) changed or added relevant lines in 12 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 90.047%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/diagnostics.ts 25 26 96.15%
Files with Coverage Reduction New Missed Lines %
src/TestResults/match-node.ts 2 90.0%
Totals Coverage Status
Change from base Build 871: 0.6%
Covered Lines: 1144
Relevant Lines: 1254

💛 - Coveralls

@connectdotz
Copy link
Collaborator Author

@CzBuCHi looks like you wrote the original debugger support (#172), if you have some time, would be great for you to review this PR as we do have some question about the stop/restart the jest process in JestExt.runTest

@stephtr would love to have you review this PR as well.

@CzBuCHi
Copy link
Contributor

CzBuCHi commented Jan 19, 2021

Not sure if i can help - i deleted my fork when it got merged and totally forget about it ...

about jest restarting via JestExt.runTest: probably because i didnt know any better ...

This was referenced Jan 19, 2021
@connectdotz
Copy link
Collaborator Author

@CzBuCHi thanks for the reply.

@connectdotz
Copy link
Collaborator Author

@stephtr looks like you are busy...

I ran with this PR last week, and it seems solid, pretty handy actually, as I started to add more parameterized tests... At this point, I think it's probably better to merge it and cut an alpha release so we can start the field testing/fixing cycle sooner... thoughts?

@connectdotz connectdotz merged commit 0a96497 into jest-community:master Jan 27, 2021
@connectdotz connectdotz deleted the parameterized-tests branch January 27, 2021 02:13
Copy link
Member

@stephtr stephtr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay
I had just a quick shallow look at it and tested v4 alpha 3, great work! 😃

* @param fileName
* @param testIds test name/pattern.
* Because a test block can have multiple test results, such as for paramertized tests (i.e. test.each/describe.each), there could be multiple debuggable candidates, thus it takes multiple test identifiers.
* Noite: If a test id is a string array, it should represent the hierarchiical relationship of a test structure, such as [describe-id, test-id].
Copy link
Member

Choose a reason for hiding this comment

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

Typos ("Noite", "hierarchiical")

command: `${extensionName}.run-test`,
title: 'Debug',
title: codeLens.testIds.length > 1 ? `Debug(${codeLens.testIds.length})` : 'Debug',
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a space after "Debug"?

Copy link
Member

@stephtr stephtr Jan 31, 2021

Choose a reason for hiding this comment

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

Is "Debug n failing tests"/"Debug failing test" too long? -- Edit: In some cases that doesn't make sense. It's better to stick to the "Debug (n)" approach.

id,
}));
const selected = await vscode.window.showQuickPick<RunTestPickItem>(items, {
placeHolder: 'select a test to debug',
Copy link
Member

Choose a reason for hiding this comment

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

How about an uppercase "Select"?

@connectdotz
Copy link
Collaborator Author

@stephtr better late than never 😉 ... I will address those in another PR

legend1202 pushed a commit to legend1202/vscode-jest that referenced this pull request Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add full support for parametrized tests
4 participants