Conversation
KnorpelSenf
left a comment
KnorpelSenf
left a comment
There was a problem hiding this comment.
A few nitpicks, a few needless usages of fake time. Overall, very solid!
arunr-inji
force-pushed
the
test/improve-bot-coverage
branch
from
d91545b to
7e73bbb
Compare
|
Still some unaddressed comments from the previous review about needles fake time usage (unless it's needed and I don't understand why), feel free to ping me once this is ready |
Yeah I meant to convert this into a draft again. I'll do that and re open for review! |
|
Top notch |
arunr-inji
force-pushed
the
test/improve-bot-coverage
branch
from
e27f445 to
a19ccf6
Compare
I think I addressed your feedback + added 3 more tests (mentioned in my other comment) |
| } | ||
| }); | ||
|
|
||
| it("should retry on HttpError during initialization", async () => { |
There was a problem hiding this comment.
@KnorpelSenf added this and two more tests below (5xx, 429 handling) to this describe block since you last reviewed
There was a problem hiding this comment.
Very cool! That's a great addition.
Btw why doesn't the library actually perform a sleep here? I assumed it would do a 3 second delay, and you're not faking time ... but the test execution is still fast. How?
There was a problem hiding this comment.
Based on my understanding, even though these are errors with strategy set to retry, the delay is only enforced after at least one error and not for the first retry:
Line 651 in 94287de
But the sleep duration in that case would be 50ms * 2 right? and so on right?
There was a problem hiding this comment.
Or perhaps you were referring to the 3s delays for handlePollingError?
the retry + delay + retry functionality is actually not tested; For errors during getUpdates, we only have tests below for the basic error handling functionality. Do you think that is something that is worth adding tests for?
There was a problem hiding this comment.
Ah great point, the first error is retried immediately. This is reasonable because of a quirk in the Bot API server.
The tests are fine the way they are, let's merge them now. In a subsequent PQ, it is worth improving the tests, for the retry mechanism in particular.
KnorpelSenf
left a comment
KnorpelSenf
left a comment
There was a problem hiding this comment.
A bunch of stubs are not cleaned up properly, and a few usages can be simplified. All of my comments are nitpicks, I assume that you can just commit my suggestions (perhaps take a look if I messed up) and then we can merge. Almost there!
Co-authored-by: KnorpelSenf <shtrog@gmail.com>
I committed your changes but had to fix some lint failures so pushed a commit. Thanks for the thorough reviews! |
2 participants
Description
This PR adds comprehensive test coverage for
bot.tsCloses #851
Test Coverage Improvements