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

Introduces test matrix based on Redis versions [8.0-M1, 7.4.1, 7.2.6, 6.2.16] #4015

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

ggivo
Copy link
Contributor

@ggivo ggivo commented Nov 10, 2024

We use docker composer to bring up the test env using redislabs/client-libs-test image.

Example command to bring up test env for dev/test

  • Redis 8.0-M01
      export REDIS_VERSION=8.0-M01
      docker compose --env-file src/test/resources/env/.env -f src/test/resources/env/docker-compose.yml up
  • Redis 7.4.1
      export REDIS_VERSION=7.4.1
      docker compose --env-file src/test/resources/env/.env -f src/test/resources/env/docker-compose.yml up
  • Redis 7.2.6
      export REDIS_VERSION=7.2.6
      docker compose --env-file src/test/resources/env/.env -f src/test/resources/env/docker-compose.yml up
  • Redis 6.2.16
    - NOTE : 6.2.16 uses a dedicated .env.v6.12.16 file, since some of the redis configuration settings are not supported in 6.2.16
    docker compose --env-file src/test/resources/env/.env.v6.12.16 -f src/test/resources/env/docker-compose.yml up

Introduces test matrix based on Redis versions [8.0-M1, 7.4.1, 7.2.6, 6.2.16]

When run against older Redis version it appears that some tests are using commands available only in newer Redis server versions. To resolve this we are introducing two new annotations/rules

  • Introduce SinceRedisVersion annotation/Rule - for conditionally running tests based on Redis server version contacted
  • Introduce EnableOnCommad annotation/Rule - for conditionally running tests based on command availability on the server

And mark respective tests with the least Redis Version required by the test

  • SinceRedisVersion ("7.4.0") - Mark tests using commands/modifiers introduced with Redis 7.4.0
  • SinceRedisVersion ("7.2.0") - Mark tests using commands/modifiers introduced with Redis 7.2.0
  • SinceRedisVersion ("7.0.0") - Mark tests using commands/modifiers introduced with Redis 7.0.0

The same approach used to mark CSC tests

  • Disabled client-side caching tests for versions below 7.4

Fix in Jedis Client (Redis 6.x)

Fixed Test failures against 6.x

  • Fix JedisPooledClientSideCacheTest
  • Fix AccessControlListCommandsTest.aclLogTest:372 » NullPointer
  • Fix AccessControlListCommandsTest.aclLogWithEntryID:473 » NullPointer
  • Fix StreamsCommandsTest
  • Fix StreamsPipelineCommandsTest xadd - Starting with Redis version 7.0.0: Added support for the -* explicit ID form.

- Test env migrated to use native Redis server TLS instead of using stunnel

  • There are still Cluster SSL tests failing against 6.2.16 env. see Revisit TLS Cluster tests to drop stunnel #4018
    The primary reason for those failures is the Redis server 6.2.16 is returning the non-TLS port in the CLUSTER SLOTS command in contrast to Redis Server 7.2.0+. I suggest reviewing & addressing Cluster SSL related test failures with follow-up PR.

Ignored tests during migration

  • Disabled ModuleTest in containerized test env.
    ModuleTest uses a custom test module to test load/unload/sendCommand. Requires pre-build test module on the same os like test container to avoid errors

  • Disable UDS tests in containerized test env
    No easy way to make unix sockets work on MAC with docker

Closes #4016, Closes #4017

@ggivo ggivo self-assigned this Nov 10, 2024
src/test/java/redis/clients/jedis/util/TlsUtil.java Dismissed Show dismissed Hide dismissed
src/test/java/redis/clients/jedis/util/TlsUtil.java Dismissed Show dismissed Hide dismissed
@ggivo ggivo force-pushed the migrate_to_clients_test_image branch 2 times, most recently from 68c372a to 0aa9343 Compare November 10, 2024 19:30
@ggivo ggivo marked this pull request as ready for review November 11, 2024 06:03
@ggivo ggivo force-pushed the migrate_to_clients_test_image branch from 77f7e4e to a691416 Compare November 11, 2024 08:36
@tishun
Copy link

