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
Member

@tux-tn 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 )
================================================================================

@tux-tn
Copy link
Member 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
Member

profnandaa commented May 29, 2019 via email

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@profnandaa
Copy link
Member

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

@profnandaa profnandaa added 🧹 needs-update For PRs that need to be updated before landing and removed 🕑 to-be-reviewed labels Jun 18, 2019
@tux-tn
Copy link
Member 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
@profnandaa profnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Jun 18, 2019
@tux-tn
Copy link
Member 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
Member

profnandaa commented Jun 18, 2019 via email

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

Successfully merging this pull request may close these issues.

3 participants