Skip to content

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 17, 2020

Ref: blueyed#163

TODO:

  • rebase to fix commit message
  • check my stashes

@blueyed blueyed requested a review from bluetech January 19, 2020 07:21
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:

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

@blueyed blueyed changed the title WIP: fix _TracebackStyle typing: fix _TracebackStyle Jan 19, 2020
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

from _pytest._code import Source

_TracebackStyle = Literal["long", "short", "no", "native"]
_TracebackStyle = Literal["long", "short", "line", "no", "native"]
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?

@blueyed blueyed force-pushed the fix-_TracebackStyle branch from b74f387 to 09e9a01 Compare January 19, 2020 10:23
@blueyed blueyed merged commit 1a75a3c into pytest-dev:master Jan 19, 2020
@blueyed blueyed deleted the fix-_TracebackStyle branch January 19, 2020 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants