-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Adding .hasAssertions to expect API #3379
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3379 +/- ##
==========================================
- Coverage 64.09% 64.04% -0.05%
==========================================
Files 177 177
Lines 6561 6570 +9
Branches 4 4
==========================================
+ Hits 4205 4208 +3
- Misses 2355 2361 +6
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Left some inline comments for you.
|
||
expect.hasAssertions() | ||
|
||
Expected at least one assertion to be called but no assertions were called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected at least one assertion to be called but received none.
maybe?
packages/jest-matchers/src/index.js
Outdated
@@ -57,6 +57,7 @@ if (!global[GLOBAL_STATE]) { | |||
state: { | |||
assertionCalls: 0, | |||
assertionsExpected: null, | |||
expectedAssertions: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
expectedAssertionsNumber: null,
isExpectingAssertions: false,
Seems a bit more descriptive. assertionsExpected
and expectedAssertions
don't tell much to me.
cc: @mxstbr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, looks great!
Alright, I fixed everything up and made the flow typing a bit better so that all of this is actually working properly :) |
2. Added test cases in `integration-tests` 3. Removed `yarn.lock` from pull request
Rebasing with master, resolved conflicts
Should be added here, right? https://github.com/facebook/jest/blob/master/packages/eslint-plugin-jest/src/rules/valid-expect.js#L19 |
Thanks @cpojer |
@SimenB yes! @anilreddykatta do you wanna send a PR for the eslint plugin change? :) |
Might actually have to update the code there, unsure if it handles a function directly on expect now (which is no error was triggered by the linter now) |
@cpojer I didn't quite get what you mean. Do you mind elaborating a bit? I can definitely raise a PR. 😄 |
Just adding the issue reference so people can find it: #3359 |
@anilreddykatta It was a bit more involved than just adding to an array, so I sent a PR for it: #3426 |
* 1. Updated docs for `.hasAssertions` API Change 2. Added test cases in `integration-tests` 3. Removed `yarn.lock` from pull request * Adding suggested changes Rebasing with master, resolved conflicts * Fixes + improved flow types.
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. |
Fixes #3371
.hasAssertions
API Changeintegration-tests
yarn.lock
from pull requestSummary
Test plan
Unit test cases along with integration tests have been added.