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

Code coverage improvements #1024

Merged
merged 11 commits into from Jun 18, 2019
Merged

Code coverage improvements #1024

merged 11 commits into from Jun 18, 2019

Conversation

@tux-tn
Copy link
Contributor

tux-tn commented May 5, 2019

Hello,

This PR contains various changes:

  • isISO8601: added a fix for leap years checking where years divisible by 400 where not handled correctly and removed a never met condition
  • isDecimal: fixed decimal point type for Egypt, Libya, Lebanon and South Africa according to the existing Wikipedia source
  • rtrim and ltrim: refactored rtrim to use the same logic and added char escaping to avoid ReDOS and unwanted behavior and chars parsing by Regexp (Using \S as chars for example)
  • isIdentityCard: removed the default value for locale to follow the same behavior of other validators like isPostalCode
  • Added Exception testing: a new attribute error in test function add possibility to check for thrown error in validators, mainly used in working with non-existing locales.

Not very confident about adding tooling/dependencies without prior discussions but I also added istanbul/nyc , it helped me to find and fix unhandled conditions and cases and may be very useful for validator.js PRs when linked with Codecov or Coverall.

Coverage before changes:

=============================== Coverage summary ===============================
Statements   : 97.39% ( 933/958 )
Branches     : 94.5% ( 567/600 )
Functions    : 100% ( 89/89 )
Lines        : 97.53% ( 908/931 )
================================================================================

Coverage after changes:

=============================== Coverage summary ===============================
Statements   : 100% ( 953/953 )
Branches     : 100% ( 586/586 )
Functions    : 100% ( 89/89 )
Lines        : 100% ( 926/926 )
================================================================================
@profnandaa profnandaa self-requested a review May 15, 2019
@tux-tn
Copy link
Contributor Author

tux-tn commented May 25, 2019

@profnandaa I have some other little fixes, should i open a new PR or push to this one?

@profnandaa
Copy link
Collaborator

profnandaa commented May 29, 2019

Copy link
Collaborator

profnandaa left a comment

LGTM

@profnandaa
Copy link
Collaborator

profnandaa commented Jun 18, 2019

@tux-tn -- sorry for my delay to reviewing this. Please fix the merge conflicts and we should land.

@tux-tn
Copy link
Contributor Author

tux-tn commented Jun 18, 2019

@tux-tn thank you for taking time to look into this, my branch is now up to date with master

@profnandaa profnandaa merged commit 9ee09a7 into validatorjs:master Jun 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tux-tn
Copy link
Contributor Author

tux-tn commented Jun 18, 2019

@profnandaa can we add a tool like Coveralls or Codecov to handle coverage change in future PRs?

@profnandaa
Copy link
Collaborator

profnandaa commented Jun 18, 2019

@tux-tn tux-tn deleted the tux-tn:feat/Fixes branch Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.