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.

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

Bring python 3.13 into integration tests #363

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Bring python 3.13 into integration tests #363

wants to merge 5 commits into from
+31 −19

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Aug 9, 2024 •
edited
Loading

It is resurrection of #336
Brings python 3.13 to the integration tests.
Updates scylla to 5.4 and switches from gabrielfalcao/pyenv-action@v16 to actions/setup-python@v5

@dkropachev dkropachev marked this pull request as ready for review August 9, 2024 14:15
@dkropachev dkropachev requested review from Lorak-mmk, fruch and sylwiaszunejko and removed request for Lorak-mmk August 9, 2024 14:15
fruch
fruch approved these changes Aug 9, 2024
View reviewed changes
.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved
@dkropachev dkropachev self-assigned this Aug 9, 2024
@Lorak-mmk Lorak-mmk mentioned this pull request Aug 12, 2024
Closed
Lorak-mmk
Lorak-mmk requested changes Aug 12, 2024
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Nit: it is a good practice to have all commits passing tests. IIUC the first commit changes used Scylla version, which breaks some tests, and subsequent commits fix this breakage. If this is true, then could you move first commit to be the last? That would keep all commits passing.

.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved
tests/integration/__init__.py Outdated Show resolved Hide resolved
tests/integration/standard/test_metadata.py Outdated Show resolved Hide resolved
tests/integration/standard/test_metadata.py Outdated Show resolved Hide resolved
tests/integration/standard/test_cluster.py Outdated Show resolved Hide resolved
@mykaul
Copy link

mykaul commented Aug 13, 2024

I've read somewhere that 3.13 optionally / experimentally removes the GIL. Would be good to test without it, to see where we are at.

@dkropachev dkropachev force-pushed the int_tests_on_3.13 branch 2 times, most recently from 2479b98 to 4e6329c Compare August 15, 2024 21:25
@dkropachev dkropachev requested a review from Lorak-mmk August 15, 2024 23:04
@dkropachev
Copy link
Collaborator Author

I've read somewhere that 3.13 optionally / experimentally removes the GIL. Would be good to test without it, to see where we are at.

#370

@dkropachev
Copy link
Collaborator Author

dkropachev commented Aug 16, 2024 •
edited
Loading

Nit: it is a good practice to have all commits passing tests. IIUC the first commit changes used Scylla version, which breaks some tests, and subsequent commits fix this breakage. If this is true, then could you move first commit to be the last? That would keep all commits passing.

It is good idea, but, reality is that full test suit takes 30 minutes to complete, say you have 5 commits, it means that I have to spend 150 minutes on testing them.
Now, say I want to change something in the partilar commit, I have to go back to the commit I am changing, spend 30 minutes on testing it and, if code I have changed is not isolated, I have retest every commit that goes after it, does it worth it ? I think no

@Lorak-mmk
Copy link

Sure, it is not practical to run integration tests on each commit right now, because they take too long. But:

  • imo it is still worth it to try and have commits without braking tests - it makes it easier to read such commits and if there is a need to bisect something, there will be less fixing to do (even if there still is some fixing)
  • running unit tests will catch a lot of problems and is much faster.

Lorak-mmk
Lorak-mmk requested changes Aug 20, 2024
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Note for the future: if you resolved an issue pointed by the comment, please mark the comment as resolved.

tests/integration/__init__.py Outdated Show resolved Hide resolved
tests/integration/__init__.py Outdated Show resolved Hide resolved
tests/integration/__init__.py Outdated Show resolved Hide resolved
Lorak-mmk
Lorak-mmk reviewed Aug 20, 2024
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

One more thing regarding comment tests/integration: introduced @xfail_scylla_version : commit message mentions xfail_scylla_version instead of xfail_scylla_version_lt

@fruch
Copy link

fruch commented Nov 6, 2024

@dkropachev

what was holding us from merging this one ?

@dkropachev
Copy link
Collaborator Author

xfail_scylla_version_lt

Fixed.

Lorak-mmk
Lorak-mmk requested changes Nov 18, 2024
View reviewed changes
tests/integration/__init__.py Outdated Show resolved Hide resolved
tests/integration/__init__.py Outdated Show resolved Hide resolved
tests/integration/simulacron/test_cluster.py Outdated Show resolved Hide resolved
@dkropachev dkropachev force-pushed the int_tests_on_3.13 branch 2 times, most recently from 502250a to 5edb0e1 Compare November 18, 2024 21:14
@fruch
Copy link

fruch commented Dec 17, 2024

@dkropachev we need this merged, without this one, and the move to use scylla 5.4, we can't install scylla anymore.
cause of the java lookup is broken

@dkropachev dkropachev force-pushed the int_tests_on_3.13 branch 4 times, most recently from 2748e27 to 82f1bab Compare December 19, 2024 12:43
@dkropachev
Copy link
Collaborator Author

@fruch , plan is to merge last pending PR: #397, rebase this PR and see what is left, which should be only pipeline changes.

@fruch @dkropachev
CI: add integration tests with python 3.13
ec2f58d 
since we want to start test the alpha/beta version of python
refactoring a bit the integration test workflow

the action for pyenv we are using isn't really getting updates
too much, and trying to switch back to the offical use python action
that can now have prerelease python versions
@dkropachev
rename assertNotEquals to assertNotEqual
cae1b82 
unittest renamed it and depricated assertNotEquals after 3.12
@dkropachev
Fix AsyncoreConnectionTests to check if asyncore present
0de2cd8 
Before 3.12 asyncore was present all the time.
Now we need to check if it is there.
@dkropachev
Fix test_ipv6 to test if asyncore is present
ba4e804 
asyncore is not present after 3.12
@dkropachev
Copy link
Collaborator Author

@fruch , @Lorak-mmk after all rebases it is left only with python-related changes please take a look.

@dkropachev dkropachev requested a review from Lorak-mmk December 22, 2024 15:11
fruch
fruch approved these changes Dec 22, 2024
View reviewed changes
Copy link

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@dkropachev
Replace assertDictContainsSubset
58847ec 
assertDictContainsSubset was removed at python 3.12 we need to replace
it with alternative
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruch fruch fruch approved these changes

@sylwiaszunejko sylwiaszunejko Awaiting requested review from sylwiaszunejko

@Lorak-mmk Lorak-mmk Awaiting requested review from Lorak-mmk

Requested changes must be addressed to merge this pull request.

Assignees

@dkropachev dkropachev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dkropachev @mykaul @Lorak-mmk @fruch