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

Add missing tests and switch to coverall #1376

Merged
merged 10 commits into from
Jul 20, 2020

Conversation

tux-tn
Copy link
Member

@tux-tn tux-tn commented Jul 11, 2020

Hello,

As discussed in #1366 i worked on the tests to increase the coverage (i reached the goal of 100%).
Sorry i didn't see @ezkemboi work on #1373 and tried to base my work on what he did.
This is a summary of the content of this PR:

  • Remove unreachable code in isIdentityCard especially in CN validator. I tried to remove unnecessary code based on the specs of the validators
  • Throw error if locale is invalid in isTaxID to follow the behaviour of other validators
  • Added case insensitive flag to isSemVer (actually the main goal of this is to reach 100% test coverage and use the second parameter in isMultilineRegex
  • Switch from coverall to codecov. Adding coverage reports in Pull requests was asked a long time ago, i see that @profnandaa added some coverall support but didn't see it working. Actually codecov is easier to use (You don't need any token or configuration, just allow it to access validator.js organization) and have built-in bot. As an example i tried to create a Pull request in my own fork and it works just fine.

What is planned:

This PR is a kind of work in progress, my initial goal was to have browsers tests running but it's harder and longer than what i expected. We need to build the tests suites, remove nodejs dependencies (utils, fs, ...) and integrate them to a browser testing platform (SauceLabs, browserstack, ...)

Actually i'm in the second step, most of the tests are running correctly in the browser. Should we keep the PR open until i finish integrating browser testing or create another PR (all code related to browser testing is uncommited) ?

Capture d’écran 2020-07-11 à 12 19 09 PM

@tux-tn tux-tn mentioned this pull request Jul 11, 2020
3 tasks
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.

Thanks for the clean-up. Please check the comments I dropped below.

82: '澳门',
91: '国外',
};
const provincesAndCities = [
Copy link
Member

Choose a reason for hiding this comment

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

Any reasons for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the original version we are just checking if the first two numbers of the ID are a valid province number. It means having an object is unnecessary and probably the result of a copy/paste

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for late response. So that we don't lose the info on the provinces, could you add those as comments on each line instead?

11, // '北京'
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -4190,10 +4191,18 @@ describe('Validators', () => {
'235407195106112745',
'210203197503102721',
'520323197806058856',
'235477190110989',
'110101491001001',
Copy link
Member

Choose a reason for hiding this comment

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

Can add a note for this churn?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both values are correct, i think the old version is an improvements made by @ezkemboi to cover 15 characters ID cards that got overwritten when i merged my own existing changes. I Can undo it if you want to

'(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))',
'?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$',
]);
'(?:-((?:0|[1-9]\\d*|\\d*[a-z-][0-9a-z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-z-][0-9a-z-]*))*))',
Copy link
Member

Choose a reason for hiding this comment

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

Can explain these changes too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was the only way to test/exec multilineRegexp second parameter. The method is only used in this function and there is no difference with the old regex except the fact that this one is shorter.
If we want 100% coverage we have to choose between keeping this little hack or removing completely flags parameter from multilineRegexp (We may use this function in other validators in the future)

@@ -209,20 +203,19 @@ const validators = {
const check15IdCardNo = (idCardNo) => {
let check = /^[1-9]\d{7}((0[1-9])|(1[0-2]))((0[1-9])|([1-2][0-9])|(3[0-1]))\d{3}$/.test(idCardNo);
if (!check) return false;
let addressCode = idCardNo.substring(0, 6);
let addressCode = idCardNo.substring(0, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Add a note too about this change.

Copy link
Member Author

@tux-tn tux-tn Jul 13, 2020

Choose a reason for hiding this comment

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

If you check the old implementation of checkAddressCode you can see that it accepts a 6 character string (addressCode), do a regexp check on it's length and verify if the first 2 characters are a valid provinceCode.
Since length check is unnecessary and already done in line 204, i am directly passing the provinceCode to checkAddressCode and it's a 2 characters numerical string

@profnandaa
Copy link
Member

@tux-tn -- sorry, I see some of the stuff has been covered in your comments above; perhaps just add a comment in the code for some of the changes.

@tux-tn
Copy link
Member Author

tux-tn commented Jul 19, 2020

@profnandaa what do you think about the switching to coverall? I think you can create the integration with your "Collaborator" access

@profnandaa
Copy link
Member

@tux-tn -- sure, we can try that, let me know what I should do.

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 profnandaa merged commit ebc6c86 into validatorjs:master Jul 20, 2020
@profnandaa profnandaa mentioned this pull request Sep 18, 2020
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.

2 participants