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

fix(helpers): prevent uniqueArray from hanging #2239

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Jul 5, 2023

fix #2207

Adds a max retry logic to uniqueArray.

I chose to use 1000*length rather than a fixed number of retries, as for example if you are
asking for a length of 2000, then 1000 retries would be insufficient.

This seems sufficient for most sensible use cases.

Obviously uniqueArray has no way to "know" how many unique values the function can return, you could define a function which returns "heads", "tails", but occasionally returns "edge" every 1000000 runs, or if the system date is a Thursday, etc etc.

@matthewmayer matthewmayer requested a review from a team as a code owner July 5, 2023 08:09
@matthewmayer matthewmayer self-assigned this Jul 5, 2023
@matthewmayer matthewmayer added c: bug Something isn't working p: 1-normal Nothing urgent m: helpers Something is referring to the helpers module labels Jul 5, 2023
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

The implementation looks good. The failing pipeling seems to be unrelated to your changes. So I already approve 👍

@xDivisionByZerox xDivisionByZerox added the s: accepted Accepted feature / Confirmed bug label Jul 5, 2023
@matthewmayer
Copy link
Contributor Author

The implementation looks good. The failing pipeling seems to be unrelated to your changes. So I already approve 👍

Yea that codecov job is quite flaky and seems to fail every so often at random.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 5, 2023

Instead of reinventing the unique logic, shouldn't we reuse the unique method/logic?

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #2239 (8db08c5) into next (052a00c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2239   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2639     2639           
  Lines      244720   244728    +8     
  Branches     1082     1081    -1     
=======================================
+ Hits       243735   243744    +9     
+ Misses        958      957    -1     
  Partials       27       27           
Files Changed Coverage Δ
src/modules/helpers/index.ts 99.09% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

@matthewmayer
Copy link
Contributor Author

Instead of reinventing the unique logic, shouldn't we reuse the unique method/logic?

I thought that was deprecated?

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jul 9, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Jul 9, 2023

Let's discuss this in the next team meeting.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 27, 2023

Team Decision

We consider this a bandaid fix that solves the issue.
In the future we will reevaluate the entire unique feature again and then also use it consistently in all our unique methods.

@ST-DDT ST-DDT changed the title fix(helpers): prevents helpers.uniqueArray hanging for functions with insufficient values to satisfy length fix(helpers): prevent uniqueArray from hanging Jul 27, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) July 27, 2023 15:47
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Jul 27, 2023
@ST-DDT ST-DDT merged commit 3dece09 into faker-js:next Jul 27, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

County method used with uniqueArray helper crashes faker.
5 participants