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

ggivo · 2024-11-10T17:33:24Z

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)

Fix NPE on CommandInfo - some of the array elements returned are available since ( Redis server 6.0/7.0)
(see NPE thrown when invoking Jedis#commandInfo against Redis server 6.2.16 #4017 )
Fix NPE when reading AccessControlLogEntry
(see NPE thrown on jedis.aclLog() used against Redis server 7.2.0< #4016)

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

src/test/java/redis/clients/jedis/util/TlsUtil.java

src/test/java/redis/clients/jedis/SSLJedisClusterTest.java

src/test/java/redis/clients/jedis/SSLJedisSentinelPoolTest.java

.github/workflows/test-on-docker.yml

tishun · 2024-11-13T08:43:00Z

LGTM, the code looks much neater this way!

atakavci

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

.github/workflows/test-on-docker.yml

Makefile

src/test/java/io/redis/test/utils/EnabledOnCommandRule.java

src/test/java/io/redis/test/utils/RedisVersionRule.java

src/test/resources/Arch/sentinel5/config/node-sentinel5/redis.conf

src/test/java/redis/clients/jedis/csc/JedisPooledClientSideCacheTest.java

src/test/java/redis/clients/jedis/csc/ClientSideCacheTestBase.java

src/test/java/redis/clients/jedis/commands/jedis/ControlCommandsTest.java

src/test/java/redis/clients/jedis/util/RedisVersionRule.java

src/test/java/redis/clients/jedis/ACLJedisPoolTest.java

src/test/java/redis/clients/jedis/ClusterPipeliningTest.java

src/test/java/redis/clients/jedis/EndpointConfig.java

src/test/java/redis/clients/jedis/JedisClusterTest.java

...test/java/redis/clients/jedis/commands/jedis/ClusterShardedPublishSubscribeCommandsTest.java

src/test/java/redis/clients/jedis/commands/jedis/ClusterJedisCommandsTestBase.java

src/test/java/redis/clients/jedis/commands/jedis/AllKindOfValuesCommandsTest.java

src/test/java/redis/clients/jedis/JedisTest.java

src/test/java/redis/clients/jedis/commands/jedis/ClusterJedisCommandsTestBase.java

src/test/java/redis/clients/jedis/commands/unified/cluster/ClusterBitCommandsTest.java

src/test/java/redis/clients/jedis/commands/unified/cluster/ClusterCommandsTestHelper.java

src/test/java/redis/clients/jedis/csc/JedisClusterClientSideCacheTest.java

src/main/java/redis/clients/jedis/resps/CommandInfo.java

src/test/java/redis/clients/jedis/JedisClusterTestBase.java

src/test/java/redis/clients/jedis/commands/commandobjects/CommandObjectsTestBase.java

src/test/java/redis/clients/jedis/EndpointConfig.java

src/test/java/redis/clients/jedis/SSLACLJedisClusterTest.java

formating & clean up Co-authored-by: M Sazzadul Hoque <[email protected]>

src/test/java/redis/clients/jedis/util/TlsUtil.java

"This uses , which is defined in and trusts any certificate."

By mistake isEqualWithTolerance was using latitude instead of tolerance hiding that mistakenly DEFAULT_TOLERANCE was changed from 1e-5 to 1e-14

Use .sslVerifyMode(SslVerifyMode.CA) to mimic tested behaviour of SSLJedisClusterTest.testSSLWithoutPortMap() Fix actions/upload-artifact@v3 deprecation

Since we have removed the stunnel and using Redis server native TLS supporttest is failing on 6.2.x branch because CLUSTER SLOTS command is returning non-tls port when using (Redis 6.2.x). Test is passing successfully against 7.x+ Redis instances.Considering previously nightly build was run only against latest Redis server (e.g 7.x+) it should be safe to mark this test with @SinceRedisVersion(value = "7.0.0")

"certificateLocation" point to root folder were certificates used by the server are stored for given endpoint. Expected following set of certificates redis.crt - RedisServer certificate client.crt - Can be used by the client for mutual auth with the server ca.crt - Root certificate for both (redis.crt & client.crt) Replace hardcoded certificateLocation with one resol ved from EndpiintConfig were possible. e.g. Cluster tests are not yet migrated to EndpointConfig infra and still use hardcoded certificates.

…ce Redis version

SearchWithParamsTest.searchTextFieldsCondition:276 » JedisData Use `INDEXEMPTY` in field creation in order to index and query for empty strings

…port it

…M02 release - AllKindOfValuesCommandsTest.restoreParams - AllKindOfValuesCommandsTestBase.restoreParams

ggivo self-assigned this Nov 10, 2024

github-advanced-security bot found potential problems Nov 10, 2024

View reviewed changes

src/test/java/redis/clients/jedis/util/TlsUtil.java Fixed Show fixed Hide fixed

src/test/java/redis/clients/jedis/util/TlsUtil.java Fixed Show fixed Hide fixed

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 marked this pull request as ready for review November 11, 2024 06:03

This was referenced Nov 11, 2024

Revisit TLS Cluster tests to drop stunnel #4018

Open

Fix AccessControlListCommandsTest tests against Redis 6.2.16 #4019

Open

ggivo force-pushed the migrate_to_clients_test_image branch from 77f7e4e to a691416 Compare November 11, 2024 08:36

sazzad16 added the testing label Nov 12, 2024

tishun reviewed Nov 13, 2024

View reviewed changes

ggivo force-pushed the migrate_to_clients_test_image branch 2 times, most recently from 5066271 to 1698b76 Compare November 19, 2024 18:40

ggivo requested review from sazzad16 and atakavci November 19, 2024 19:48

atakavci previously approved these changes Nov 21, 2024

View reviewed changes

ggivo dismissed atakavci’s stale review via e1c0453 November 23, 2024 20:52

sazzad16 requested changes Nov 24, 2024

View reviewed changes

ggivo force-pushed the migrate_to_clients_test_image branch from b5e04e4 to 6c58815 Compare November 25, 2024 12:42

sazzad16 reviewed Nov 26, 2024

View reviewed changes

src/test/java/redis/clients/jedis/commands/jedis/ControlCommandsTest.java Outdated Show resolved Hide resolved

sazzad16 reviewed Nov 26, 2024

View reviewed changes

src/test/java/redis/clients/jedis/commands/jedis/ControlCommandsTest.java Outdated Show resolved Hide resolved

sazzad16 requested changes Nov 27, 2024

View reviewed changes