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

Enable testing with Python 3.11 #9511

Merged
merged 10 commits into from
Feb 11, 2022
Merged

Enable testing with Python 3.11 #9511

merged 10 commits into from
Feb 11, 2022

Conversation

nicoddemus
Copy link
Member

We should add Python 3.11 to our testing matrix in order to antecipate possible problems (even if we don't attempt to solve any of them right away).

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM

It seems like it's failing now - do you intend to fix the problems before merging, or merge just without marking the py311 checks as required?

BTW, this post is relevant: https://sethmlarson.dev/blog/python-prereleases-and-pip-cache though not sure if it affects our setup.

@nicoddemus
Copy link
Member Author

do you intend to fix the problems before merging, or merge just without marking the py311 checks as required?

I'm not sure I will have time to tackle this soon, but I think it would still be valuable to merge this regardless, as at least this problems will be visible in general. But let's see what others think.

@asottile
Copy link
Member

committed fixes to your branch -- hope you don't mind <3

@@ -455,7 +455,10 @@ def test_idmaker_enum(self) -> None:
enum = pytest.importorskip("enum")
e = enum.Enum("Foo", "one, two")
result = idmaker(("a", "b"), [pytest.param(e.one, e.two)])
assert result == ["Foo.one-Foo.two"]
if sys.version_info >= (3, 11):
assert result == ["one-two"]
Copy link
Member

Choose a reason for hiding this comment

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

Looks like they went ahead with the Enum str change, which means the nodeids for tests parametrized with enums changes. I think the new ID is better, but we should probably keep the old format for backward compat. But we should make it consistent between versions either way.

Copy link
Member

Choose a reason for hiding this comment

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

eh, I kinda disagree -- we shouldn't deviate from python's behaviour unless there's a very good reason to do so (as it subverts expectations and creates a "surprise")

Copy link
Member

Choose a reason for hiding this comment

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

You mean that you think the node ID should be allowed to differ between versions? Many people run pytest on several versions, and for things like -k, reporting, plugins, etc I think it's expected that the node IDs would be the same.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think they should differ between versions

Copy link
Member

Choose a reason for hiding this comment

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

structurally this will lead to false test inequality, which makes test result aggregation a total mess ^^
maybe we can test this with better test ids tho

Copy link
Member Author

Choose a reason for hiding this comment

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

While I understand @bluetech's reasoning, I'm leaning towards Anthony here, because I think it is reasonable for pytest to use Enum's str representation in the node id and "wash his hands".

If in the end the user impact is minimal and justified in cases like this, I think it is best to lean towards our own maintenance sanity. I can envision for example, if we were to know have special handling for enums, for that to change again in a future Python release and break this again, but now it would be "our fault" because we took responsibility for that feature. Also users often pass their own objects to parametrize and their str representation is used, which could differ between versions as well.

@hugovk
Copy link
Member

hugovk commented Feb 11, 2022

BTW, this post is relevant: sethmlarson.dev/blog/python-prereleases-and-pip-cache though not sure if it affects our setup.

My PR actions/setup-python#303 has been merged and released which fixed that too: a fresh cache is created for new patch/alpha/beta/RC versions (actions/setup-python#319 (comment)).

@nicoddemus
Copy link
Member Author

committed fixes to your branch -- hope you don't mind <3

Of course not, thanks a bunch. 👍

I will rebase this now and see how it fares.

nicoddemus and others added 6 commits February 11, 2022 09:54
We should add Python 3.11 to our testing matrix in order to antecipate possible problems (even if we don't attempt to solve any of them right away).
RuntimeWarning: Running interpreter doesn't sufficiently support code object introspection.  Some features like bare super() or accessing __class__ will not work with slotted classes.
@nicoddemus
Copy link
Member Author

Weird, I thought we had sorted this out:

https://github.com/pytest-dev/pytest/runs/5156307323?check_suite_focus=true

    def get_resource_reader(self, name: str) -> importlib.abc.TraversableResources:  # type: ignore
        if sys.version_info < (3, 11):
            from importlib.readers import FileReader
        else:
>           from importlib.resources.readers import FileReader
E           ModuleNotFoundError: No module named 'importlib.resources.readers'; 'importlib.resources' is not a package

@asottile
Copy link
Member

Weird, I thought we had sorted this out:

https://github.com/pytest-dev/pytest/runs/5156307323?check_suite_focus=true

    def get_resource_reader(self, name: str) -> importlib.abc.TraversableResources:  # type: ignore
        if sys.version_info < (3, 11):
            from importlib.readers import FileReader
        else:
>           from importlib.resources.readers import FileReader
E           ModuleNotFoundError: No module named 'importlib.resources.readers'; 'importlib.resources' is not a package

huh weird -- does github actions not have alpha 4 ?

@hugovk
Copy link
Member

hugovk commented Feb 11, 2022

Yep, GitHub has the newest alpha 5 as well.

If you use 3.11-dev instead of pinning it will use the latest alpha/beta/RC available.

@nicoddemus
Copy link
Member Author

nicoddemus commented Feb 11, 2022

Using 3.11-dev worked:

platform linux -- Python 3.11.0a5, pytest-7.1.0.dev220+g64f943958, pluggy-1.0.0

But we now get two failures:

https://github.com/pytest-dev/pytest/runs/5157307611?check_suite_focus=true

    def test_idmaker_enum(self) -> None:
        enum = pytest.importorskip("enum")
        e = enum.Enum("Foo", "one, two")
        result = IdMaker(
            ("a", "b"), [pytest.param(e.one, e.two)], None, None, None, None
        ).make_unique_parameterset_ids()
        if sys.version_info >= (3, 11):
>           assert result == ["one-two"]
E           AssertionError: assert ['Foo.one-Foo.two'] == ['one-two']
E             At index 0 diff: 'Foo.one-Foo.two' != 'one-two'
E             Use -v to get the full diff
    def test_run_result_repr() -> None:
        outlines = ["some", "normal", "output"]
        errlines = ["some", "nasty", "errors", "happened"]
    
        # known exit code
        r = pytester_mod.RunResult(1, outlines, errlines, duration=0.5)
        if sys.version_info >= (3, 11):
>           assert (
                repr(r) == "<RunResult ret=TESTS_FAILED len(stdout.lines)=3"
                " len(stderr.lines)=4 duration=0.50s>"
            )
E           AssertionError: assert '<RunResult r...ration=0.50s>' == '<RunResult r...ration=0.50s>'
E             - <RunResult ret=TESTS_FAILED len(stdout.lines)=3 len(stderr.lines)=4 duration=0.50s>
E             ?                ^^^^^^^^^^^^
E             + <RunResult ret=1 len(stdout.lines)=3 len(stderr.lines)=4 duration=0.50s>
E             ?     

@asottile
Copy link
Member

heh looks like it got reverted again -- fortunately you can rebase out just the one commit I had python/cpython@42a64c0

@nicoddemus nicoddemus enabled auto-merge (squash) February 11, 2022 15:07
@nicoddemus nicoddemus merged commit b79eff0 into pytest-dev:main Feb 11, 2022
@nicoddemus nicoddemus deleted the py-311 branch February 11, 2022 15:20
@nicoddemus
Copy link
Member Author

Thanks everyone!

nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Feb 11, 2022
nicoddemus added a commit that referenced this pull request Feb 11, 2022
[7.0.x] Enable testing with Python 3.11 (#9511)
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.

5 participants