Add some unit tests #92
Conversation
|
Hi, we don't use external dependencies for this project. You can fix your PR and remove deps? |
|
Ok @negasus will fix this one |
|
Hi @negasus just add some fixes, could you give some reviews. thanks |
|
Hi, from the remaining test failures, I found that we need to improve the |
renatosaksanni
force-pushed
the
feature/add-some-unit-tests
branch
from
5e39b01 to
130af6e
Compare
|
Hi @negasus , is there anything else that needs to be added or should we squash the commits for this PR? |
| @@ -32,7 +32,13 @@ func (b *Bot) WebhookHandler() http.HandlerFunc { | |||
| case <-req.Context().Done(): | |||
| b.error("some updates lost, ctx done") | |||
| return | |||
| default: | |||
There was a problem hiding this comment.
This code confuses me. What is the essence of his change?
There was a problem hiding this comment.
@negasus
In the first select block, default: is used to ensure that if the context has been canceled, it will immediately exit the select block and execute the error handling logic without waiting. This helps avoid sending the update if the context is already invalid.
There was a problem hiding this comment.
With default: The code checks for context cancellation non-blockingly, allowing it to proceed immediately if the context is not canceled. And if without default: then code will block until the context is canceled, potentially causing delays and inefficiencies.
There was a problem hiding this comment.
in summary the first select block will checks if the context is canceled. If it is, it logs an error and returns without sending the update to the channel.
And the second select block will sends the update to the bot.updates channel, or logs an error if the context is canceled during the send operation.
@negasus just to give you some context to the line changes, due to I ran into this error while creating and running the webhook_handler unit test:
FAIL: TestWebhookHandler_ContextDone (0.00s)
webhook_handler_test.go:171: Received update despite context cancellation
|
Hi @negasus Is there anything you're still confused about, or is everything resolved and ready to merge? |
Just check this repo and wanna contribute some unit tests