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

Warn if describe returns a value #7852

Merged
merged 11 commits into from
Mar 7, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Feb 10, 2019

Summary

As the troubleshooting docs state, tests must be defined synchronously. This PR aims to prevent a class of errors where describe is used with an async function by printing a dedicated error message instead of "Your test suite must contain at least one test)", which usually appears if something is awaited before the it calls.

Would have saved these people.
Unfortunately does not protect against this one though, and we'd need static analysis to save people from top-level awaits.

This could be considered breaking, especially if someone can think of a legitimate use case for returning a Promise (or anything at all) from describe. Or it could be considered non-breaking because returning a Promise (or anything at all) from describe was never supported. Hopefully FB doesn't do it 😅

Test plan

New e2e test green with and without JEST_CIRCUS=1.

@jeysal
Copy link
Contributor Author

jeysal commented Feb 10, 2019

Just noticed overrideGlobals is failing, will take care of that.

@SimenB
Copy link
Member

SimenB commented Feb 10, 2019

This could be considered breaking, especially if someone can think of a legitimate use case for returning a Promise (or anything at all) from describe

It probably is - we had a test at work with an async describe which worked (we didn't await anything, so it was able to run within a tick or whatever). I'm fine with it though - the fix is for people to just delete it (and they should use https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/valid-describe.md)

@jeysal jeysal force-pushed the fail-describe-returns-promise branch from 48e4d52 to 93f23e6 Compare February 10, 2019 15:38
@jeysal jeysal force-pushed the fail-describe-returns-promise branch 2 times, most recently from 9831585 to f17bcce Compare March 2, 2019 21:21
@jeysal
Copy link
Contributor Author

jeysal commented Mar 2, 2019

I've updated the PR to only console.warn for now.
Will create an issue for failing in Jest 25 once this is merged

@jeysal
Copy link
Contributor Author

jeysal commented Mar 2, 2019

Added an ugly stack trace:
image

Unfortunately couldn't find an easy way to print a nice one in those files, but at least it does contain the describe location. @SimenB if you can find us a ProjectConfig, jest-message-util would of course be a lot prettier 😄
When changing from console.warn to throwing, the error will be pretty automatically

@jeysal jeysal force-pushed the fail-describe-returns-promise branch from 6b64186 to afa46bc Compare March 2, 2019 23:11
@jeysal
Copy link
Contributor Author

jeysal commented Mar 2, 2019

Update: error stack now prettier
image

@jeysal
Copy link
Contributor Author

jeysal commented Mar 2, 2019

Jasmine
image
Circus
image

@jeysal jeysal changed the title Fail test suite if describe returns a Promise Warn if describe returns a value Mar 3, 2019
@jeysal
Copy link
Contributor Author

jeysal commented Mar 3, 2019

Updated e2e tests to snapshot stdout instead of just matching a few of the lines. I think this is ready now

@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #7852 into master will increase coverage by 0.77%.
The diff coverage is 21.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7852      +/-   ##
==========================================
+ Coverage   63.31%   64.09%   +0.77%     
==========================================
  Files         263      264       +1     
  Lines       10266    10218      -48     
  Branches     2098     1836     -262     
==========================================
+ Hits         6500     6549      +49     
+ Misses       3273     3262      -11     
+ Partials      493      407      -86
Impacted Files Coverage Δ
packages/jest-circus/src/index.ts 68.05% <0%> (-4.01%) ⬇️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-util/src/isPromise.ts 100% <100%> (ø)
packages/expect/src/jasmineUtils.ts 83.92% <0%> (-0.9%) ⬇️
packages/jest-haste-map/src/index.ts 83.28% <0%> (-0.05%) ⬇️
packages/jest-watcher/src/PatternPrompt.ts 0% <0%> (ø) ⬆️
packages/jest-runtime/src/index.ts 68.98% <0%> (ø) ⬆️
packages/jest-changed-files/src/index.ts 0% <0%> (ø) ⬆️
packages/jest-matcher-utils/src/index.ts 91.83% <0%> (ø) ⬆️
packages/jest-circus/src/formatNodeAssertErrors.ts 14.28% <0%> (ø) ⬆️
... and 72 more

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 4c25942...cf17d6a. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Mar 3, 2019

@thymikee thoughts?

@thymikee
Copy link
Collaborator

thymikee commented Mar 3, 2019

Love it. I'm thinking that maybe it would be better to leave the warning title yellow and the rest of the message (or at least code frame) in default color?

@SimenB
Copy link
Member

SimenB commented Mar 3, 2019

It's yellow due to console.warn. Could use console.log? I don't think that messes with colors

@thymikee
Copy link
Collaborator

thymikee commented Mar 3, 2019

I'd go with console.log and yellow heading. Yellow codeframe feels odd, and it's something we don't do anywhere else in the code, right? Although jest-validate's warnings are whole yellow, so maybe let's just do chalk.reset on the code frame

@jeysal
Copy link
Contributor Author

jeysal commented Mar 3, 2019

New warnings for
Jasmine
image
Circus
image

@jeysal jeysal force-pushed the fail-describe-returns-promise branch from 5328444 to cf17d6a Compare March 3, 2019 19:08
@SimenB
Copy link
Member

SimenB commented Mar 3, 2019

Cool! Should hold off on merging this until we land the massive #7970

@SimenB
Copy link
Member

SimenB commented Mar 6, 2019

Rebase now that we've merged the jasmine PR? 🙂

@jeysal
Copy link
Contributor Author

jeysal commented Mar 6, 2019

Aye

@jeysal jeysal force-pushed the fail-describe-returns-promise branch from cf17d6a to 0c8038e Compare March 6, 2019 23:43
@SimenB SimenB merged commit 690221b into jestjs:master Mar 7, 2019
@jeysal jeysal deleted the fail-describe-returns-promise branch March 7, 2019 10:10
danielbayley added a commit to danielbayley/jest-preset-coffeescript that referenced this pull request Jul 6, 2019
CoffeeScript implicit `return`s trigger a warning since `describe` [should not](jestjs/jest#7852) `return` a value.
@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 11, 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.

5 participants