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

Open
wants to merge 11 commits into
base: release-1.1
Choose a base branch
from
Open

Py/rc testing #2314

wants to merge 11 commits into from

Conversation

avifenesh
Copy link
Collaborator

No description provided.

)


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?

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