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 expiration argument back to GithubIntegration.create_jwt #2439
Conversation
4f3913f
to
2e82f31
Compare
Codecov ReportBase: 98.77% // Head: 98.77% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #2439 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 117 117
Lines 11674 11677 +3
=======================================
+ Hits 11531 11534 +3
Misses 143 143
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Looking good, just one niggle.
@@ -61,6 +61,27 @@ def testCreateJWT(self): | |||
) | |||
sys.modules["time"].time = self.origin_time | |||
|
|||
def testCreateJWTWithExpiration(self): | |||
self.origin_time = sys.modules["time"].time | |||
sys.modules["time"].time = lambda: 1550055331.7435968 |
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 feels awkward. Why not use a mock 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.
I don't like it either, just copied from existing testCreateJWT
above.
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.
Then we should fix both places, and I'd rather circle back and fix one, rather than both.
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 meant to be a quick fix, to get a fix for 1.58.0 out, asap. People are rightly annoyed: #2430 (comment)
Cleaning up test code can be done for 1.59.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 don't have time to cut a release right now, but I'll get this merged.
1172288
to
57e8977
Compare
Fixes #2432.