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

add tests for password and fix an edge case #2741

Merged

Conversation

DeepakRaj228
Copy link
Contributor

Summary

Found an invalid edge case where the password method returns a value.

Screenshot 2023-04-05 at 4 57 23 PM

  1. Have added an exception if the min length and max length are 0 in the Faker::Internet.password method.
  2. Updated the number range starting from 0.
  3. Added tests and covered the following existing flows.
  1. min_length must be at least 2 if mix_case: true && special_characters: false
    Faker::Internet.password(min_length: 10, max_length: 20, mix_case: true) #=> "3k5qS15aNmG"
  2. min_length must be at least 3 if mix_case: true && special_characters: true
    Faker::Internet.password(min_length: 10, max_length: 20, mix_case: true, special_characters: true) #=> "*%NkOnJsH4"

Test results:
Screenshot 2023-04-05 at 8 17 42 PM

Other Information

@DeepakRaj228 DeepakRaj228 force-pushed the tests_for_password_generators branch from a6cced8 to cb573da Compare April 5, 2023 15:28
@DeepakRaj228
Copy link
Contributor Author

@stefannibrasil, I have added a few test cases as you mentioned here. Please let me know if there are any other cases that need to be covered.

Copy link
Contributor

@stefannibrasil stefannibrasil left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you so much @DeepakRaj228 🙏

I've left some suggestions/questions, let me know if they make sense.

I think you covered most of them. The only one I can think of now is passing max_length only. Then, with it being less and bigger than min_length.

lib/faker/default/internet.rb Outdated Show resolved Hide resolved
lib/faker/default/internet.rb Show resolved Hide resolved
test/faker/default/test_faker_internet.rb Outdated Show resolved Hide resolved
test/faker/default/test_faker_internet.rb Show resolved Hide resolved
test/faker/default/test_faker_internet.rb Outdated Show resolved Hide resolved
test/faker/default/test_faker_internet.rb Outdated Show resolved Hide resolved
test/faker/default/test_faker_internet.rb Outdated Show resolved Hide resolved
@DeepakRaj228
Copy link
Contributor Author

Hey @stefannibrasil thank you for reviewing this :), I have updated and fixed all your review comments and ran the latest test.

Screenshot 2023-04-18 at 11 36 48 AM

@gkunwar
Copy link
Contributor

gkunwar commented Apr 18, 2023

Awesome work @DeepakRaj228.

I think this PR is ready to merge.

Comment on lines 162 to 163
raise ArgumentError, 'max_length must be more than min_length' if max_length < min_length
raise ArgumentError, 'min_length and max_length must have at least one character' if(min_length == 0 && max_length == 0)
Copy link
Contributor

@thdaraujo thdaraujo Apr 18, 2023

Choose a reason for hiding this comment

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

I think the first assertion message is misleading as min and max could be equal.

If that is true, and we can allow min_length == max_length, then we could be a bit more descriptive, as the base case would be:
min_length == 1 && max_length == 1.

Otherwise, min_length <= max_length where min and max are greater than 1. What do you think?

Suggested change
raise ArgumentError, 'max_length must be more than min_length' if max_length < min_length
raise ArgumentError, 'min_length and max_length must have at least one character' if(min_length == 0 && max_length == 0)
raise ArgumentError, 'min_length and max_length must be greater than or equal to one' if min_length < 1 || max_length < 1
raise ArgumentError, 'min_length must be smaller than or equal to max_length' unless min_length <= max_length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thdaraujo I agree with your point and the above suggestion is even clearer than the current one.

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh, thank you @thdaraujo for catching that! 👀

@stefannibrasil
Copy link
Contributor

@DeepakRaj228 thank you for your contribution and flexibility. There are only some rubocop issues to solve, and then this can be merged 🎉

@stefannibrasil
Copy link
Contributor

@DeepakRaj228 you can run rubocop locally to catch the issues before CI: https://github.com/faker-ruby/faker/blob/main/CONTRIBUTING.md#code-style 👀

@DeepakRaj228
Copy link
Contributor Author

Thanks @stefannibrasil, fixed the error.
Screenshot 2023-04-24 at 1 21 59 PM

@stefannibrasil stefannibrasil merged commit cfdcdc0 into faker-ruby:main Apr 24, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants