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

Test all Python versions #1758

Merged
merged 10 commits into from
Jul 2, 2024
Merged

Test all Python versions #1758

merged 10 commits into from
Jul 2, 2024

Conversation

barshaul
Copy link
Collaborator

@barshaul barshaul commented Jul 2, 2024

This PR includes the following changes:

  1. Fixed Supported Engine Versions in Readmes:

    • Updated documentation to accurately reflect supported engine versions.
  2. Python Linting:

    • Excluded the submodules folder from mypy checks. We shouldn't run linters on external repositories.
  3. Python CI Enhancements:

    • Extended the Python version matrix in the CI tests to all supported Python versions (3.8 to 3.12) and fixed related test issues to ensure compatibility across versions.
  4. Bug Fixes and Improvements:

    • Corrected a function that was not being awaited properly.
    • Removed a flaky test for the "wait" command from the transaction module and changed integer types to doubles for where needed.
    • Temporarily excluded pubsub tests as a workaround for the flakey tests, until the flakiness will be resolved.

@barshaul barshaul force-pushed the test_all_py_vers branch 7 times, most recently from a0cf49a to 3e37e94 Compare July 2, 2024 16:27
@@ -626,8 +626,6 @@ async def transaction_test(
args.append(b"one")
transaction.srandmember_count(key7, 1)
args.append([b"one"])
transaction.wait(1, 1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

@barshaul barshaul Jul 2, 2024

Choose a reason for hiding this comment

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

I wrote in the description but i'll elaborate about the reason - "wait" has a flakey response in its nature as it requires the replicas to respond to the server in the specified timeout duration. If the replicas doesn't get to answer in time - which can happen in an integration test environment, we wouldn't get the expected result. The test already expect not to get a the replica to respond in time (expected value is 0), but we see in some tests that the return value is 1 meaning that the replica was able to response in the specified time. So we see some flakeyness where the return value is 1 instead of 0. This command shouldn't be tested inside of the long transaction test as it makes the whole test flakey.

@barshaul barshaul force-pushed the test_all_py_vers branch from 3e37e94 to f14c7a2 Compare July 2, 2024 17:04
@Yury-Fridlyand Yury-Fridlyand added python Python wrapper CI/CD CI/CD related labels Jul 2, 2024
python/pytest.ini Outdated Show resolved Hide resolved
@barshaul barshaul marked this pull request as ready for review July 2, 2024 17:32
@barshaul barshaul requested a review from a team as a code owner July 2, 2024 17:32
@barshaul barshaul force-pushed the test_all_py_vers branch from 5d2c56f to c08f8ba Compare July 2, 2024 17:57
@barshaul barshaul merged commit 07dd198 into valkey-io:main Jul 2, 2024
59 checks passed
Comment on lines +61 to +62
- "3.8"
- "3.9"
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, do we support 3.8 and 3.9 or not?
I still hear differnt gossips even now, after this PR merged.


GLIDE for Redis is API-compatible with open source Redis version 6 and 7.
Refer to the [Supported Engine Versions table](https://github.com/aws/glide-for-redis/blob/main/README.md#supported-engine-versions) for details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a relative link to avoid branch like 2.x poiting to main


Python 3.8 or higher.
| Python Version |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have similar for node, java and c#?

@@ -371,7 +371,7 @@ def _create_geosearch_args(
store_dist: bool = False,
) -> List[TEncodable]:
args: List[TEncodable] = keys
if isinstance(search_from, str | bytes):
if isinstance(search_from, (str, bytes)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

search_from should be TEncodable.

from datetime import date, datetime, timedelta, timezone
from typing import Any, Dict, List, Tuple, Union, cast
from typing import Any, Dict, List, Mapping, Tuple, Union, cast
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we stop using Mapping of favour of Dict?

@@ -388,7 +388,7 @@ async def transaction_test(
args.append(False)

transaction.zadd(key8, {"one": 1, "two": 2, "three": 3, "four": 4})
args.append(4)
args.append(4.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How it worked before?

@@ -471,16 +471,16 @@ async def transaction_test(
args.append(3)
transaction.zinter([key14, key15])
args.append([b"one", b"two"])
transaction.zinter_withscores(cast(list[str | bytes], [key14, key15]))
transaction.zinter_withscores(cast(List[Union[str, bytes]], [key14, key15]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

TEncodable

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jul 16, 2024
* Fixed Supported Engine Versions in all readmes

* Python lint: Exclude submodules folder from mypy

* Python CI: Added tests to all supported python versions and fixed tests accordingly

* Removed flakey "wait" test from transaction, changes ints to doubles

* Exclude pubsub tests as a temp solution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD CI/CD related python Python wrapper
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants