-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
chore: improving code coverage to 100% branches #1624
chore: improving code coverage to 100% branches #1624
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1624 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 100 100
Lines 1807 1800 -7
=========================================
- Hits 1807 1800 -7
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.
Thank you @renanmontebelo ! Good catch 🎉
I have some questions/suggestions :
- Do you think you can make the same change to other places where we are using
hasOwnProperty
? When i added code coverage to the project i used another approach and ignored the else branch of the condition. You can search for// istanbul ignore else
to find them. - Do you think it's harmless to disable
guard-for-in
directly in eslint config ? - According to istanbul there is another uncovered branch here in
isStrongPassword
. Can you take care of it as well?
7fdf04a
to
5530e3d
Compare
@tux-tn I pushed a new commit for the coverage issue in About the other points you mentioned, I think it will be a case-by-case thing. As you can see, the For These are very good points for discussion you mentioned. |
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 guess it's because you removed lines. Hits are total covered lines of code and the total is matching the number in your screenshot. Thank you again! LGTM 🎉
I guess we can leave the other occurences of hasOwnProperty as it right now.
In isLicensePlate.js the
hasOwnProperty()
function is being used probably as a requirement of es-lintguard-for-in
rule. In my opinion this linter rule brings no value here and it's preventing 100% branch coverage in tests.Checklist