Create random vehicle vin numbers with the correct check-digit #2633

alextaujenis · 2022-11-25T17:02:59Z

Summary

The currently generated Vehicle VIN numbers are invalid when put through a VIN checker. (The 9th check-digit is incorrect).
There is dead code in the ruby class that looks like it was attempting to generate a valid 9th check-digit, but it's not currently implemented.
This PR changes the way this library generates and validates VIN numbers with the correct 9th check digit (see https://en.wikibooks.org/wiki/Vehicle_Identification_Numbers_(VIN_codes)/Check_digit)

Other Information

The previous regular expression in this library assumed the last 5 characters of the VIN are digits, but in the real world there are characters in the "Vehicle Serial Number" as well. The new generator takes this into consideration.

Randomly Failing Test (not from this PR)

@stefannibrasil it looks like there's a testing issue that needs to be resolved...

In the file test_ja_locale.rb there is a randomly failing test that I commented out:

This is not part of my PR, but this randomly failing test pushed my CI checks red.
The test validates that all Faker::Games::SuperMario.character in the ja locale are NOT English.
One of the Super Mario Characters is named クッパJr. (translated Bowser Jr.), which includes the English characters Jr. at the end.
This failing test has been commented out for this PR.
This test should be removed, or the Super Mario Characters of the ja locale shouldn't have /[a-zA-Z]/ characters in their name to satisfy this test.

jremes-foss · 2022-11-25T23:19:46Z

Hi Alex. Please hold off for now. I've found the reason why the unit test was failing and fixed the randomly failing unit test in separate pull request. This will be OK to merge after my change has gone through. Thanks.

alextaujenis · 2022-11-25T23:46:34Z

I've found the reason why the unit test was failing...

@Newman101 I laid that out very clearly in this PR and was asking for an opinion on how to proceed. I'm a little surprised you poached the issue like it was your own and performed a full write-up and PR instead of just collaborating with me on this open PR. You also missed the fact that コクッパJr., as well as コクッパ are in the yaml file, making your edit create a duplicate entry.

I'm probably not the first person to complain about the gamification of commits on github, but at least you helped make the decision on this issue through the lens of a regular contributor.

@Newman101 I correctly fixed the issue in this open PR, you can close your redundant PR.

jremes-foss · 2022-11-25T23:54:05Z

Sorry about poaching the issue, won't happen again. I've closed my pull request - please proceed.

alextaujenis · 2022-11-25T23:56:29Z

@Newman101 Thank you. I hope that didn't come off to strongly. It's just that I'm spending time babysitting this PR and if you take that fix out of here then I'll have to wait for your PR to be reviewed and merged, I'll have to merge your changes into my own fork, then I'll need to commit again, and then have my PR finally reviewed. Thanks for saving me some time.

jremes-foss · 2022-11-26T00:05:06Z

@alextaujenis No worries at all. I'll get your PR reviewed.

jremes-foss

LGTM.

alextaujenis · 2022-11-26T00:22:34Z

@Newman101 Reviewing this thread; I did stress that the randomly failing test is "not from this PR"...

I think that point was misunderstood as "I won't fix this issue". I was trying to communicate that I didn't create the random failure, but did perform due-diligence and needed an opinion to resolve the issue. I will try to communicate more clearly in the future.

Thanks for approving this PR.

stefannibrasil · 2022-11-27T01:51:53Z

@alextaujenis thank you for opening a PR. Sorry about the confusion that happened with the failing test. Could you leave that out from your PR? I will create an issue for it to be fixed later. The CI won't pass but I can approve the PR if it's the only test failing.

stefannibrasil · 2022-11-27T01:59:08Z

lib/faker/default/vehicle.rb

+        checksum = "#{front}A#{back}".chars.each_with_index.map do |char, i|
+          (char =~ /\A\d\z/ ? char.to_i : VIN_TRANSLITERATION[char.to_sym]) * VIN_WEIGHT[i]
+        end.inject(:+) % 11


What do you think of moving all this logic into a method? There is one for a singapore checksum, it would be nice to follow that.

Could we also use an if/else for this logic? Ternary operators are hard to read. The regexes already add complexity to the file.

What do you think of moving all this logic into a method? There is one for a singapore checksum, it would be nice to follow that.

I could follow the example of the singapore checksum, but I'm hesitant because that method relies upon the regexify method in this library, which literally has the disclaimer of "you shouldn't use it" in the notes above it due to performance and the possibility of "it blows up on you in a spectacular fashion". Also, the singapore checksum is for license plate numbers and the check digit is appended to the end of the license plate number. For VIN numbers the check digit is in the middle of the string (it's the 9th character out of 17 characters). The check digit is injected after generating an invalid VIN, which is a little more confusing than just appending it to a partial license plate number.

The only requirements that I used to tdd this functionality were:

Generate a valid VIN -> return string

Verify if a VIN is valid -> return bool

I could break this generator up to satisfy the requirement of "return a valid VIN checksum when provided an invalid VIN" to look like the singapore method, but that isn't a base requirement of the library and I'm not sure why this is necessary. If you don't want a few lines of code in the public Faker::Vehicle.vin method: I could create a private random_vin method and call it from the public method, but that seems pedantic as well.

If you have other thoughts on how to refactor this then please show me with example code.

Could we also use an if/else for this logic? Ternary operators are hard to read. The regexes already add complexity to the file.

I refactored the code to mimic the ternary operator found inside of singapore_checksum. Let me know if that improved readability or if this needs to be taken further.

Oh, I get it. It's good for now, thanks!

lib/locales/ja/super_mario.yml

stefannibrasil · 2022-11-27T02:00:34Z

test/faker/default/test_faker_vehicle.rb

+    if vin && vin =~ Faker::Vehicle::VIN_REGEX
+      total = 0
+      vin.chars.each_with_index do |char, index|
+        total += (char =~ /\A\d\z/ ? char.to_i : Faker::Vehicle::VIN_TRANSLITERATION[char.to_sym]) * Faker::Vehicle::VIN_WEIGHT[index]


Same here about the ternary operator. It's nice to have tests serving as self-documentation, so readability is important.

This was refactored. Let me know if that improved readability or if this needs to be taken further.

…locale to un-fix a randomly failing test

stefannibrasil

Thank you for opening a PR!

stefannibrasil · 2022-11-29T01:57:58Z

The spec that is failing is not related to the changes in this PR.

alextaujenis added 3 commits November 25, 2022 10:01

Create random vehicle vin numbers with the correct check-digit

0118731

Fix test issue

4d3ab25

Fix randomly failing test

09db98b

jremes-foss added a commit to jremes-foss/faker-1 that referenced this pull request Nov 25, 2022

Fix randomly failing unit test spotted in pull request faker-ruby#2633

84fcfae

Fix randomly failing test for ja locale

fef5703

jremes-foss approved these changes Nov 26, 2022

View reviewed changes

stefannibrasil reviewed Nov 27, 2022

View reviewed changes

alextaujenis added 2 commits November 27, 2022 12:46

Refactor ternary operator for readability

38aead0

After discussion on PR#2633; adding Bowser jr. back into super mario …

0bacea8

…locale to un-fix a randomly failing test

stefannibrasil approved these changes Nov 29, 2022

View reviewed changes

stefannibrasil merged commit c2db3d8 into faker-ruby:main Nov 29, 2022

stefannibrasil mentioned this pull request Nov 29, 2022

Random spec failing: Failure: test_ja_supermario_methods(TestJaLocale) #2637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create random vehicle vin numbers with the correct check-digit #2633

Create random vehicle vin numbers with the correct check-digit #2633

alextaujenis commented Nov 25, 2022 •

edited

Loading

jremes-foss commented Nov 25, 2022

alextaujenis commented Nov 25, 2022 •

edited

Loading

jremes-foss commented Nov 25, 2022

alextaujenis commented Nov 25, 2022

jremes-foss commented Nov 26, 2022 •

edited

Loading

jremes-foss left a comment

alextaujenis commented Nov 26, 2022

stefannibrasil commented Nov 27, 2022

stefannibrasil Nov 27, 2022

alextaujenis Nov 27, 2022

stefannibrasil Nov 29, 2022

stefannibrasil Nov 27, 2022

alextaujenis Nov 27, 2022

stefannibrasil left a comment

stefannibrasil commented Nov 29, 2022

Create random vehicle vin numbers with the correct check-digit #2633

Create random vehicle vin numbers with the correct check-digit #2633

Conversation

alextaujenis commented Nov 25, 2022 • edited Loading

Summary

Other Information

Randomly Failing Test (not from this PR)

jremes-foss commented Nov 25, 2022

alextaujenis commented Nov 25, 2022 • edited Loading

jremes-foss commented Nov 25, 2022

alextaujenis commented Nov 25, 2022

jremes-foss commented Nov 26, 2022 • edited Loading

jremes-foss left a comment

Choose a reason for hiding this comment

alextaujenis commented Nov 26, 2022

stefannibrasil commented Nov 27, 2022

stefannibrasil Nov 27, 2022

Choose a reason for hiding this comment

alextaujenis Nov 27, 2022

Choose a reason for hiding this comment

stefannibrasil Nov 29, 2022

Choose a reason for hiding this comment

stefannibrasil Nov 27, 2022

Choose a reason for hiding this comment

alextaujenis Nov 27, 2022

Choose a reason for hiding this comment

stefannibrasil left a comment

Choose a reason for hiding this comment

stefannibrasil commented Nov 29, 2022

alextaujenis commented Nov 25, 2022 •

edited

Loading

alextaujenis commented Nov 25, 2022 •

edited

Loading

jremes-foss commented Nov 26, 2022 •

edited

Loading