Fix Config.get_terminal_writer() crash when terminalreporter is unregistered#14378
Fix Config.get_terminal_writer() crash when terminalreporter is unregistered#14378antoineleclair wants to merge 1 commit intopytest-dev:mainfrom
Conversation
When a plugin such as pytest-tap in streaming mode unregisters the terminalreporter plugin, Config.get_terminal_writer() crashed with an internal AssertionError that masked the real test failures. Fall back to create_terminal_writer(self) — the same factory TerminalReporter uses internally — so assertion rewriting, show_test_item, and setuponly keep working. Fixes pytest-dev#14377. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bluetech
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I would rather avoid a solution like this, it seems like something that might cause unexpected issues. It's also a bit wasteful to create a fresh instance on each assert.
I would rather look at removing the dependency on TerminalWriter from assertrepr_compare.
Seems to me that ideally assertrepr_compare would return some structured format that is then "rendered" externally. But the list[str] is pretty ingrained for now (there's a pytest_assertrepr_compare hook which returns it), so for now let's say this solution is out.
A question is whether highlighting should be performed when terminalreporter is disabled:
If yes, we should separate a "Highlighter" from TerminalWriter, and have both TerminalWriter and assertrepr_compare depend on it.
If no, then we should make assertrepr_compare not highlight when terminal reporter is disabled.
My thinking is "no", since a custom rendered probably doesn't want terminal escape codes. But maybe there are plugins which depend on it already, and plugins which don't want it just disable highlighting. So probably safer to go with "yes" solution.
Let me know if you want to try it, if not, I'll try looking into it myself.
| @@ -0,0 +1 @@ | |||
| Fixed :meth:`pytest.Config.get_terminal_writer` crashing with an internal ``AssertionError`` when the terminal reporter plugin has been unregistered (for example, by :pypi:`pytest-tap` in streaming mode). A fresh terminal writer is now created on demand. | |||
There was a problem hiding this comment.
The get_terminal_writer function is private, so you can't ref it. Also, let's remove the last sentence describing the solution since that's not relevant for a changelog.
| Fixed :meth:`pytest.Config.get_terminal_writer` crashing with an internal ``AssertionError`` when the terminal reporter plugin has been unregistered (for example, by :pypi:`pytest-tap` in streaming mode). A fresh terminal writer is now created on demand. | |
| Fixed a crash from `Config.get_terminal_writer` when the terminalreporter plugin has been unregistered (for example, by :pypi:`pytest-tap` in streaming mode). |
There was a problem hiding this comment.
Thanks @bluetech.
I would rather look at removing the dependency on TerminalWriter from assertrepr_compare.
I think you're right. I was proposing a solution, mostly to highlight the problem and in case the solution would be acceptable (after all... it's crashing without the change, and it's not crashing with the change).
Let me know if you want to try it, if not, I'll try looking into it myself.
I think you have more knowledge of the code base and will do a better job with less effort. So if you're OK, I'd leave that one to you. We can close the PR whenever you want.
Or I may try to tackle it in a few months if I see it's still not been touched (which would be totally understandable).
Thank you for your time.
Closes #14377.
The fix
Config.get_terminal_writer()had a hardassert terminalreporter is not Nonethat crashes when a plugin (e.g.pytest-tapin streaming mode) unregisters the terminal reporter. It now falls back tocreate_terminal_writer(self)— the same factoryTerminalReporter.__init__uses internally — so the happy path is unchanged and callers keep working when the reporter is gone.This fixes the reported assertion-diff crash, and also covers
runner.show_test_item(--collect-only -q) andsetuponly._show_fixture_action(--setup-only/--setup-plan), which hit the same assert in their respective modes.Does this change the behavior?
For the use case of
pytest-tap, I don't think it changes the behavior for the assertion path —assertrepr_compareonly uses_highlight, which is a pure string transformation; nothing gets written to stdout, so pytest-tap's TAP output is unaffected. In practice, it's a slight behavior change, so please let me know if you think this could have bad side effects, e.g. for other plugins.For
show_test_item/setuponlythe fresh writer would actually emit to stdout, but those paths used to crash, so this seems strictly better. Happy to revisit if you'd rather silence them in that case.Tests
test_config.py::TestConfigAPI::test_get_terminal_writer_without_terminalreporter— direct contract test onget_terminal_writer()with the plugin unregistered.test_assertion.py::test_assertrepr_compare_without_terminalreporter—pytesterintegration test reproducing the pytest-tap scenario end-to-end.Both fail without the fix and pass with it.
Checklist
closes #14377.Co-authored-bytrailer. I reviewed every line and own the change.changelog/14377.bugfix.rstadded.AUTHORSin alphabetical order.