Skip to content

Error Handler tests#954

Merged
kosayoda merged 48 commits into
python-discord:mainfrom
ks129:error-handler-test
Apr 28, 2021
Merged

Error Handler tests#954
kosayoda merged 48 commits into
python-discord:mainfrom
ks129:error-handler-test

Conversation

@ks129
Copy link
Copy Markdown
Contributor

@ks129 ks129 commented May 19, 2020

Closes #575

About PR

  • Created tests for all error handler functions
  • Made 2 small changes in handler itself:
    • In 3ea33bc I changed hasattr to getattr to make testing possible.
    • In e950def made that this request coroutine only when this is required to avoid not awaited coroutines.
  • In 28e962b added new variable to class for testing.

ks129 added 30 commits May 18, 2020 11:15
Created test that make sure when error is already handled in local error
handler, this don't await `ctx.send` to make sure that this don't move
forward.
- Added `assertIsNone` to `test_error_handler_already_handled`.
- Removed `ErrorHandlerTests.cog`, moved it to each test recreation.
Replaced `hasattr` with `getattr` for unit tests
Added `invoked_from_error_handler` attribute that is `False` default.
Added test for case when `Context.invoked_from_error_handler`
is `True`
These 3 errors is:
- `ConversionError`
- `MaxConcurrencyReached`
- `ExtensionError`
@coveralls
Copy link
Copy Markdown

coveralls commented Nov 21, 2020

Coverage Status

Coverage increased (+2.3%) to 58.92% when pulling 3855b4f on ks129:error-handler-test into aa5e39c on python-discord:master.

Copy link
Copy Markdown
Contributor

@Senjan21 Senjan21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the one comment, tests look excellent to me, thanks.

Comment on lines +63 to +65
if case["patch_verification_id"]:
with patch("bot.exts.backend.error_handler.Channels.verification", new=1234):
self.assertIsNone(await cog.on_command_error(self.ctx, error))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have verification channel anymore so the this is redundant, same goes for any other part of the code that relates to patching verification channel id

@ks129 ks129 requested a review from Akarys42 as a code owner February 19, 2021 10:32
Copy link
Copy Markdown
Contributor

@Senjan21 Senjan21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thanks for adding tests for newly introduced errors. Amazing quality overall! Thanks.

Base automatically changed from master to main March 13, 2021 19:40
@Senjan21 Senjan21 added p: 3 - low Low Priority and removed p: 2 - normal Normal Priority labels Mar 18, 2021
@Xithrius Xithrius requested review from Den4200 and kosayoda April 22, 2021 06:37
@Xithrius Xithrius added the t: feature New feature or request label Apr 22, 2021
This was removed in the branding manager rewrite:
#1463
BadUnionArgument sends command help after:
#1434
@kosayoda kosayoda enabled auto-merge April 28, 2021 08:52
Copy link
Copy Markdown
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for @kosayoda

@kosayoda kosayoda merged commit b19b1cf into python-discord:main Apr 28, 2021
@ks129 ks129 deleted the error-handler-test branch April 28, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests Related to tests (e.g. unit tests) p: 3 - low Low Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write unit tests for bot/cogs/error_handler.py

8 participants