-
Notifications
You must be signed in to change notification settings - Fork 9
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
DX-2689 Add MFA Integration Tests #93
Conversation
1st pass at writing integration tests
Use assertAuthException to match unittest assert names
No auth header causes a 401
Otherwise we hit a rate limit very quickly
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.
Please review comments, no major issues
from bandwidth.exceptions import ApiException, UnauthorizedException, ForbiddenException | ||
|
||
# seed the random number generator for the verify request | ||
seed(randint(10, 500)) |
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.
Wondering why seed
here and not in setup
?
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.
Its currently not working correctly so I need to fix this
scope="scope", | ||
message="Your temporary {NAME} {SCOPE} code is {CODE}", | ||
digits=6, | ||
) |
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.
The setup
function looks clean and variable
names are expressive, nice work.
Any chance to apply DRY principle?
|
||
api_response: VoiceCodeResponse = self.api_instance.generate_voice_code( | ||
self.account_id, self.voice_code_request) | ||
self.assertIs(type(api_response.call_id), str) |
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.
Would empty string be valid as per spec?
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.
No - same as above - will add a check to ensure its a non empty string
Requested by MFA team
These never get used due to the nature of the ApiException that gets raised. Overkill to initialize these by kwarg and test
Bump time.sleep from 30-35
sets seed value as system time by default - this works better for multiple tests in parallel
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.
Changes look good. Like the RateLimit
test.
No description provided.