tishun commented Nov 13, 2024

LGTM, the code looks much neater this way!

@ggivo ggivo force-pushed the migrate_to_clients_test_image branch from f965234 to 5cfdb42 Compare November 19, 2024 15:53
ggivo and others added 18 commits November 19, 2024 20:40
… 6.2.16]

Use docker composer to bring up the test env using `redislabs/client-libs-test` image.

When run against older Redis version some tests are using commands available only in newer Redis server versions. To resolve this we are introducing two new annotations/rules

 - Introduce `SinceRedisVersion` annotation/Rule - for conditionally running tests based on Redis server version contacted
 - Introduce `EnableOnCommad` annotation/Rule -  for conditionally running tests based on command availability on the server

And mark respective tests with the least Redis Version required by the test
 - SinceRedisVersion ("7.4.0") - Mark tests using commands/modifiers introduced with Redis 7.4.0
 - SinceRedisVersion ("7.2.0") - Mark tests using commands/modifiers introduced with Redis 7.2.0
 - SinceRedisVersion ("7.0.0") - Mark tests using commands/modifiers introduced with Redis 7.0.0

 The same approach used to mark CSC tests
 - Disabled client-side caching tests for versions below  7.4

Fix in Jedis Client against Redis server 6.x

 - Fix NPW on CommandInfo - some of the array elements returned are available based from given RedisServer
         aclCategories (as of Redis 6.0) ,
         tips ,  (as of Redis 7.0) subcommands

 - Fix NPE  AccessControlLogEntry when used against Redis 6
   Starting with Redis version 7.2.0: Added entry ID, timestamp created, and timestamp last updated fields.

Fix Test failures against 6.x
 - Fix JedisPooledClientSideCacheTest
 - Fix AccessControlListCommandsTest.aclLogTest:372 » NullPointer
 - Fix AccessControlListCommandsTest.aclLogWithEntryID:473 » NullPointer
 - Fix StreamsCommandsTest
 - Fix StreamsPipelineCommandsTest xadd - Starting with Redis version 7.0.0: Added support for the <ms>-* explicit ID form.

- Test env migrated to use native Redis server TLS instead of  using stunnel

Not all tests were migrated
 - Disable Modules test in containerized test env ModuleTest uses custom test module to test load/unload/sendCommand.
    Requires pre-build test module on the same os like test container to avoid errors

 - Disable UDS tests in containerized test env
    No easy way to make unix sockets work on MAC with docker
java.lang.AssertionError:
Expected :[(2.1909382939338684,41.433790281840835), (2.187376320362091,41.40634178640635)]
Actual   :[(2.1909382939338684,41.43379028184083), (2.187376320362091,41.40634178640635)]

Compare with tolerance to avoid error on returned precision on mac
Adding ignored unit test for repro
Remove stunnel. Redis server version 6 and above have native support for TLS.
If set will assume this version instead of resolving it from tested server instaces.

Used to force running tests agains older server versions
In 6.2.16 CLUSTER slots command returns the non-tls port when contacted by tls one.
Tests need to be revisited to make them compatible with 6.2.x
In 6.2.16 CLUSTER slots command returns the non-tls port when contacted by tls one.
Tests need to be revisited to make them compatible with 6.2.x
atakavci
atakavci previously approved these changes Nov 21, 2024
Copy link
Contributor

@atakavci atakavci left a comment

Choose a reason for hiding this comment

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

this is hard work and i appreciate how we improve it.
i left a couple of comments and questions but none of them is a big deal TBH,
it looks pretty good to me.
one thing to note; we need to find our way through smaller PRs, at least for those not containing public API changes.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/test-on-docker.yml Outdated Show resolved Hide resolved
.github/workflows/test-on-docker.yml Show resolved Hide resolved
Makefile Show resolved Hide resolved
  - Remove unused configuration
  - [Code clean up] Use endpoint configuration for creating redis version rule
@ggivo ggivo force-pushed the migrate_to_clients_test_image branch from b5e04e4 to 6c58815 Compare November 25, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants