Warning subclass which break subclassing protocol break xdist

[BrandonHoffman]

The latest xdist change causes issues whenever a subclass of the Warning class which does not set the args property due to this line: nicoddemus@eb81450#diff-cb98e0a92df3b797a1570a6e1a49b8d6R429

see the issue I created for an example: briancurtin/deprecation#36

I think it would be a good to add some error handling for cases like this in xdist so that they do not cause pytest to fail with an internal error.

[nicoddemus]

Hi @BrandonHoffman,

Not sure what pytest-xdist can do in this case, as the Warning subclass is not following basic subclassing guarantees (keeping the same interface as the parent class).

@RonnyPfannschmidt what's your opinion?

[nicoddemus]

(@BrandonHoffman took the liberty of updating the title to reflect the underlying issue better)

[RonnyPfannschmidt]

xdist doesnt follow the reduce/restore protocols for serialization, so its a xdist bug for now

[BrandonHoffman]

I think it may be a good idea to just add a try accept around the class initialization so that it does not cause a pytest to halt abruptly with an internal error

try:
    message = cls(*data["message_args"])
except TypeError:
    message = Warning("Failed to pass through {module}.{class} with args {args}".format(module=data["message_module"], class=data["message_class_name"], args=data["message_args"]))

another option for the message would be to store message_str here:

nicoddemus@eb81450#diff-30240c5c51db0f0f5c8cb678115ed270R185

as

message_str = str(warning_message.message)

that way you can use that message in the exception case shown above

This would at least fix the issue short term but I think @RonnyPfannschmidt is right it would be nice if xdist followed the reduce/restore protocols as a more long term fix.

[nicoddemus]

@BrandonHoffman that might be a valid workaround, but it seems @RonnyPfannschmidt suggestion to implement the reduce protocol is the correct one, albeit seems a lot more work because the reduce protocol (IIUC) is more complex to support.

[BrandonHoffman]

@nicoddemus agreed, not sure what you two think about doing a 1.23.2 release with the patch I mentioned above, would be. This way the pytest failure is resolved, while the larger reduce/restore issue is being made?

[RonnyPfannschmidt]

also setstate/getstate, it basically requires a safe serialize/de-serialize just like we need it for reports as well - this is something that takes quite a while to get right

[nicoddemus]

@BrandonHoffman I'm fine with implementing a quick workaround like the one you mention and releasing 1.23.2. Would you like to open a PR with that? I'm afraid I'll be short on time on the next few days to work on that PR.

also setstate/getstate, it basically requires a safe serialize/de-serialize just like we need it for reports as well - this is something that takes quite a while to get right

My feelings exactly.

[BrandonHoffman]

yeah, I can make a patch, might have to be in a few latter in the day though.

[nicoddemus]

@BrandonHoffman 1.23.2 is out, thanks for bringing this up and providing the initial patch! 👍

[nicoddemus]

xdist doesnt follow the reduce/restore protocols for serialization

@RonnyPfannschmidt I was refreshing my memory about the reduce protocol, and as I remember, it is quite convoluted and error prone: https://docs.python.org/3/library/pickle.html#object.__reduce__

Should we:

1. Try to implement the full protocol?
2. "cheat" a little and use pickle/unpickle directly, so it takes care of implementing the correct protocol.
3. Some other approach?

What is your opinion?

I was taking a look at fixing #349 on the weekend, and due to how warnings can be produced as warnings.warn(msg, UserWarning) or warnings.warn(UserWarning(msg)) (what a mess), it seems we need to go forward with the full __reduce__ protocol as the code is getting quite unyielding.

[gsakkis]

Not sure if it's the same bug but I'm getting DumpError: can't serialize <class '_pytest.warning_types.RemovedInPytest4Warning'> with pytest-xdist>1.23.0; pytest-xdist==1.23.0 works fine (both with pytest==3.9.3)