-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: exclude backslashes from initially generated passwords #170
Conversation
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.
In general it looks good to me, however I added a comment about testing this.
I think if we can't come up with a better alternative, I would just not test this case as it is too run dependent.
for _ in range(1000): | ||
password = create_users._generate_password() | ||
assert backslash_char not in 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 understand why this is tested like that, but perhaps there is a better way to test the behavior of the logic rather than the end result. To me this seems like it would make for a hard to understand sometimes failing / sometimes not test.
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.
good point. What about now?
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.
Looks good.
def test_generated_password_has_no_backslashes(): | ||
backslash_char = "\\" | ||
for _ in range(1000): | ||
password = create_users._generate_password() | ||
assert backslash_char not in 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.
Same comment as for the MongoDB 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.
Looks good to me now.
6b842e9
to
1ab8c4b
Compare
Refs: OPMONDEV-192