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

Better error message when a describe block is empty #6372

Merged
merged 10 commits into from
Oct 7, 2018

Conversation

eddiesholl
Copy link
Contributor

@eddiesholl eddiesholl commented Jun 1, 2018

Summary

When you create an empty describe block with no body, the error message returned is obscure. If you have done it by accident, it doesn't give you any help trying to correct the problem.

The same type of helpful error is already returned for empty it blocks, this is just taking the same approach to checking the params supplied.

This was raised originally in #6224

Test plan

Before
image

After
image

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@eddiesholl eddiesholl force-pushed the esholl/allow-empty-describe branch from dea6999 to fe0577e Compare June 1, 2018 05:20
@SimenB
Copy link
Member

SimenB commented Jun 1, 2018

Mind fixing CI? 🙂

@eddiesholl
Copy link
Contributor Author

Yep sorry, hopefully over the weekend 👍

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

@rickhanlonii
Copy link
Member

I fixed the tests and merged in master

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.

Missing changelog entry

@rickhanlonii
Copy link
Member

@SimenB added 🙈

@rickhanlonii
Copy link
Member

Ah, yes, we'll need to make the same updates to circus

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.

Need to update for jest-circus as well. To se failure locally, run JEST_CIRCUS=1 ./jest globals

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@thymikee
Copy link
Collaborator

thymikee commented Aug 8, 2018

@codecov-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #6372 into master will decrease coverage by 0.02%.
The diff coverage is 52.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6372      +/-   ##
==========================================
- Coverage   66.67%   66.65%   -0.03%     
==========================================
  Files         253      253              
  Lines       10607    10621      +14     
  Branches        3        4       +1     
==========================================
+ Hits         7072     7079       +7     
- Misses       3534     3541       +7     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-circus/src/index.js 72.13% <59.37%> (-0.42%) ⬇️

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 dbdc061...3d2064d. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Sep 17, 2018

@eddiesholl ping, mind taking a look at fixing circus as well? 🙂 Link to relevant code in the post above mine.

This also needs a rebase

@SimenB
Copy link
Member

SimenB commented Oct 6, 2018

I merged in master and fixed circus. However, I broke jasmine while doing so (since I cleaned up the stack).

I have to go now, so I pushed regardless, but should be pretty straight forward for somebody to make the last tweak needed for Jasmine 🙂

@SimenB SimenB force-pushed the esholl/allow-empty-describe branch from a9700b7 to 8ab258f Compare October 6, 2018 15:04
@SimenB SimenB force-pushed the esholl/allow-empty-describe branch from ebc0d65 to d9d80e1 Compare October 7, 2018 08:30
@@ -20,7 +20,7 @@ const TEST_DIR = path.resolve(DIR, '__tests__');

function cleanStderr(stderr) {
const {rest} = extractSummary(stderr);
return rest.replace(/.*(jest-jasmine2|jest-circus).*\n/g, '');
return rest.replace(/.*(jest-jasmine2).*\n/g, '');
Copy link
Member

@SimenB SimenB Oct 7, 2018

Choose a reason for hiding this comment

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

no need to strip out circus stuff as we always create an error without any circus traces in it. Can't be bothered doing the same fix for jasmine, though

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.

I've fixed the linting error locally. will push and merge if/when green

@SimenB SimenB merged commit e0ba3e6 into jestjs:master Oct 7, 2018
@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.

6 participants