-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
add tests for password and fix an edge case #2741
Conversation
a6cced8
to
cb573da
Compare
@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. |
There was a problem hiding this 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.
Hey @stefannibrasil thank you for reviewing this :), I have updated and fixed all your review comments and ran the latest test. |
Awesome work @DeepakRaj228. I think this PR is ready to merge. |
lib/faker/default/internet.rb
Outdated
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) |
There was a problem hiding this comment.
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?
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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! 👀
…sonal:DeepakRaj228/faker into tests_for_password_generators
@DeepakRaj228 thank you for your contribution and flexibility. There are only some rubocop issues to solve, and then this can be merged 🎉 |
@DeepakRaj228 you can run rubocop locally to catch the issues before CI: https://github.com/faker-ruby/faker/blob/main/CONTRIBUTING.md#code-style 👀 |
Thanks @stefannibrasil, fixed the error. |
Summary
Found an invalid edge case where the password method returns a value.
Test results:
Other Information