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

Use deterministic helper to execute the test provided in the block successive number of times #2813

Closed
stefannibrasil opened this issue Aug 19, 2023 · 9 comments · Fixed by #2816

Comments

@stefannibrasil
Copy link
Contributor

faker has a deterministic_verify test helper that we can use for verifying the randomness of generators multiple times.

We have some tests using 10.times, 100.times, 1000.times blocks that we want to replace with the test helper mentioned above. Here are some examples of places that can be improved using the deterministic helper:

And other examples of tests using the helper:

https://github.com/faker-ruby/faker/blob/main/test/faker/default/test_faker_internet.rb#L61

Expected Outcomes

  • replace existing tests using loops with the deterministic helper

This issue is great for a beginner, so please choose another way to contribute to faker if you have already contributed to other open source projects.

@fernandomenolli
Copy link
Contributor

Hello,
I am a junior/mid level ruby on rails dev, I am looking for some open source projects to contribute, because I believe it helps a lot the learning journey.

Would this issue to be something like this?

 # Faker::Lovecraft.word generates random word from wordlist

 def test_word
   deterministically_verify -> { @tester.word }, depth: 5 do |word|
     assert_includes @wordlist, word
   end
 end
# test/test_faker.rb:16

  def test_numerify
    deterministically_verify -> { Faker::Base.numerify('###') }, depth: 5 do |result|
      assert_match(/[1-9]\d{2}/, result)
    end
  end

If so, I think I can take this one.

In the case we have 1000.times loop, like Lovecraft.word generator, should we use depth: 40 or any other number near this?

@ak2-lucky
Copy link

I want to do this myself!

ak2-lucky added a commit to ak2-lucky/faker that referenced this issue Aug 24, 2023
@ak2-lucky
Copy link

@stefannibrasil
I modified it and created a PR.
Please check it out!

#2815

@stefannibrasil
Copy link
Contributor Author

Hi @fernandomenolli yes, you can follow the existing tests implementation for the new ones.

About the depth, I don't think we need a high number. 4 or 5 looks good. Thank you!

@stefannibrasil
Copy link
Contributor Author

Hi @ak2-lucky thank you for your contribution! @fernandomenolli already has shared an interest in taking this one, I just didn't have the time to assign the issue to them earlier this week. I hope you understand. Thanks!

@fernandomenolli
Copy link
Contributor

Thanks @stefannibrasil, gonna work on it.

@ak2-lucky
Copy link

Oh, Sorry @fernandomenolli and @stefannibrasil
I've already fixed it, but I'll leave it to @fernandomenolli

@stefannibrasil
Are there any other tasks I can do?

@fernandomenolli
Copy link
Contributor

@stefannibrasil I just opened a PR to this issue. Feel free to let me know if anything needs to be improved.

@stefannibrasil
Copy link
Contributor Author

no worries, @ak2-lucky thanks for understanding.

I don't have details yet as this current issue but one thing we always need help with is checking the locales files, and seeing if they are working as expected. I guess trying some generators with a specific locale (portuguese, italian, etc.) and seeing if they break with the locales, would be helpful.

stefannibrasil pushed a commit that referenced this issue Aug 31, 2023
…2813 issue (#2816)

* Changed tests from x.times loop to deterministically_verify helper

* Removed an unecessary depth value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants