-
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 digits to password #2705
add digits to password #2705
Conversation
Hi! Thank you for the PR! |
I'm leaning towards treating it as a bug instead of passing digits as a parameter and having to update the documentation, but I think either way works! |
assert_match(/[a-z]/, password) | ||
assert_match(/[A-Z]/, password) |
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 that might be already covered by the test test_password_with_mixed_case in the line 153 (148 on the main).
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 it's okay to have it here as well because this is testing the case of when there is no mix case argument.
Not really! This is a bug. The generator is intended to use digits, and not as a parameter. Fixing the bug with regression tests is the solution here. |
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.
hi @wyattroyc thanks for submitting a PR and for the patience. I will merge this as it fixes the bug.
Unfortunately, the tests don't seem to be exercising the code as we'd expect. For example, if you simply change the expectation here:
def test_password
assert_match(/\w{3}/, @tester.password)
end
to expect any number, the test pass. I have some ideas to improve the tests to catch those bugs and have a reliable test suite for this generator. For now, thank you for the contribution!
and thank you @MicBruz for the comments!
assert_match(/[a-z]/, password) | ||
assert_match(/[A-Z]/, password) |
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 it's okay to have it here as well because this is testing the case of when there is no mix case argument.
Summary
Adds digits to generated passwords
Fixes Issue #2704
Other Information