added exception object to the TestReport

[brianmaissy]

Fixes #3343

[coveralls]

Coverage increased (+0.04%) to 92.856% when pulling e861ccb on brianmaissy:feature/add_exception_to_testreport into 2962c73 on pytest-dev:features.

[RonnyPfannschmidt]

this completely breaks with xdist as reports are network transferrable and exception objects not

it should be noted that the error section of the report should contain an accesible exception object (which gets turned into text by the xdist transfer)

[nicoddemus]

Thanks @brianmaissy,

As @RonnyPfannschmidt commented this breaks xdist, unfortunately. pytest-xdist has its own serialization/unserialization code, which was a source of bugs in the past and makes it harder to implement the kind of change in this PR because we need to synchronize both xdist and pytest releases.

Perhaps as a starting point @brianmaissy could contribute a patch moving serialization/unserialization code from pytest-xdist into pytest? This would make it easier to implement this kind of feature.

We might even consider turning TestReport objects into attrs instances, and use attrs serialization instead?

[RonnyPfannschmidt]

the topic is a bid convulve,d since we still have all the exception repressentation objects around that a) need porting, and b) leak to the externals as part of the serialization in a way i'd like to avoid

unfortunately i lack the time to clean up the exception representation in a timely manner

[nicoddemus]

I see where you might be going, but some clarifications please:

lve,d since we still have all the exception repressentation objects around that need porting

You mean the Repr objects, like ReprExceptionInfo, and also TestReport I assume?

b) leak to the externals as part of the serialization in a way i'd like to avoid

What do you mean?

[RonnyPfannschmidt]

@nicoddemus if we make exceptions part of test report serialization in a structured way - we create a exterannly visible api, so if we leak it that way, we create a point where we either have to break people or keep something deprecated

i dont meant the ReprExceptionInfo, but the ExceptionInfo itself

[nicoddemus]

@RonnyPfannschmidt your two comments below conflict directly with each other, so I'm still a little confused:

it should be noted that the error section of the report should contain an accesible exception object (which gets turned into text by the xdist transfer)

if we make exceptions part of test report serialization in a structured way - we create a exterannly visible api, so if we leak it that way, we create a point where we either have to break people or keep something deprecated

Having said that I agree that we can't make the exception info object part of the TestReport, at least not in its entirety given that exception info objects carry even a traceback object in recent Python versions (which are not serializable).

But I notice now that #3343 only needs access to the exception message, so perhaps we should add report.exc_message as suggested? We might even add report.exc_class_name as well, which might be helpful in the future.

EDIT: @RonnyPfannschmidt perhaps is that what you meant with your first comment?

[RonnyPfannschmidt]

@nicoddemus i just checked the code and i indeed did misremember, not an exception but an exception repr is stored

[nicoddemus]

i just checked the code and i indeed did misremember, not an exception but an exception repr is stored

Which code? This PR clearly stores exc_info.value...

And what's your opinion about adding report.exc_message and report.exc_class_name as suggested?

[RonnyPfannschmidt]

im against that , we already put an exception repressentation in, we ought to take a look at that

[nicoddemus]

im against that , we already put an exception repressentation in, we ought to take a look at that

Can you elaborate? #3343 comments that they are already looking into longreprtext, but it feels brittle to parse that.

[RonnyPfannschmidt]

@nicoddemus report.longrepr is a representation for the reported exception, and as far as i can tell its quite a mess - i'd rather see that cleaned up and more accessible that dropping random other attributes into reports simply because our objects for representing exceptions are to bad that we need to add extra ways to represent them, because that would just make 2 wrongs from one

[nicoddemus]

Hmm I see, I think you are right.

Taking a look at the code, I would say we should give ReprTraceback (which is report.longrepr more responsibility by receiving the excinfo object and taking care of extracting the representation itself. This way ReprTraceback can provide more fine-grained attributes like exc_message and exc_class_name for example. Also ReprTracebackNative should be thrown away in this refactoring (ReprTraceback should be able to produce native "repr"s from the excinfo).

[RonnyPfannschmidt]

i question the approach of the exceptionrepr objects in general, i'd like to approach it in a serialize/display manner and do away with the current abstraction alltogether

[nicoddemus]

i question the approach of the exceptionrepr objects in general, i'd like to approach it in a serialize/display manner and do away with the current abstraction alltogether

Not sure exactly what you mean, but seems like this could be done in another feature if/when someone has the time to do it. I don't see "repr" objects going away anytime soon. 😅

I would like for us to decide right now if we should close this issue or guide @brianmaissy in a practical and workable solution. I would prefer the latter, as aiming for the perfect solution usually takes a very long time.

[RonnyPfannschmidt]

the exception repr objects should gain the metadata on the exception, thanks for cutting down the tangent thats more appropriate for a follow-up

[nicoddemus]

Thanks @RonnyPfannschmidt.

@brianmaissy, I think we can continue from #3361 (comment), what do you think?

[brianmaissy]

sounds good

[brianmaissy]

Actually not sure I follow exactly what the proposal is. ReprTraceback is an object whose purpose is to serializable a representation of a traceback by calling toterminal? Or is ReprTraceback itself serializable and therefore we can add attributes to it like exc_message and have it do the (still somewhat brittle) string parsing?

[RonnyPfannschmidt]

@brianmaissy this whole thing is a mess i'd love to just remove and rip out but cant due dot time and backward compat restraints ^^ - but its the thing that repressents exceptions, and as such it should get the attributes

but it will loose those over network transfer since we coerce exceptions into strings as of now

[brianmaissy]

Ok. So we want to add exc_class_name and exc_message to the ReprTraceback object, even though they will be lost in serialization.

It should determine them by parsing the lines of the ReprEntry? Isn't that still just as brittle as the user parsing it himself?

And how will it get exposed in the TestReport at the end of the day? The longrepr is the already-serialized string version of the representation.

[nicoddemus]

Ok. So we want to add exc_class_name and exc_message to the ReprTraceback object, even though they will be lost in serialization.

They would not be lost, they are just str objects after all. I think the confusion here is that at some point it was discussed to add the exception object, which indeed would be lost during serialization.

It should determine them by parsing the lines of the ReprEntry?

Traceback receives an exc_info object, so you can take the exception class name and message from it:

def exc_class_name(self):
    return self._exc_info.type.__name__ if self._exc_info is not None else ''

def exc_message(self):
    return safe_str(self._exc_info.value) if self._exc_info is not None else ''

(Not sure why exc_info might be None)

And how will it get exposed in the TestReport at the end of the day? The longrepr is the already-serialized string version of the representation.

Oh I was under the impression that TestReport.longrepr was the TracebackRepr object, not the serialized version...

[RonnyPfannschmidt]

@nicoddemus they would no longer be available on a xdist master

[nicoddemus]

they would no longer be available on a xdist master

Who? The TestReport.longrepr object?

[RonnyPfannschmidt]

@nicoddemus xdist turns the longrepr into a string as well as each of the sections that support it, so while its availiable on a worker, its gone on the master

[nicoddemus]

argh I see, thanks.

[nicoddemus]

Well I'm not sure what to do here, it seems we need to redesign how traceback and error representation is done in pytest, which seems like a major refactoring. @RonnyPfannschmidt you seem to have an idea of the direction you would like this to go, want to start brainstorming this in a separate issue with me and others interested?

[RonnyPfannschmidt]

sounds good

[nicoddemus]

Closing so we can discuss in #3399