Fix TestReport.longreprtext when TestReport.longrepr is not a string#7561
Fix TestReport.longreprtext when TestReport.longrepr is not a string#7561nicoddemus merged 1 commit intopytest-dev:masterfrom
Conversation
| longrepr.toterminal(out) | ||
| else: | ||
| try: | ||
| out.line(longrepr) |
There was a problem hiding this comment.
@bluetech do you know why mypy didn't catch this problem? TerminalWriter.line expects a str argument, but longrepr is declared as Any in the class definition.
There was a problem hiding this comment.
a function which accepts str also accepts Any because it's gradually typed (there's options to turn this off in mypy, but getting to that ideal is very very difficult)
There was a problem hiding this comment.
Ahh I indeed:
A static type checker will treat every type as being compatible with Any and Any as being compatible with every type.
Thanks! My thought was that a function declaring str would accuse Any as incompatible, but the above definition clearly states otherwise. 👍
| longrepr.toterminal(out) | ||
| else: | ||
| try: | ||
| out.line(longrepr) |
There was a problem hiding this comment.
a function which accepts str also accepts Any because it's gradually typed (there's options to turn this off in mypy, but getting to that ideal is very very difficult)
pytest 6.0.0 has broken the job with issue [1], until it's fixed with PR [2] and released let's pin it. [1] pytest-dev/pytest#7559 [2] pytest-dev/pytest#7561 Change-Id: I073cfbcb4a65e716373f0a4df4766fd38a801618
bluetech
left a comment
bluetech
left a comment
There was a problem hiding this comment.
LGTM. I think CollectErrorRepr might need the same treatment, not sure.
The typing of longrepr is unfortunate...
|
Anyone with merge and release super powers? That changes deserves a fast track. I already opened pytest-dev/pytest-html#318 as I have a strong interest in keeping this plugin green and compatible with any new release of pytest. |
Every maintainer has that. 😁 We should have a release real soon, as there are a number of regressions we should take care. 👍 |
nicoddemus
force-pushed
the
longreprtext-7559
branch
from
c587089 to
d3267bc
Compare
Good call! It didn't require changes, but I added a test to avoid future regressions.
Indeed. We should eventually try to narrow down all possible values it might have, to eventually refactor that out somehow. |
nicoddemus
added
the
needs backport
[6.0.x] Merge pull request #7561 from nicoddemus/longreprtext-7559
nicoddemus
added
backported
and removed
needs backport
4 participants
Fix #7559