Skip to content

typing: fix _TracebackStyle #6482

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 19, 2020
Merged

typing: fix _TracebackStyle #6482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/_pytest/_code/code.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@

from _pytest._code import Source

_TracebackStyle = Literal["long", "short", "no", "native"]
_TracebackStyle = Literal["long", "short", "line", "no", "native"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"native" in not used internally with FormattedExcinfo at least (https://github.com/blueyed/pytest/blob/f1609d6e956bdf4d712610d8e6ca9fbb0b2ed108/src/_pytest/_code/code.py#L612-L631), so it might make sense to have two different types here (but it makes sense to match the --tb option here I guess (currently (with this patch) missing "auto")) - but that would be a followup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should line be added to ExceptionInfo.getrepr() docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it is used there - i.e. I am not sure.
I think it is only used here:

pytest/src/_pytest/terminal.py

Line 868 in 719e0e3

if self.config.option.tbstyle == "line":



class Code:
Expand Down
9 changes: 7 additions & 2 deletions src/_pytest/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
from .reports import CollectReport
from .reports import TestReport
from _pytest._code.code import ExceptionInfo
from _pytest._code.code import ExceptionRepr
from _pytest.compat import TYPE_CHECKING
from _pytest.nodes import Collector
from _pytest.nodes import Node
from _pytest.outcomes import Exit
from _pytest.outcomes import Skipped
Expand Down Expand Up @@ -251,7 +253,7 @@ def pytest_runtest_makereport(item, call):
return TestReport.from_item_and_call(item, call)


def pytest_make_collect_report(collector) -> CollectReport:
def pytest_make_collect_report(collector: Collector) -> CollectReport:
call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
longrepr = None
if not call.excinfo:
Expand All @@ -264,7 +266,10 @@ def pytest_make_collect_report(collector) -> CollectReport:
skip_exceptions.append(unittest.SkipTest) # type: ignore
if call.excinfo.errisinstance(tuple(skip_exceptions)):
outcome = "skipped"
r = collector._repr_failure_py(call.excinfo, "line").reprcrash
r_ = collector._repr_failure_py(call.excinfo, "line")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but looking at the type of _repr_failure_py, I wonder:

  1. Why do _repr_failure_py and repr_failure both exist? repr_failure just forwards directly _repr_failure_py so why not just inline it?
  2. Is it correct that it can only take Failed and FixtureLookupError and not other exception types?
  3. Is it possible to add overloads to it so this assert is not needed?
  4. Should its style argument be typed with _TracebackStyle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. it is used by other subclasses etc - it is rather tangled (I have some WIP to fix this, also for 4.)
  2. IIRC we/I've limited this intentionally for what has been observed, but from the code it is clear that this is not correct:

    pytest/src/_pytest/nodes.py

    Lines 288 to 292 in 719e0e3

    if isinstance(excinfo.value, Failed):
    if not excinfo.value.pytrace:
    return str(excinfo.value)
    if isinstance(excinfo.value, FixtureLookupError):
    return excinfo.value.formatrepr()
  3. overload might be a good idea, but in general it would be better to not return a string there. Also it is based on excinfo.value.pytrace - so would need separate types for this I assume.
  4. yes, I have that in the WIP to clean up handling of --fulltrace

assert isinstance(r_, ExceptionRepr), r_
r = r_.reprcrash
assert r
longrepr = (str(r.path), r.lineno, r.message)
else:
outcome = "failed"
Expand Down