re-export shared bot exceptions instead of redefining them #598
Conversation
matt-codecov
changed the title
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #598 +/- ##
==========================================
- Coverage 97.57% 97.56% -0.01%
==========================================
Files 429 429
Lines 35951 35958 +7
==========================================
+ Hits 35079 35084 +5
- Misses 872 874 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #598 +/- ##
==========================================
- Coverage 97.57% 97.56% -0.01%
==========================================
Files 429 429
Lines 35951 35958 +7
==========================================
+ Hits 35079 35084 +5
- Misses 872 874 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #598 +/- ##
==========================================
- Coverage 97.57% 97.56% -0.01%
==========================================
Files 429 429
Lines 35951 35958 +7
==========================================
+ Hits 35079 35084 +5
- Misses 872 874 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #598 +/- ##
==========================================
- Coverage 97.61% 97.61% -0.01%
==========================================
Files 464 464
Lines 37157 37164 +7
==========================================
+ Hits 36271 36276 +5
- Misses 886 888 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
This change has been scanned for critical changes. Learn more |
| mock_owner_bot = mocker.patch( | ||
| "shared.bots.repo_bots.get_owner_or_appropriate_bot" | ||
| ) | ||
| mock_owner_bot.side_effect = OwnerWithoutValidBotError() |
There was a problem hiding this comment.
Technically this mock returns the locally imported helpers.exceptions.OwnerWithoutValidBotError, which could still exhibit the same problem (as in: being a different type from shared.bots.exceptions.OwnerWithoutValidBotError)
There was a problem hiding this comment.
this is a "gotcha" for mocking (if foo.py does from bar import baz, you have to mock foo.baz instead of bar.baz), but from what i was reading python still understands OwnerWithoutValidBotError is the same type regardless of the import path
worker copy/pastes the definitions of exception types that live in shared now. worker code tries to catch the worker redefinitions, but shared is throwing the original shared definitions. even though they're named/defined identically, they're totally unrelated types. so the exceptions aren't caught
i added a unit test for the specific case i saw where an exception was not getting caught. there may be others