Remove extraneous traceback and simplify string repr of ConftestImportFailure #7222
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asottile
approved these changes
May 17, 2020
| @@ -511,9 +516,10 @@ def _importconftest(self, conftestpath): | |||
| # Using Path().resolve() is better than py.path.realpath because | |||
| # it resolves to the correct path/drive in case-insensitive file systems (#5792) | |||
| key = Path(str(conftestpath)).resolve() | |||
| try: | |||
| return self._conftestpath2mod[key] | |||
| except KeyError: | |||
There was a problem hiding this comment.
iirc we could still keep this structure but instead do
with contextlib.suppress(KeyError):
return ...
...There was a problem hiding this comment.
Great suggestion, it makes the code easier to follow, thanks.
I've also refactored the check for non-top-level pytest_plugins declaration, I believe it makes the code even easier to follow then.
This removes the KeyError from the traceback chain when an
conftest fails to import:
return self._conftestpath2mod[key]
E KeyError: WindowsPath('D:/projects/pytest/.tmp/root/foo/conftest.py')
During handling of the above exception, another exception occurred:
...
raise RuntimeError("some error")
E RuntimeError: some error
During handling of the above exception, another exception occurred:
...
E _pytest.config.ConftestImportFailure: (...)
By slightly changing the code, we can remove the first chain, which is often
very confusing to users and doesn't help with anything.
Fix pytest-dev#7223
The default message is often hard to read:
E _pytest.config.ConftestImportFailure: (local('D:\\projects\\pytest\\.tmp\\root\\foo\\conftest.py'), (<class 'RuntimeError'>, RuntimeError('some error',), <traceback object at 0x000001CCC3E39348>))
Using a shorter message is better:
E _pytest.config.ConftestImportFailure: RuntimeError: some error (from D:\projects\pytest\.tmp\root\foo\conftest.py)
And we don't really lose any information due to exception chaining.
Decided to move the 'if' logic together with the error message, as this leaves the _importconftest method cleaner.
nicoddemus
force-pushed
the
remove-key-error-conftest-exception
branch
from
00447cc to
c26f389
Compare
bluetech
approved these changes
May 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the
KeyErrortraceback from the chain when an conftest fails to import:By slightly changing the code, we can remove the first chain, which is often confusing to users and doesn't help with anything.
Also while at it, simplified the string representation of
ConftestImportFailurefrom:To:
Fix #7223