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.

Sign up for GitHub

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

Jump to bottom

Added Missing Client Tests. #127

Merged

SanHalacogluImproving merged 9 commits into java/integ_SanH_add_SharedClientTests from java/dev_SanH_add_SharedClientTests

Mar 13, 2024

SanHalacogluImproving commented Mar 5, 2024

No description provided.

acarbonetto reviewed

View reviewed changes

acarbonetto left a comment

This is hard to review because you've deleted-created the test files and it's hard to know what you've added.

I'd suggest a separate PR to rename the test files.
And another PR to add tests to the renamed test file.

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/standalone/ClientTests.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed

View reviewed changes

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/standalone/ClientTests.java Outdated

Yury-Fridlyand Mar 6, 2024

Please move common tests to one place and use parametrized test to avoid code duplicating
Some tests remain non common though

Author

SanHalacogluImproving Mar 6, 2024

Hmm, they had kept all these tests in one single class is breaking them up worth it?

Yury-Fridlyand Mar 6, 2024

We can do better!

Author

SanHalacogluImproving Mar 12, 2024

Done!

SanHalacogluImproving force-pushed the java/dev_SanH_add_SharedClientTests branch from 3412cf0 to 46963a5 Compare

March 6, 2024 22:03

Yury-Fridlyand reviewed

View reviewed changes

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed

View reviewed changes

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated

+                              RedisClusterClient.CreateClient(commonClusterClientConfig().build()).get();
+                      String key = "foo";
+                      String value = "שלום hello 汉字";

Yury-Fridlyand Mar 6, 2024

We may need to use unicode codes to store that string to keep source files ASCII
\x<code> and so on
https://www.ssec.wisc.edu/~tomw/java/unicode.html

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated

+                  @Test
+                  @SneakyThrows
+                  public void close_client_throws_ExecutionException_with_ClosingException_cause() {

Yury-Fridlyand Mar 6, 2024

Please move to exception hadling tests

Author

SanHalacogluImproving Mar 12, 2024

The tests in error handling are not parameterized which would require another PR change. Also, how would we go about initializing the clients if we close them once would we have to change it so it initializes the clients before each test?

java/integTest/src/test/java/glide/standalone/StandaloneClientTests.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed

View reviewed changes

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated Show resolved Hide resolved

SanHalacogluImproving added 6 commits

March 12, 2024 14:31


          Added Shared Client Tests.

f9523e3


          Renamed tests, migrated some functions to TestUtilities, minor refact…

a497fcf

…or to imports.


          For concurrent handling test changed executor from fixed size to cach…

7c6e51d

…ed thread pool.


          Minor update to test name and ordering.

f9cbb19


          Fixed functionality of the power function.

cb3389c


          Refactored shared commands in to a shared tests file.

0793cfb

SanHalacogluImproving force-pushed the java/dev_SanH_add_SharedClientTests branch from 8c69197 to 0793cfb Compare

March 12, 2024 21:31


          Minor update to IT tests based on PR comments.

106698a

aaron-congo approved these changes

View reviewed changes

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java Outdated Show resolved Hide resolved

java/integTest/src/test/java/glide/standalone/StandaloneClientTests.java Outdated Show resolved Hide resolved

SanHalacogluImproving added 2 commits

March 13, 2024 11:26


          Minor update.

6a09e0f


          Spotless.

26472f9

SanHalacogluImproving requested a review from jonathanl-bq

March 13, 2024 18:28

jonathanl-bq approved these changes

View reviewed changes

jonathanl-bq left a comment

Don't have full context, but seems fine to me.

SanHalacogluImproving merged commit 8334d28 into java/integ_SanH_add_SharedClientTests

10 checks passed

SanHalacogluImproving added a commit that referenced this pull request


          Java: Added Missing Client Tests. (#127) (valkey-io#1120)

5c6a79d

* Java: Added Missing Client Tests. (#127)

* Minor update.

* Refactored to unicode litterals.

SanHalacogluImproving deleted the java/dev_SanH_add_SharedClientTests branch

March 19, 2024 18:01

cyip10 pushed a commit that referenced this pull request


          Java: Added Missing Client Tests. (#127) (valkey-io#1120)

5ccf8b5

* Java: Added Missing Client Tests. (#127)

* Minor update.

* Refactored to unicode litterals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet