Skip to content
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.

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

fix: Report fails to render with pytest-xdist (1) #591

Closed

Conversation

BeyondEvil
Copy link
Contributor

@BeyondEvil BeyondEvil commented Mar 10, 2023

The reason why the report fails to render when running with pytest-xdist, is that it adds a node attribute with a WorkerController object as its value.

This object is unserializable, so it fails when trying to convert the test data to JSON.

This is proposed solution 1.

Solution 2: #592
Solution 3: #593

There's also the option of making the WorkerController class serializable upstream. Ping @nicoddemus

@BeyondEvil BeyondEvil changed the title fix: Report fails to render with pytest-xdist fix: Report fails to render with pytest-xdist (1) Mar 10, 2023
@nicoddemus
Copy link
Member

There's also the option of making the WorkerController class serializable upstream. Ping @nicoddemus

IMHO the WorkerController is an implementation detail and of little to no value to users, so I don't see an advantage in serializing it. 👍

@BeyondEvil BeyondEvil removed the request for review from ssbarnea March 10, 2023 15:08
@BeyondEvil
Copy link
Contributor Author

There's also the option of making the WorkerController class serializable upstream. Ping @nicoddemus

IMHO the WorkerController is an implementation detail and of little to no value to users, so I don't see an advantage in serializing it. 👍

By extension then, I guess there's no point in passing it in the JSON to the report either? (invalidating solution 3 #593 )

@BeyondEvil BeyondEvil requested a review from ssbarnea March 10, 2023 15:10
@nicoddemus
Copy link
Member

Possibly, but more investigation would be needed.

@BeyondEvil
Copy link
Contributor Author

Possibly, but more investigation would be needed.

I think it's safe to keep it out of the report.

It wasn't part of the legacy report afaict. And if someone has a use case for having it, we can re-add it easily.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

this is lossy in the face of valid extensions, os i recommend against it

@BeyondEvil
Copy link
Contributor Author

Fixed by: #598

@BeyondEvil BeyondEvil closed this Mar 19, 2023
@BeyondEvil BeyondEvil deleted the beyondevil/fix-xdist-1 branch March 19, 2023 17:58
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.

3 participants