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

Py/rc testing #2314

Merged
merged 13 commits into from
Sep 23, 2024
Merged

Py/rc testing #2314

merged 13 commits into from
Sep 23, 2024

Conversation

avifenesh
Copy link
Collaborator

@avifenesh avifenesh commented Sep 18, 2024

Added tests for RC and release for python, as we do for node.
We used to tests manually.
Here it will run on all platform, will download the new release and will run tests against it to make sure everything work as excepted.

)


def start_servers(cluster_mode: bool, shard_count: int, replica_count: int) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you re-use start/stop/parse from IT? No need to re-invent the wheel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Python is very harsh in this regard, i need the envs to be aware of each other.
If i want to reuse i need to take those functions out to the root folder, create a hack, or call it through cli.
Since it's not really something we use, it's just a fofo test, I'm not sure it's worth it.
Especially if it will affect the efficiency of the IT development because it is extra dependent.
I'm open to hearing if you think differently or you know it's easy to do so.
But importing the base on the path doesn't work.

output = start_servers(False, 1, 1)
servers, folder = parse_cluster_script_start_output(output)
standalone_client = await create_standalone_client(servers)
await run_commands(standalone_client)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Run all IT instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already had this discussion with Bar on the node rc tests.
That's not what those tests should do.
We have IT tests for a reason, and we have rc tests for another, the goal is just a slight check that nothing weird happened, mainly the compilation and packing.
Not to test all functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't find missing exports with these tests. We found missing exports for node client just because I ran all IT on an RC (#2307).
For basic checks you can run benchmark - and again without re-inventing a wheel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we want to run IT against the packed packages we should do it regularly using the npm pack for example and use it as "artifact".
Testing it during release doesn't make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand that it might be needed, but not during release, not against all host types, and do it regularly locally using packing.

The "not inventing the wheel" in the case of using the benchmarks is really much more pain than gain.
You take three different components of the repo with three different use cases and try to jingle between them when python anyway doesn't like stuff that is not in its environment.

I think its an over engineering.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

import { GlideClient, GlideClusterClient } from "@valkey/valkey-glide";
import { getServerVersion } from "../../../node/tests/TestUtilities.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

why? pls remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wired, will do

@adarovadya
Copy link
Collaborator

Could you please add a description for this PR? How was it done in the previous release?

Signed-off-by: Avi Fenesh <[email protected]>
@avifenesh avifenesh merged commit 0cff921 into release-1.1 Sep 23, 2024
24 checks passed
@avifenesh avifenesh deleted the PY/RC-testing branch September 23, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants