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

use pinpointed handling of non-serializable data #35

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented May 2, 2023

fixes #12

the caveat is that now we sometimes have magic json objects in the depth of a message to fix the details

this also exposed a bug in warning record message handling that was hidden by the broad str magic

@nicoddemus
Copy link
Member

@RonnyPfannschmidt can you please rebase on main? Thanks!

instead of str

this creates a difference in logged data
warning messages needed a change
instead of toplevel values, only the sub-object is affected
@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus done

@nicoddemus
Copy link
Member

LGTM, let's see what @The-Compiler has to say.

@The-Compiler
Copy link
Member

Sorry for the delay, just came back from Berlin for yet another pytest company training on Saturday 😅

Just dug into the git history of the project where this happened to me, and I basically accidentally serialized a set instead of a list. I suppose this behavior makes sense. I wonder if a TypeError might make even more sense, but I suppose it would be a bit in the way if people use record_property for other stuff?

Either way, this seems fine to me. Perhaps "handle unserializable objects with a pinpointed marker" in the changelog should be a bit more specific so people know what to watch out for?

@RonnyPfannschmidt
Copy link
Member Author

Perhaps a little bikeshedding can help

It might also be a good idea to switch from str to safe_repr

@The-Compiler
Copy link
Member

Some questions to ponder on:

  • Why are we doing this instead of raising a TypeError?

Probably so someone with previous usage of record_property can start using the plugin without things exploding?

  • Do we foresee people using the actual values, or is this basically just included as debugging output?

I'd say debugging output?

  • If the latter, would it suffice if we just included the keys instead, and leave the values out entirely? After all, they might be rather noisy, if e.g. containing Python memory addresses which change every time.

IMHO it would be fine? But I can see there is value in seeing the value (hah) too.

@RonnyPfannschmidt
Copy link
Member Author

If we switch from str to length limited safe repr, I think we will be fine

@nicoddemus
Copy link
Member

nicoddemus commented May 10, 2023

Probably so someone with previous usage of record_property can start using the plugin without things exploding?

It was introduced here:

bac07c3

I don't recall the details, but probably xdist was including JSON-incompatible data into the reports. Problem is described here: #5

@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus xdist adds report.node but doesnt expand report serialization to turn it into json compatible objects atm

@nicoddemus
Copy link
Member

Yes (you probably did not see my edit where I found the reason after the initial post hehehe)

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.

cleanup_unserializable results in weird handling of unserializable user_properties
3 participants