-
-
Notifications
You must be signed in to change notification settings - Fork 501
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
otp: add tests #1008
otp: add tests #1008
Conversation
0601149
to
307c052
Compare
Codecov Report
@@ Coverage Diff @@
## master #1008 +/- ##
==========================================
- Coverage 63.64% 63.57% -0.07%
==========================================
Files 172 173 +1
Lines 9932 9965 +33
==========================================
+ Hits 6321 6335 +14
- Misses 2824 2842 +18
- Partials 787 788 +1
Continue to review full report at Codecov.
|
Rebased and passing. (Not sure I agree that code coverage decreased as a result of this PR...) cc: @dominikschulz |
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.
LGTM. Not sure if I like the check function - would prefer table based tests - but easy to refactor and ok for now.
And yes, the coverage Delta looks wrong.
307c052
to
b442450
Compare
Signed-off-by: Cole Mickens <[email protected]>
b442450
to
f7dce83
Compare
@dominikschulz Thanks for the feedback! I went ahead and fixed the structure -- I'd been meaning to switch it to table-tests. I was going to fix the OTP code checking as well, but it seems like the change in go-kyle/twofactor is not yet sufficient to do this. |
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.
LGTM
Signed-off-by: Cole Mickens <[email protected]>
Signed-off-by: Cole Mickens <[email protected]>
Add tests for OTP functionality. Attempt to prevent future regressions like being discussed in #1006.
Note, this won't pass until one of the proposed fixes is merged:
Note, a case that I expected to work is not working. It's not listed in the docs, but I was still a bit surprised: