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.

Sign up for GitHub

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

Jump to bottom

fix: Luhn generation algorithms and tests #980

Merged
merged 17 commits into from
May 25, 2022
Merged

fix: Luhn generation algorithms and tests #980

merged 17 commits into from
May 25, 2022

Conversation

ejcheng
Copy link
Member

@ejcheng ejcheng commented May 21, 2022

fixes #952

@ejcheng ejcheng added c: bug Something isn't working p: 1-normal Nothing urgent c: test labels May 21, 2022
@ejcheng ejcheng added this to the v7 - Current Major milestone May 21, 2022
@ejcheng ejcheng self-assigned this May 21, 2022
@ejcheng
Copy link
Member Author

ejcheng commented May 21, 2022

@prisis Seems like the new luhnCheck function I pasted in causes tests to fail...👀

https://github.com/faker-js/faker/runs/6537655385?check_suite_focus=true

@ejcheng
Copy link
Member Author

ejcheng commented May 21, 2022 •
edited
Loading

Just tested the function: https://replit.com/@ericcheng2005/luhnTest#index.js

Made sure the testcases I used in the Replit above were right too:
Screen Shot 2022-05-21 at 12 11 42 PM

@ST-DDT
Copy link
Member

ST-DDT commented May 21, 2022

Probably because the luhn value we are generating is broken too.
Maybe we should change that.
I created #981 to improve the error messages for the failures.

@ejcheng
Copy link
Member Author

ejcheng commented May 21, 2022 •
edited
Loading

Probably because the luhn value we are generating is broken too. Maybe we should change that. I created #981 to improve the error messages for the failures.

Yeah, there's something wrong with helpers.replaceCreditCardSymbols.

@codecov
Copy link

codecov bot commented May 23, 2022 •
edited
Loading

Codecov Report

Merging #980 (2375fdc) into main (4e38a70) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #980   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files        2111     2112    +1     
  Lines      225957   225980   +23     
  Branches      979      981    +2     
=======================================
+ Hits       225217   225240   +23     
  Misses        720      720           
  Partials       20       20
Impacted Files Coverage Δ
src/modules/helpers/index.ts 97.81% <100.00%> (-0.09%) ⬇️
src/modules/helpers/luhn-check.ts 100.00% <100.00%> (ø)

ejcheng
ejcheng commented May 23, 2022
View reviewed changes
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@ejcheng ejcheng marked this pull request as ready for review May 23, 2022 18:01
@ejcheng ejcheng requested a review from a team as a code owner May 23, 2022 18:01
@ejcheng ejcheng changed the title fix: luhnCheck algorithm fix: Luhn generation algorithms and tests May 23, 2022
@ejcheng ejcheng requested a review from a team May 23, 2022 18:02
@ejcheng
Copy link
Member Author

ejcheng commented May 23, 2022

Note: all of the numbers I changed the seeded tests to expect are Luhn-valid numbers, verified using https://simplycalc.com/luhn-validate.php

prisis
prisis approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@prisis prisis left a comment

Choose a reason for hiding this comment

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

Good job :)

@Shinigami92 Shinigami92 enabled auto-merge (squash) May 25, 2022 16:51
@Shinigami92 Shinigami92 merged commit c95826f into main May 25, 2022
@Shinigami92 Shinigami92 deleted the luhn_investigate branch May 25, 2022 17:00
Minozzzi pushed a commit to Minozzzi/faker that referenced this pull request Jul 19, 2022
@ejcheng @ST-DDT @Minozzzi
fix: Luhn generation algorithms and tests (faker-js#980)
d73b6cc 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@xDivisionByZerox xDivisionByZerox added the m: helpers Something is referring to the helpers module label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

@prisis prisis prisis approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

Assignees

@ejcheng ejcheng

Labels
c: bug Something isn't working c: test m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v7 - Current Major
Development

Successfully merging this pull request may close these issues.

Luhn algorithm generation not working
5 participants
@ejcheng @ST-DDT @prisis @Shinigami92 @xDivisionByZerox