Skip to content

chore: improving code coverage to 100% branches#1624

Merged
profnandaa merged 2 commits intovalidatorjs:masterfrom
renanmontebelo:coverage-license-plates
Mar 12, 2021
Merged

chore: improving code coverage to 100% branches#1624
profnandaa merged 2 commits intovalidatorjs:masterfrom
renanmontebelo:coverage-license-plates

Conversation

@renanmontebelo
Copy link
Contributor

@renanmontebelo renanmontebelo commented Mar 6, 2021

In isLicensePlate.js the hasOwnProperty() function is being used probably as a requirement of es-lint guard-for-in rule. In my opinion this linter rule brings no value here and it's preventing 100% branch coverage in tests.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Mar 6, 2021 •
edited
Loading

Codecov Report

Merging #1624 (5530e3d) into master (deb1d1e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1624   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1807      1800    -7     
=========================================
- Hits          1807      1800    -7
Impacted Files Coverage Δ
src/lib/isStrongPassword.js 100.00% <ø> (ø)
src/lib/isLicensePlate.js 100.00% <100.00%> (ø)

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 deb1d1e...5530e3d. Read the comment docs.

tux-tn
tux-tn previously approved these changes Mar 6, 2021
View reviewed changes
Copy link
Member

@tux-tn tux-tn left a 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?

@renanmontebelo
Copy link
Contributor Author

renanmontebelo commented Mar 6, 2021

@tux-tn I pushed a new commit for the coverage issue in isStrongPassword.

About the other points you mentioned, I think it will be a case-by-case thing. As you can see, the isStrongPassword required the "istambul ignore else" approach because that was the only feasible way of getting the coverage at that point.

For hasOwnProperty I also think it will be case-by-case; if the hasOwnProperty is part of our test code I believe it would be safe to remove it because we know the code that we're testing very well. That might not be true for other parts of the library, I believe I don't have enough experience with the library for a better judgment. I guess that if the hasOwnProperty is not creating coverage issues I'd leave it as is.

These are very good points for discussion you mentioned.

@renanmontebelo
Copy link
Contributor Author

renanmontebelo commented Mar 6, 2021

I'm not sure why the code coverage report here in Github now mentions "Hits -7". In my local it's covering 100%:

image

tux-tn
tux-tn approved these changes Mar 6, 2021
View reviewed changes
Copy link
Member

@tux-tn tux-tn left a 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.

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed 🎉 first-pr labels Mar 7, 2021
@profnandaa profnandaa merged commit 6262f62 into validatorjs:master Mar 12, 2021
@renanmontebelo renanmontebelo deleted the coverage-license-plates branch March 13, 2021 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@profnandaa profnandaa profnandaa approved these changes

@tux-tn tux-tn tux-tn approved these changes

Assignees

No one assigned

Labels

🎉 first-pr ready-to-land For PRs that are reviewed and ready to be landed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@renanmontebelo @profnandaa @tux-tn