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

test(internet): fix username expecting numbers with length 2 #1284

Merged

Conversation

xDivisionByZerox
Copy link
Member

Found in #1059.

The implementation of internet.username() uses the provided first and last name and can append a random number to them. This number is between 0 and 99 (inclusive).

result = `${firstName}${this.faker.helpers.arrayElement([
'.',
'_',
])}${lastName}${this.faker.datatype.number(99)}`;

The test on the other hand side uses a regex that expects the email to have a number (if existing) that has at least a length of 2.

This PR fixes the described issue.

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent c: test m: internet Something is referring to the internet module labels Aug 18, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team August 18, 2022 18:03
@xDivisionByZerox xDivisionByZerox self-assigned this Aug 18, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner August 18, 2022 18:03
@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #1284 (3830368) into main (93ef876) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1284      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2154     2154              
  Lines      239914   239914              
  Branches     1003     1002       -1     
==========================================
- Hits       239015   239014       -1     
- Misses        878      879       +1     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/finance/index.ts 99.77% <0.00%> (-0.23%) ⬇️

@Shinigami92
Copy link
Member

Please use the new PR title naming convention: https://github.com/faker-js/faker/blob/main/CONTRIBUTING.md#committing

@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Aug 18, 2022
@ST-DDT ST-DDT added this to the v7 - Current Major milestone Aug 18, 2022
@xDivisionByZerox xDivisionByZerox changed the title test(internet.username): fix expecting numbers with length 2 test(internet): fix username expecting numbers with length 2 Aug 18, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Aug 18, 2022

Maybe it wanted to generate 00-09?
Or why did this never failed?

@ST-DDT
Copy link
Member

ST-DDT commented Aug 18, 2022

Or why did this never failed?

Because it doesn't generate random numbers in the test currently.

@xDivisionByZerox
Copy link
Member Author

Maybe it wanted to generate 00-09? Or why did this never failed?

It did fail at one point in #1059. That's why that PR has a dedicated commit to fix that issue. But since this issue can potentially hit any other contributor at any time I wanted to take it out of that PR.

Commit ref.

@Shinigami92 Shinigami92 merged commit a372c8c into main Aug 18, 2022
@Shinigami92 Shinigami92 deleted the test/internet/fix-username-always-expeecting-2-numbers branch August 18, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: test m: internet Something is referring to the internet module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants