-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Missing expect assertions call #43
Missing expect assertions call #43
Conversation
Looking for someone to help me understand how to solve below CI issues, I am not able to figure out what is subject, header,body etc. cc: @SimenB |
It's yelling at you for not having semantic commits. See http://karma-runner.github.io/2.0/dev/git-commit-msg.html |
@SimenB Thanks for the help 👍 I will follow those conventions and verify it locally before pushing. I am thinking to amend the commits on my branch and then do force push. Will that work or I am required to create a new PR. |
Amending in this PR would be the best! 🙂 |
Not sure about the name, seems like |
|
||
const ruleTester = new RuleTester(); | ||
const expectedMsg = | ||
'Every test should have expect.assertions({number of assertions}) OR expect.hasAssertions() as first expression'; |
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.
I think this might read better as
Every test should have either `expect.assertions(<number of assertions>)` or `expect.hasAssertions()` as it's first expression
if (isTestOrItFunction(node)) { | ||
if (!isFirstLineExprStmt(node)) { | ||
reportMsg(context, node); | ||
return; |
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.
I think this return statement is unnecessary?
Intial code for missing_expect_assertions_call rule
Report error when expect.assertions() call is present but the argument provided is not a number
Rule will not report any error, if expect.hasAssertions() call is present. After this change, either expect.hasAssertions() or expect.assertions({number of assertions}) is required to pass the rule
Rename missing-expect-assertions-call to prefer-expect-assertions
e8aefb1
to
2f4bc72
Compare
@giodamelio Thank for your feedback. I have renamed rule to @SimenB All CI checks are green. Thank you for your help! 🙂 |
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.
This is looking good!
Could you ensure 100% test coverage for the new rule?
Lines 13,47,55 are not covered (run yarn test --coverage
to see)
(Also make sure to pull, I pushed a tiny bit to your branch 🙂 )
@SimenB added the missing coverage. |
Thanks! Released as 21.6.0 |
@tushardhole we forgot to update the README 😱 Mind sending a PR for that? 🙂 |
@SimenB I would create an PR for README |
Docs published in 21.6.1 |
Adding new lint rule which validates,
Every test to have expect.assertions({number of assertions}) as first line
OR
Every test to have expect.hasAssertions() as first line