-
Notifications
You must be signed in to change notification settings - Fork 53
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
Py/rc testing #2314
Conversation
) | ||
|
||
|
||
def start_servers(cluster_mode: bool, shard_count: int, replica_count: int) -> str: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run all IT instead
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Adar Ovadia <[email protected]>
Signed-off-by: Adar Ovadia <[email protected]>
Signed-off-by: avifenesh <[email protected]>
Signed-off-by: avifenesh <[email protected]>
Signed-off-by: avifenesh <[email protected]>
Signed-off-by: avifenesh <[email protected]>
Signed-off-by: avifenesh <[email protected]>
1a3eac8
to
e3687b1
Compare
import { GlideClient, GlideClusterClient } from "@valkey/valkey-glide"; | ||
import { getServerVersion } from "../../../node/tests/TestUtilities.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? pls remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wired, will do
Could you please add a description for this PR? How was it done in the previous release? |
Signed-off-by: Avi Fenesh <[email protected]>
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.