-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make KeyboardInterrupt cancel pytest #10279
Conversation
be35731
to
4a7be3d
Compare
from _pytest.runner import CallInfo | ||
from _pytest.runner import CollectReport | ||
from _pytest.runner import TestReport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like these may all be available in the public API instead of _pytest
. pytest-dev/pytest#7469
Perhaps not applicable after checking above, but I expect isort might prefer these to be all together? If not we may want to adjust a setting for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the version of pytest I have they are not exposed under pytest
. That was my first attempt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't pay attention whether pre-commit
ran isort
for me or not. But it should have, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find I frequently have to explicitly run pre-commit run --all
to have checks be run..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python 3.9.5 (default, Jun 3 2021, 15:18:23)
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pytest
>>> pytest.__version__
'7.0.1'
>>> from pytest import CallInfo, CollectReport, TestReport
>>>
If you have latest pytest and it isn't working for you I would be curious to know. We could dig into that on keybase.
from _pytest.runner import CallInfo | |
from _pytest.runner import CollectReport | |
from _pytest.runner import TestReport | |
from pytest import CallInfo, CollectReport, TestReport |
On the pre-commit, it's default is to only run on 'modified' files. I'm not sure the definition of that. I personally just manually run it with --all
.
tests/conftest.py
Outdated
call: CallInfo[Any], | ||
report: Union[CollectReport, TestReport], | ||
) -> None: | ||
pytest.exit("Cancelled by exception") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.pytest.org/en/6.2.x/reference.html#pytest.hookspec.pytest_exception_interact
It sounds like this would terminate tests early on an uncaught exception in a test? This is really required as opposed to just the sys.exit()
removal above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the sys.exit()
code that I removed did not contribute meaningfully to anything. That exception was also caught by pytest, just like KeyboardInterrupt
is. This hook is called later, for tests (or collections) that resulted in an exception. pytest.exit()
throws a special exception that pytest recognizes means it should shut down.
Are you suggesting I make this conditional on KeyboardInterrupt
specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I think this is only called for "interactive" exceptions. This is what the docs say.
Called when an exception was raised which can potentially be interactively handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall pytest itself having an issue with terminating on ctrl+c. Rather, I had just assumed that the issue was that we were consuming the KeyboardInterrupt
ourselves. We had at least one place that we just claimed a signal handler without any cooperative considerations when starting a service. That was changed recently in #10163 with a Windows regression addressed in #10263. Maybe there's something with pytest-asyncio related to this so I'll look there to get a bit more confidence I understand what is going on.
I saw that phrase you quoted about interactively handleable errors. I have no idea what it means. I'll ask around and see what I can figure out.
4a7be3d
to
2d5b93d
Compare
a65d9fd
to
c7b6b3d
Compare
This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
No description provided.