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

Replace "redis" with "valkey" test code #287

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

Shivshankar-Reddy
Copy link
Contributor

@Shivshankar-Reddy Shivshankar-Reddy commented Apr 10, 2024

Occurrences of "redis" in TCL test suites and helpers, such as TCL client used in tests, are replaced with "valkey".

Occurrences of uppercase "Redis" are not changed in this PR.

No files are renamed in this PR.

Signed-off-by: Shivshankar-Reddy <[email protected]>
Signed-off-by: Shivshankar-Reddy <[email protected]>
Signed-off-by: Shivshankar-Reddy <[email protected]>
@Shivshankar-Reddy
Copy link
Contributor Author

@enjoy-binbin @zuiderkwast I have changed most of redis to valkey in this PR, I tried changing meaningfull names but it gave lot of trouble and test case failure so I went with replacing redis with "valkey". If you have some time could you please have a look at this?

src/cli_common.c Outdated
Comment on lines 321 to 323
const char *scheme = "redis://";
const char *tlsscheme = "rediss://";
const char *scheme = "valkey://";
const char *tlsscheme = "valkeys://";
const char *curr = uri;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not test code.

This is a breaking change. It is #198. See the discussions in #199.

We're not doing that. Besides, it is interfering with #199.

We already have had several PRs doing the same changes and we're reviewing them over and over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got your point, But I had benchmark: tls connecting using URI with authentication set,get and benchmark: connecting using URI with authentication set,get test failed. that was the reason I changed, No problem we can wait until #199 (#198) gets merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure #198 will get merged at all.

Either way, I think we should test the redis:// protocol to make sure we support it. We want valkey-cli 8.0 to be compatible with valkey-server 7.2.x and with redis oss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh.. Do we have any macro or check for backport compatibilty defined, I can go ahead update the commit in this pr or first I will commit them in different PR as they are going to be breaking changes and later we can focus on this PR changes. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just keep redis:// and rediss:// in this PR. Only update internal names in the test.

We are sure we want to support it and still test it.

We are not yet sure if we want to add another scheme. If we do, we want to test both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the details, sure I will change internally in test and commit again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completed the changes and updated tests as well to pass now, when scheme is added we can change those test together. Thanks!

Signed-off-by: Shivshankar-Reddy <[email protected]>
@enjoy-binbin
Copy link
Member

@Shivshankar-Reddy
Copy link
Contributor Author

full ci: https://github.com/valkey-io/valkey/actions/runs/8731463488

Thank you @enjoy-binbin , I will look into it and fix.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Very good. I will merge it before there are new merge conflicts. :)

Just to be clear what this PR is doing.

I can see only lowercase "redis" is replaced by "valkey".

It does not change "Redis" (in the output of ./runtest --help) but we can do that in another PR.

It also does not rename any files. We can do that in another PR too. (Rename redis.tcl to valkey.tcl.)

@zuiderkwast zuiderkwast changed the title Rename test procedures to valkey Replace "redis" with "valkey" test code Apr 18, 2024
@zuiderkwast zuiderkwast merged commit 34413e0 into valkey-io:unstable Apr 18, 2024
14 checks passed
@Shivshankar-Reddy Shivshankar-Reddy deleted the rebrand-test-proc branch April 18, 2024 15:40
@Shivshankar-Reddy
Copy link
Contributor Author

full ci: https://github.com/valkey-io/valkey/actions/runs/8731463488

Hi @enjoy-binbin , The failures happen on Mac OS workflows and I dont have Mac book so I have tried replicate the same tests on Ubuntu machine for multiple times and I could not replicate them.

From the logs we could see below tests failed.

  1. We can read back the value we set before
  2. Create a 5 nodes cluster

We may need help fom someone who has Mac and test this cases and see whether its related to CI or fails on manual run as well..

PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Occurrences of "redis" in TCL test suites and helpers, such as TCL
client used in tests, are replaced with "valkey".

Occurrences of uppercase "Redis" are not changed in this PR.

No files are renamed in this PR.

---------

Signed-off-by: Shivshankar-Reddy <[email protected]>
zuiderkwast pushed a commit that referenced this pull request Apr 24, 2024
Renamed below procedures and variables (missed in #287) as follows.

redis_cluster             ->     valkey_cluster
redis1                    ->     valkey1
redis2                    ->     valkey2
get_redis_dir             ->     get_valkey_dir
test_redis_cli_rdb_dump   ->     test_valkey_cli_rdb_dump
test_redis_cli_repl       ->     test_valkey_cli_repl
redis-cli                 ->     valkey-cli
redis_reset_state         ->     valkey_reset_state
redisHandle               ->     valkeyHandle
redis_safe_read           ->     valkey_safe_read
redis_safe_gets           ->     valkey_safe_gets
redis_write               ->     valkey_write
redis_read_reply          ->     valkey_read_reply
redis_readable            ->     valkey_readable
redis_readnl              ->     valkey_readnl
redis_writenl             ->     valkey_writenl
redis_read_map            ->     valkey_read_map
redis_read_line           ->     valkey_read_line
redis_read_null           ->     valkey_read_null
redis_read_bool           ->     valkey_read_bool
redis_read_double         ->     valkey_read_double
redis_read_verbatim_str   ->     valkey_read_verbatim_str
redis_call_callback       ->     valkey_call_callback

---------

Signed-off-by: Shivshankar-Reddy <[email protected]>
WM0323 pushed a commit to WM0323/valkey that referenced this pull request Apr 26, 2024
Renamed below procedures and variables (missed in valkey-io#287) as follows.

redis_cluster             ->     valkey_cluster
redis1                    ->     valkey1
redis2                    ->     valkey2
get_redis_dir             ->     get_valkey_dir
test_redis_cli_rdb_dump   ->     test_valkey_cli_rdb_dump
test_redis_cli_repl       ->     test_valkey_cli_repl
redis-cli                 ->     valkey-cli
redis_reset_state         ->     valkey_reset_state
redisHandle               ->     valkeyHandle
redis_safe_read           ->     valkey_safe_read
redis_safe_gets           ->     valkey_safe_gets
redis_write               ->     valkey_write
redis_read_reply          ->     valkey_read_reply
redis_readable            ->     valkey_readable
redis_readnl              ->     valkey_readnl
redis_writenl             ->     valkey_writenl
redis_read_map            ->     valkey_read_map
redis_read_line           ->     valkey_read_line
redis_read_null           ->     valkey_read_null
redis_read_bool           ->     valkey_read_bool
redis_read_double         ->     valkey_read_double
redis_read_verbatim_str   ->     valkey_read_verbatim_str
redis_call_callback       ->     valkey_call_callback

---------

Signed-off-by: Shivshankar-Reddy <[email protected]>

Signed-off-by: Sher Sun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants