Feat: Don't add signal handlers on Windows #3065
Conversation
harshil21
left a comment
There was a problem hiding this comment.
nice. hinrich reviewed pretty much everything, so only one comment from me-
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Thanks for the updates! Can you add tests for the missing coverage?
tests/test_application.py
Outdated
| if platform.system() == "Windows": | ||
|
|
||
| def signal_handler_test_windows(*args, **kwargs): | ||
| # this should be empty, but still called once. | ||
| assert args == [] | ||
|
|
||
| monkeypatch.setattr(loop, "add_signal_handler", signal_handler_test_windows) | ||
| # this should run through, and we expect the signals to be empty | ||
| app.job_queue.run_once(abort_app, 2) | ||
| # pass false so it is not default | ||
| app.run_webhook( | ||
| port=49152, webhook_url="example.com", close_loop=False, stop_signals=False | ||
| ) |
There was a problem hiding this comment.
sorry, but this still doesn't do it IMO. stop_signals=False doesn't make sense because we do in fact want to test the default case. Moreover, add_signal_handler is never called (as intended), so the assert in the mocked function doesn't test anything. Granted, it raises an AssertionError if a code change accidently passes a signal on windows (only without stop_signal=False though), but that's really hard to read from this test. Additionally, it feels unncessary to have the above tests do nothing on windows (which again is not obvious from the tests) and only this if-clause actually test something.
What speaks against implementing the test as suggested in https://github.com/python-telegram-bot/python-telegram-bot/pull/3065/files#r879864569 ?
There was a problem hiding this comment.
What speaks against implementing the test as suggested
me forgetting you suggested that.
In case we break, we also have timeout now I decided
Bibo-Joshi
left a comment
There was a problem hiding this comment.
LGTM :) You probably run the test on windows a few times to check if the pytesttimeout does the trick?
|
I did yup |
Checklist for PRs
.. versionadded:: version,.. versionchanged:: versionor.. deprecated:: versionto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst(optional)__all__s