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

[Jest Circus] Make hooks in empty describe blocks error #6320

Merged
merged 9 commits into from
May 30, 2018

Conversation

captbaritone
Copy link
Contributor

Fixes #6293


● Test suite failed to run

thrown: \\"Invalid: beforeEach() in a describe block with no tests.\\"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB This is using the same formatting as user errors. I'm not sure if these errors should have the thrown: prefix. Thoughts?

Also, not sure about the wording here. Got any suggestions for other wording that would be more in-line with other Jest errors?

Copy link
Member

@SimenB SimenB May 27, 2018

Choose a reason for hiding this comment

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

Yeah, the thrown part is a bit heavy handed. Not sure how to best differentiate. Maybe you can just copy the stack from asyncError and append your own message? Like state.unhandledErrors.push({message: myMessage: stack: asynError.stack}) or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@SimenB SimenB May 27, 2018

Choose a reason for hiding this comment

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

You can probably just do asyncError.message = 'my message' and push it into errors, and it'll be correct, without the thrown: part

11 |
12 | describe('another block with tests', () => {

at packages/jest-circus/build/index.js:33:17
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB Is there anything special that I need to do to filter out this line of the stack trace?

Copy link
Member

@SimenB SimenB May 27, 2018

Choose a reason for hiding this comment

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

I was a bit lazy in #6281 and didn't add all the Error.captureStackTraces I should. I just added it to test, should be added to all hooks as well.

A gotcha is that it only removes a single frame, so when we have _createHook or whatever the function is called, you need to move the error creation one level up to remove the correct frame

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

So happy about this!

Does this deserve a changelog entry? This is a new feature of Circus, not just feature parity with jasmine

@captbaritone
Copy link
Contributor Author

@SimenB It should probably be a part of the Jest Circus changelog entry. Any idea where to write that in the mean time?

@SimenB
Copy link
Member

SimenB commented May 27, 2018

something like

### Features

* `[jest-circus]` Make hooks in empty describe blocks error ([#6320](https://github.com/facebook/jest/pull/6320))

?

Copy link
Contributor

@aaronabramov aaronabramov left a comment

Choose a reason for hiding this comment

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

this is amazing!

now what about this case? 😛

test('lol', () => {
  test('lmao', () => {});
});

i've seen in somewhere in our codebase

@@ -0,0 +1,14 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget to move these to e2e :)

currentDescribeBlock.hooks.forEach(hook => {
state.unhandledErrors.push([
`Invalid: ${hook.type}() in a describe block with no tests.`,
hook.asyncError,
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm so happy that this will actually show where the hook is defined

@SimenB
Copy link
Member

SimenB commented May 27, 2018

@aaronabramov the test in test thing can be even worse, FB apparently needs hooks inside tests: #6098.

if (Error.captureStackTrace) {
Error.captureStackTrace(asyncError, hookFn);
}
dispatch({asyncError, fn, hookType, name: 'add_hook', timeout});
Copy link
Member

Choose a reason for hiding this comment

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

it was returned previously? not sure if it matters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dispatch() returns void.

test('hook in empty describe', () => {
const result = runJest(dir, ['hook-in-empty-describe.test.js']);
expect(result.status).toBe(1);
expect(extractSummary(result.stderr)).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

missing the snapshots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Got lost in the move to e2e. Added.

if (!describeBlockHasTests(currentDescribeBlock)) {
currentDescribeBlock.hooks.forEach(hook => {
state.unhandledErrors.push([
`Invalid: ${hook.type}() in a describe block with no tests.`,
Copy link
Member

Choose a reason for hiding this comment

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

if you do

hook.asyncError.message = `Invalid: ${hook.type}() in a describe block with no tests.`

I think the message will be without the extra quotes, and without thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc417a2 Works perfectly. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

awesome!

@codecov-io
Copy link

codecov-io commented May 27, 2018

Codecov Report

Merging #6320 into master will decrease coverage by 0.09%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6320     +/-   ##
=========================================
- Coverage   63.86%   63.77%   -0.1%     
=========================================
  Files         228      228             
  Lines        8699     8713     +14     
  Branches        3        3             
=========================================
+ Hits         5556     5557      +1     
- Misses       3142     3155     +13     
  Partials        1        1
Impacted Files Coverage Δ
.../src/legacy_code_todo_rewrite/jest_adapter_init.js 0% <0%> (ø) ⬆️
packages/jest-circus/src/event_handler.js 12.85% <0%> (-0.78%) ⬇️
packages/jest-circus/src/index.js 52.17% <38.46%> (-9.37%) ⬇️
packages/jest-circus/src/utils.js 21.73% <50%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c78f3d...dbadab9. Read the comment docs.

@captbaritone
Copy link
Contributor Author

@aaronabramov It looks like tests within tests are explicitly supported. In fact, that's how test.each works. cc @mattphillips

@mattphillips
Copy link
Contributor

@captbaritone test.each doesn't nest tests it take the global test function as a callback and forEachs over the data calling the test callback making each test a sibling.

test.each function is just sugar for forEach 😄

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

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

Successfully merging this pull request may close these issues.

[jest-circus] throw if beforeAll or afterAll hooks defined in empty describe block
7 participants