RD-4296 - mimic AWS import strategy of the original handler #175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
==========================================
+ Coverage 96.21% 97.44% +1.22%
==========================================
Files 11 22 +11
Lines 952 1762 +810
==========================================
+ Hits 916 1717 +801
- Misses 36 45 +9 Continue to review full report at Codecov.
|
| raise RuntimeError( | ||
| f"Invalid handler format: Bad handler '{os.environ[ORIGINAL_HANDLER_KEY]}': not enough values to unpack (expected 2, got 1)" | ||
| ) | ||
| ) from None |
There was a problem hiding this comment.
Why are you adding from None? The user will lose the context of the exception:
Same regarding the other from None is this file
There was a problem hiding this comment.
Yep, I tried to copied AWS behavior:
| raise ImportError( | ||
| f"Unable to import module '{handler_module}': No module named '{handler_module}'" | ||
| ) | ||
| f"Runtime.ImportModuleError: Unable to import module '{handler_module}': {str(e)}" |
There was a problem hiding this comment.
Here you even add of original exception in the text. Don't you prefer to do from e instead?
There was a problem hiding this comment.
I wanted to copy AWS's bootstrap default behavior:
| def test_import_error(monkeypatch): | ||
| monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "blabla.not.exists") | ||
|
|
||
| with pytest.raises(ImportError): |
There was a problem hiding this comment.
Why not do it with pytest.raises?
Same issue regarding the other tests in this file as well.
There was a problem hiding this comment.
That's a great question!
I did use pytest.raises in the first place, but then I noticed that it removes the context of the message, and I want to validate that there was no context (to follow AWS behavior!) :D
There was a problem hiding this comment.
Add a comment explaining why don't we use it
| def test_import_error(monkeypatch): | ||
| monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "blabla.not.exists") | ||
|
|
||
| with pytest.raises(ImportError): |
There was a problem hiding this comment.
Add a comment explaining why don't we use it
No description provided.