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

Unpin pytest #3035

Closed
rominf opened this issue May 2, 2024 · 6 comments
Closed

Unpin pytest #3035

rominf opened this issue May 2, 2024 · 6 comments
Assignees
Labels
Triaged Has been looked at recently during old issue triage

Comments

@rominf
Copy link
Contributor

rominf commented May 2, 2024

Problem Statement

As a Fedora packager I would like to have pytest to not have pin < 7.0.0, so that python-sentry-sdk could be tested in Fedora without workarounds. Also, pytest==7.0.0 was released on Feb 4, 2022.

All actual Fedora releases contain pytest>7.0.0 and installing an older version from PyPI during package build for testing is not possible. Thus, I unpinned it. Since some tests fail with pytest>7.0.0 because of the issue in pytest-forked, I had to find a way how to fix them. Luckily, thanks to this investigation there is a workaround: reorder the tests so that non-forked tests are not last in the file, see patch.

Solution Brainstorm

Of course, the best solution would be to fix the issue in pytest-forked, however it looks like a hard problem.

My suggestions:

  • Unpin pytest.
  • Reorder the tests.
  • Write a note in CONTRIBUTING.md about this quirk. Another option is to add a linter that would inform about the need to reorder tests.
@szokeasaurusrex
Copy link
Member

@rominf Thank you for opening this issue, and for linking the investigation! We will place this issue on our internal backlog. However, you are also welcome to open a PR, if you would like – we might be able to implement your suggested changes more quickly that way.

@getsantry getsantry bot removed the status in GitHub Issues with 👀 2 May 3, 2024
rominf added a commit to rominf/sentry-python that referenced this issue Oct 2, 2024
Fix: getsentry#3035

Rearrange test items so that forked tests come before normal tests
within their respective modules.
Swap the last forked test with the last normal test if necessary.

Workaround to unpin pytest. See:
pytest-dev/pytest#9621,
pytest-dev/pytest-forked#67, and specifically:
pytest-dev/pytest-forked#67 (comment)
rominf added a commit to rominf/sentry-python that referenced this issue Oct 2, 2024
Fix: getsentry#3035

Rearrange test items so that forked tests come before normal tests
within their respective modules.
Swap the last forked test with the last normal test if necessary.

Workaround to unpin pytest. See:
pytest-dev/pytest#9621,
pytest-dev/pytest-forked#67, and specifically:
pytest-dev/pytest-forked#67 (comment)
@szokeasaurusrex szokeasaurusrex added the Triaged Has been looked at recently during old issue triage label Oct 7, 2024
@anthonyryan1
Copy link

Gentoo is going to remove support for sentry-python on November 17th if we don't have a release with pytest 8 support before that date.

Reference: https://bugs.gentoo.org/937896

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 24, 2024
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
Remove Pytest pin. This requires ensuring that any test modules which contain both forked and non-forked tests do not have a forked test as the last test in the module. See [here](pytest-dev/pytest-forked#67 (comment)).

Closes #3035
@szokeasaurusrex
Copy link
Member

@anthonyryan1 thanks for the heads up, we will try to see whether we can get rid of the pin.

@szokeasaurusrex szokeasaurusrex self-assigned this Oct 28, 2024
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
Remove Pytest pin. This requires ensuring that any test modules which contain both forked and non-forked tests do not have a forked test as the last test in the module. See [here](pytest-dev/pytest-forked#67 (comment)).

Closes #3035
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
Remove Pytest pin. This requires ensuring that any test modules which contain both forked and non-forked tests do not have a forked test as the last test in the module. See [here](pytest-dev/pytest-forked#67 (comment)).

Closes #3035
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
Remove Pytest pin. This requires ensuring that any test modules which contain both forked and non-forked tests do not have a forked test as the last test in the module. See [here](pytest-dev/pytest-forked#67 (comment)).

Closes #3035
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
The pytest pin in requirements-devenv.txt appears to be unnecessary. Our tests anyways do not seem to respect this pin; the actual pins are defined for each environment in tox.ini.

ref #3035
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
The pin appears to be unnecessary in Python 3.8+.

ref #3035
@szokeasaurusrex
Copy link
Member

@anthonyryan1, looks like the pin might not be needed on Python 3.8+. Would Gentoo be able to continue shipping the Python SDK if we remove the pin for Python 3.8+, but keep it around for the older Python versions we still support (3.6 and 3.7)?

szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
This pin appears to be unnecessary on Python 3.8+.

ref #3035
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
The pin appears to be unnecessary in Python 3.8+.

ref #3035
szokeasaurusrex added a commit that referenced this issue Oct 28, 2024
Unpin pytest for Celery tests. This requires adding a placeholder test to workaround a bug with pytest-forked.

ref #3035
@anthonyryan1
Copy link

Gentoo appears to already be supporting a minimum of Python 3.11 for this package, so there's no change to minimum python version it can be used together with.

@szokeasaurusrex
Copy link
Member

szokeasaurusrex commented Oct 29, 2024

The pytest pins across most of our test suite have now been removed. This work was implemented in the following PRs:

Some pins, however, still remain, because it was not possible to remove these pins with trivial changes to the test code:

  • py{3.6,3.7}-{common,gevent} still pin pytest<7.0.0. This might be okay, as these are older Python versions.
  • Our Redis tests pin pytest<8.0.0. We should investigate why our Redis tests fail with Pytest ≥8.0.0 and implement a fix, if possible.
  • Our tornado-v{6.0,6.2} tests pin pytest<8.2. This because these versions of tornado are incompatible with Pytest ≥8.2; tornado v6.4.1 fixes the problem. Our tornado-latest tests therefore do not pin pytest.

This work is likely sufficient to close this issue. I will open a new issue to address the Redis pin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triaged Has been looked at recently during old issue triage
Projects
Archived in project
Archived in project
3 participants