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

NHS: fix occasional bad numbers #1425

Merged

Conversation

ChaoticBoredom
Copy link
Contributor

As I've been working in this repo, I noticed the test for this code was being skipped. On investigation, it was because this generator was not truly accurate, and the test would occasionally fail (as it should have)
The generator would create a final digit for the generated number, but did not handle the edge cases when this 'check digit' was 10 or 11.
In the case when the check digit is 10, the entire number is invalid. In this situation, I subtracted one from the generated number which would then be guaranteed to be a valid number.
In the case when the check digit is 11, it should be returned as a 0, not as 11.
Rules were read from the wikipedia page that was linked in the original PR that introduced this faker.
These changes have tests, and the original test that set me onto this problem is no longer omitted, and should be consistently passing now.

Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

I investigated this failing test but I couldn't fix it and omitted the test, so other contributors could see it and fix whenever they feel comfortable.

Thanks for the explanation in the description the PR 👍

@vbrazo vbrazo merged commit f9782d7 into faker-ruby:master Oct 20, 2018
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Add additional tests around NHS number edge cases

* Add code to handle check digit 10 and 11 edge cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants