python-hyper / wsproto

Sans-IO WebSocket protocol implementation
https://wsproto.readthedocs.io/
MIT License
261 stars 38 forks source link

Python 3.10: test_successful_handshake fails #159

Closed musicinmybrain closed 3 years ago

musicinmybrain commented 3 years ago

With Python 3.10.0 beta 2 on Fedora Linux Rawhide (development version):

=================================== FAILURES ===================================
__________________________ test_successful_handshake ___________________________
    def test_successful_handshake() -> None:
        client = H11Handshake(CLIENT)
        server = H11Handshake(SERVER)

        server.receive_data(client.send(Request(host="localhost", target="/")))
        assert isinstance(next(server.events()), Request)

        client.receive_data(server.send(AcceptConnection()))
        assert isinstance(next(client.events()), AcceptConnection)

        assert client.state is ConnectionState.OPEN
        assert server.state is ConnectionState.OPEN

>       assert repr(client) == "H11Handshake(client=True, state=ConnectionState.OPEN)"
E       AssertionError: assert 'H11Handshake..., state=OPEN)' == 'H11Handshake...onState.OPEN)'
E         - H11Handshake(client=True, state=ConnectionState.OPEN)
E         ?                                 ----------------
E         + H11Handshake(client=True, state=OPEN)
test/test_handshake.py:22: AssertionError

As Tomáš Hrnčiar noted in the downstream issue:

There were some changes in repr() in latest alpha of Python 3.10. https://docs.python.org/3.10/whatsnew/changelog.html#python-3-10-0-alpha-7

bpo-40066: Enum: adjust repr() to show only enum and member name (not value, nor angle brackets) and str() to show only member name. Update and improve documentation to match.

bpo-40066: Enum’s repr() and str() have changed: repr() is now EnumClass.MemberName and str() is MemberName. Additionally, stdlib Enum’s whose contents are available as module attributes, such as RegexFlag.IGNORECASE, have their repr() as module.name, e.g. re.IGNORECASE. https://bugs.python.org/issue40066

It seems like the most obvious thing would be to loosen the test to accept either representation (based on a Python version test if you are feeling really picky).

I suppose you could customize wsproto.connection.ConnectionState.__str__() instead, or use a different format string in H11Handshake.__repr__(), if you really wanted a consistent representation across Python versions.

What do you think?

Kriechi commented 3 years ago

ref https://github.com/python-hyper/h2/issues/1253

I was hoping for this breaking change to be reverted - but it looks like this might actually make it into the final Python 3.10 release... 😞 This is breaking a lot of packages.

Since wsproto (and the other projects in the python-hyper organization) are mostly libraries as well, we should aim for a consistent format which is independent on the Python version. Otherwise our downstream who depends on exact repr output might suffer from the same problem again. WDYT?

I'm happy to review and merge PRs for this - although I'm still hoping for an upstream fix (revert?).

musicinmybrain commented 3 years ago

Looking at it again, if you wanted to enforce a consistent format across the board, my first impulse would be to write a little class in wsproto.utilities implementing __str__() and __repr__() the way that Enum did in Python 3.9 and earlier, and apply it as a mixin to all the classes in wsproto that are derived from Enum or IntEnum.

Of course, that won’t affect any enums that come from h11 or the standard library. I’m not sure whether any “external” enums are formatted in wsproto.

Is that the kind of thing you were thinking of?

Kriechi commented 3 years ago

Yes, that was my thinking - but you correctly pointed out that we also rely on enums and their repr from our dependencies. And your original test failure report even contains one!

We need to do some research if there is already some kind of consensus in the Python community how to handle this breakage with the upcoming 3.10. Looking at PEP619 we have until October to figure it out.

Maybe it's not worth aiming for such strong consistency? Change my mind!

musicinmybrain commented 3 years ago

Maybe it's not worth aiming for such strong consistency? Change my mind!

My best argument for not overriding the format is that by doing nothing (except adapting the tests), you would at least maintain consistency with everything else that uses enums in the Python ecosystem. There would be one-time pain for those relying on exact representations—but those doing so would surely encounter the change via other dependencies too, and in the long term following the standard library’s format would maintain the principle of least astonishment.

But in the end, I don’t have a strong personal opinion! Maybe someone else will drop in and present one.

musicinmybrain commented 3 years ago

Looking at PEP619 we have until October to figure it out.

I agree that there is still time to decide, but as the maintainer of the python-fastapi package in Fedora Linux, which has (python-)wsproto in its dependency chain, I hope you’ll choose an approach a bit earlier than October. We just finished rebuilding everything against Python 3.10 in Rawhide, the development version of Fedora. See https://fedoraproject.org/wiki/Changes/Python3.10 for the schedule we’re working with and https://bugzilla.redhat.com/show_bug.cgi?id=1949430 for the downstream version of this issue. I’m not the maintainer of python-wsproto in Fedora; they might have something to say too.

Kriechi commented 3 years ago

I understand your pain - and of course I'm sympathetic to finding a solution!

At the same time I need to stress that wsproto does not support Python 3.10 (or any pre-release of it) as of today - which is clearly documented in our setup.py and the lack of CI for it. I think this applies to all python-hyper project as of now. So in due course we will keep the projects updated with the newer Python versions, but I can't promise this will happen before or to the launch date, and it most likely will not support beta or RC versions.

Let's see if the community converges on a suitable solution, or somebody comes forward with strong enough opinions in either direction.

carlwgeorge commented 3 years ago

Fedora python-wsproto maintainer here. I'm in favor of whatever gets the tests passing on Fedora Rawhide the fastest. I'm happy to apply a WIP pull request as a patch to the Fedora package.

encukou commented 3 years ago

With my CPython dev hat on: __repr__ output, error messages and the like are gnerally not stable, and you can always expect it to change between versions. Should that be mentioned more clearly in Python's docs? For Enum specifically, use name and/or value if you need something stable. Or is there a special reason to use __repr__ here?

With my Fedora maintainer hat on: Note that Rawhide is the pre-release version of Fedora.

musicinmybrain commented 3 years ago

I was hoping for this breaking change to be reverted

You got your wish! The CPython changes were reverted for Python 3.10.0b4 (full thread) and will not be present in Python 3.10.

That fixes this issue for now, so I’m closing this report. It does look like these changes are coming back in some form for Python 3.11, so hopefully it will be possible to track the forthcoming PEP and consider in advance how best to handle it.