Skip to content

[change] Update tested versions #332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
May 29, 2025
Merged

[change] Update tested versions #332

merged 9 commits into from
May 29, 2025

Conversation

adamchainz
Copy link
Contributor

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

n/a

Description of Changes

Make these version changes:

  1. Add Python 3.13.
  2. Add Django 5.2 (currently in beta, expected April).

Also:

  1. Declare the the Django version dependency in setup.py.
  2. Update openwisp-utils to install from GitHub’s tarball URLs. The lack of suffix caused issues for me with uv via tox-uv.
  3. Drop parallel testing - it was slower for me, 2s versus 1s.

Screenshot

n/a

@adamchainz
[change] Update tested versions
072b644 
Make these version changes:

1. Add Python 3.13.
2. Add Django 5.2 (currently in beta, [expected April](https://docs.djangoproject.com/en/5.2/releases/5.2/)).

Also:

1. Declare the the Django version dependency in `setup.py`.
2. Update openwisp-utils to install from GitHub’s tarball URLs. The lack of suffix caused issues for me with [uv](https://docs.astral.sh/uv/) via [tox-uv](https://pypi.org/project/tox-uv/).
3. Drop parallel testing - it was slower for me, 2s versus 1s.
@adamchainz adamchainz mentioned this pull request Mar 12, 2025
Closed
@adamchainz
Copy link
Contributor Author

Sigh... the install step is broken on Python 3.13 because coveralls pins to Python <3.13 and hasn't removed that yet. Pending PR: TheKevJames/coveralls-python#542 .

I didn't see this locally because I use tox with uv, via tox-uv. They deliberately ignore upper version bounds on Python: astral-sh/uv#4022 . Would you be interested in converting the testing to uv? It wouldn't take much more than installing tox-uv.

pauloxnet
pauloxnet reviewed Mar 13, 2025
View reviewed changes
Copy link
Contributor

@pauloxnet pauloxnet left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

It seems CI action fails because coveralls doesn't support Python 3.13 yet and doesn't seem well maintained.

It can be a good idea to drop it, enforcing 100% test coverage using coverage report --fail-under=xx, like in this other PR https://github.com/kipe/pycron/pull/41/files

tox.ini Outdated
django42: Django~=4.2.0
django50: Django~=5.0.0
django51: Django~=5.1.0
django42: Django~=4.2a1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested in the motivation to use alpha version of Django here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ~ means “compatible with”. It still picks the latest version, just from alpha onwards rather than the GA release onwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I currently use ~= but usually starting with the first stable version (e.g. django~=4.2.0). I was curious about the motivation to include also alpha version (e.g. django~=4.2a1). What's the benefit?

@adamchainz
Copy link
Contributor Author

It can be a good idea to drop it, enforcing 100% test coverage using coverage report --fail-under=xx, like in this other PR https://github.com/kipe/pycron/pull/41/files

Yes, will do

devkapilbansal
devkapilbansal requested changes Mar 22, 2025
View reviewed changes
Copy link
Member

@devkapilbansal devkapilbansal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @adamchainz PR title should be descriptive enough to know the changes done. Also, reference the issue in PR description and commit message. Do follow these guidelines:
https://openwisp.io/docs/dev/developer/contributing.html

requirements-test.txt Outdated
@@ -2,4 +2,4 @@ psycopg2~=2.9.10
django-filter>=2.0
contexttimer
# QA checks
openwisp-utils[qa] @ https://github.com/openwisp/openwisp-utils/tarball/1.2
openwisp-utils[qa] @ git+https://github.com/openwisp/openwisp-utils@1.1.1
Copy link
Member

Choose a reason for hiding this comment

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

Why this change is made? We shouldn't pin the version in test dependencies

tox.ini
@@ -14,12 +15,13 @@ setenv =
DRFG_TEST_RUNNER=./tests/manage.py test
pytest: DRFG_TEST_RUNNER=-m pytest
commands =
coverage run {env:DRFG_TEST_RUNNER} {posargs:tests/django_restframework_gis_tests} --parallel
coverage run {env:DRFG_TEST_RUNNER} {posargs:tests/django_restframework_gis_tests}
Copy link
Member

Choose a reason for hiding this comment

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

Why this parallel flag is removed? Kindly follow the guidelines
https://openwisp.io/docs/dev/developer/contributing.html#avoiding-unnecessary-changes

@lairdresearch
Copy link

lairdresearch commented May 4, 2025 •
edited
Loading

Is there anything blocking adding the 3.16 series of DRF as well? It's pinned to drf < 3.16. Also the current django-filter==25.1 fails as well.

nemesifier added 3 commits May 29, 2025 12:04
@nemesifier
Merge branch 'update_tested_versions' of github.com:adamchainz/django…
6ab5a99 
…-rest-framework-gis into adamchainz-update_tested_versions

# Conflicts:
#	.github/workflows/ci.yml
#	setup.py
#	tox.ini
@nemesifier
[ci] Run on Ubuntu 20.04
37fa36c
@nemesifier
[chores] Updated CI build URL
8431099
@nemesifier nemesifier self-assigned this May 29, 2025
@nemesifier nemesifier added the dependencies Pull requests that update a dependency file label May 29, 2025
nemesifier
nemesifier previously approved these changes May 29, 2025
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks to @adamchainz @pauloxnet @devkapilbansal @lairdresearch for contributing with code or review and discussions!

I updated the remaining bits, it should be good now, I am going to merge, if you see anything wrong please open an issue.

@adamchainz adamchainz dismissed nemesifier’s stale review May 29, 2025 16:07

The merge-base changed after approval.

@nemesifier nemesifier merged commit 66e7bcc into openwisp:master May 29, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from To do (general) to Done in OpenWISP Contributor's Board May 29, 2025
@adamchainz adamchainz deleted the update_tested_versions branch May 30, 2025 12:25
@adamchainz
Copy link
Contributor Author

Thank you, cheers! I had the tab open but hadn't found the time to return and fix things :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pauloxnet pauloxnet pauloxnet left review comments

@devkapilbansal devkapilbansal devkapilbansal requested changes

@nemesifier nemesifier nemesifier left review comments

Assignees

@nemesifier nemesifier

Labels
dependencies Pull requests that update a dependency file
Projects
OpenWISP Contributor's Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adamchainz @lairdresearch @pauloxnet @nemesifier @devkapilbansal @ulgens