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

Thread safety #2520

Merged
merged 2 commits into from
Aug 13, 2022
Merged

Thread safety #2520

merged 2 commits into from
Aug 13, 2022

Conversation

kiskoza
Copy link
Contributor

@kiskoza kiskoza commented Aug 8, 2022

Issue#

#1700

Description:

In my project, I have a lot of specs, so I run them in parallel to get quicker results, usually in a few threads. I also have some special specs dealing with mocked data, where I need a random first name, but it can't be the same as the initial data. At this point, I started to use the unique generators, something like this:

before do
  generator = Faker::Name.unique
  generator.exclude(:first_name, [], %w[Adam]) }
  @first_name = generator.first_name
  generator.clear # have a lot of test, don't want to run out of names just because one is forbidden
end

It worked fine when I was running only one of the tests, but as soon as I started them in parallel, they became flaky and sometimes it even generated the excluded first name.

I saw that others also had problems with thread safety, so here I am, trying to fix it. I added two new test cases, one is checking the thread safety of the Config.random (code derived from the linked isssue), the other is covering my problem with unique generators having their own sets of exlusions.

I hope my PR helps and could be merged soon, just let me know if you need anything

@Zeragamba
Copy link
Contributor

Looks good to me, but i'd like someone else from the faker team to have a look as well before merging.

Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

also, can confirm that this PR fixes #1700

(see discussion here)

@thdaraujo thdaraujo merged commit 6aecb58 into faker-ruby:master Aug 13, 2022
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.

3 participants