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

Upgrade Twisted to a version compatible with both Python 3.7 and 3.11 #2796

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

lunkwill42
Copy link
Member

Because Twisted 20 won't build for Python 3.11.

Closes #2792

Copy link

github-actions bot commented Jan 5, 2024

Test results

     12 files       12 suites   12m 2s ⏱️
3 308 tests 3 308 ✔️ 0 💤 0
9 399 runs  9 399 ✔️ 0 💤 0

Results for commit 68e0641.

♻️ This comment has been updated with latest results.

Copy link

sonarqubecloud bot commented Jan 5, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@lunkwill42
Copy link
Member Author

This change actually appears to break NAV, so I wouldn't approve of it :)

Several Twisted-relevant tests are hanging indefinitely (i.e. all the test suites have failed after timing out).

I've found that Twisted needs to have an upper bound version of 23.8.0 for the time being, as this is the last version of Twisted that supports Python 3.7. NAV still must support Python 3.7, but we hope to drop this during 2024.

@lunkwill42 lunkwill42 marked this pull request as draft February 28, 2024 08:52
@lunkwill42
Copy link
Member Author

lunkwill42 commented Feb 28, 2024

Actually, Twisted 23.8.0 is the last version to support Python 3.7, and officially the first to support 3.11, so we may need to lock this version explicitly, for now.

Because Twisted 20 doesn't support Python 3.11, while 23.8 is the first
that will - and also the last that will support Python 3.7, which we
need to remain compatible until later this year.
@lunkwill42 lunkwill42 changed the title Remove upper bound for Twisted Upgrade Twisted to a version compatible with both Python 3.7 and 3.11 Feb 28, 2024
Since we just upgraded Twisted, we should also upgrade our
pytest-twisted dependency to ensure tests are still working.
@lunkwill42
Copy link
Member Author

lunkwill42 commented Feb 29, 2024

ATM, it seems some specific ipdevinfo integration tests are hanging forever.

Specifically, it's the test defined in tests/integration/web/ipdevinfo_test.py::test_bad_name_should_not_crash_ipdevinfo: The ipdevinfo view being tested by this is running asynchronouse DNS requests by manually iterating a Twisted reactor, and it seems to be getting stuck when running under the test suite once Twisted is upgraded. When manually running the web site and accessing the URLs in question, there is no problem, so there seems to be some sort of Twisted reactor interaction issue only during pytest runs....

1.14 apparently caused issues with hanging tests.  Don't know why,
because pytest-twisted does not seem to publish a
proper changelog, so I have no idea what changed between 1.13 and 1.14.
@lunkwill42
Copy link
Member Author

The abovementioned test will actually succeed if I downgrade pytest-twisted from 1.14 to 1.13. But yet other tests will hang, like tests/integration/ipdevpoll/utils_test.py::test_get_arista_vrf_instances_should_return_expected_instances

This type annotation caused some tests to freeze indefinitely when
upgrading from Twisted 20 to 23.8.  The correct location for `Failure`
is in the module `twisted.python.failure`, but it may have been
incidentally available also in `twisted.internet.defer.failure` in
earlier versions.
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.14%. Comparing base (de2a90b) to head (68e0641).
Report is 35 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2796      +/-   ##
==========================================
+ Coverage   57.10%   57.14%   +0.04%     
==========================================
  Files         567      567              
  Lines       41278    41272       -6     
==========================================
+ Hits        23571    23586      +15     
+ Misses      17707    17686      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lunkwill42 lunkwill42 marked this pull request as ready for review February 29, 2024 13:23
@lunkwill42 lunkwill42 requested a review from hmpf February 29, 2024 13:23
@lunkwill42
Copy link
Member Author

The abovementioned test will actually succeed if I downgrade pytest-twisted from 1.14 to 1.13. But yet other tests will hang, like tests/integration/ipdevpoll/utils_test.py::test_get_arista_vrf_instances_should_return_expected_instances

In the end, it turned out that this was some code that crashed and apparently caused the event loop to get stuck without handling the crash: Namely, a type annotation in a function was now referencing a class that was no longer available in the Twisted module it was originally imported from.

I had to run pytest with the --pdb option and interrupt the hanging test suite to get a proper traceback that would reveal this.

@lunkwill42 lunkwill42 merged commit cdac0a5 into Uninett:master Feb 29, 2024
12 checks passed
@lunkwill42 lunkwill42 deleted the upgrade/twisted branch February 29, 2024 13:38
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.

Upgrade Twisted to a version that supports Python 3.11
2 participants