Streamline test execution (#3760)

* Fix a flaky test

It turns out that sometimes the slow log entry has a duration of zero,
so cover for this case too.

* Try to fix a flaky test

* Streamline unit test execution in CI vs. locally

The current way of running JUnit tests has some drawbacks.

The CI executes tests by running multiple `mvn` commands. When running
tests locally, it is tedious to run the same multiple commands, in
order, for example, to get the same coverage as the CI gets. Ideally
the tests are configured in the project itself, in the pom.xml
file and by annotations on the tests, so that simply running `mvn test`
will do everything needed.

In order to achieve this, adapt the tests to be parameterized, so that
JUnit takes care of running them with different RESP protocols. It is OK
to use environment variables in general, for example for injecting
connection details. But it is not OK to use environment variables for
driving test execution, by injecting a RESP version and running the
tests multiple times. If I have to run more than `mvn test`, it does not
feel right.

Some tests rely on static fields and @BeforeClass and @afterclass
annotations. These do not work well with parameterized tests, so remove
the static and switch to @before and @after annotations.

The Makefile is a bit unpolished, i.e. the sequence of tasks is
controlled manually when it could be controlled declaratively, and the
`start` and `stop` tasks do not cover starting and stopping the Redis
Stack docker. Take care of these aspects.

All in all, now if I run `make test` I know I get the correct coverage
in the JaCoCo report. And the CI can also just run `make test`, without
additional work.

* React to code review

Add comments to clarify the intentions. Make more tests parameterized.

Use a friendlier port number for Redis Stack.

* Remove unnecessary Jedis object.

Jedis object is now being created in every clearData() call. That removes the necessity of this object.

* Comment of RedisJsonV1Test class protocol limit

* Comment about default protocol

* Polish a bit some documentation

---------

Co-authored-by: Gabriel Erzse <[email protected]>
Co-authored-by: M Sazzadul Hoque <[email protected]>

.github/workflows/integration.yml

@@ -51,28 +51,10 @@ jobs:
           mvn javadoc:jar
       - name: Run tests
         run: |
-          TEST="" make test
+          make test
         env:
           JVM_OPTS: -Xmx3200m
           TERM: dumb
-      - name: sleep 10s
-        run: sleep 10s
-      - name: Make - start
-        run: |
-          make start
-          sleep 2s
-      - name: Docker - mod or stack
-        run: docker run -p 52567:6379 -d redis/redis-stack-server:edge
-      - name: Test commands - default protocol
-        run: mvn -Dtest="redis.clients.jedis.commands.**" test
-      - name: Test commands - RESP3 protocol
-        run: mvn -DjedisProtocol=3 -Dtest="redis.clients.jedis.commands.**" test
-      - name: Test module commands - default protocol
-        run: mvn -DmodulesDocker="localhost:52567" -Dtest="redis.clients.jedis.modules.**" test
-      - name: Test module commands - RESP3 protocol
-        run: mvn -DjedisProtocol=3 -DmodulesDocker="localhost:52567" -Dtest="redis.clients.jedis.modules.**" test
-      - name: Make - stop
-        run: make stop
       - name: Codecov
         run: |
           bash <(curl -s https://codecov.io/bash)

Makefile

@@ -413,7 +413,7 @@ ifndef STUNNEL_BIN
 endif
 export SKIP_SSL
 
-start: stunnel cleanup
+start: stunnel cleanup compile-module
 	echo "$$REDIS1_CONF" | redis-server -
 	echo "$$REDIS2_CONF" | redis-server -
 	echo "$$REDIS3_CONF" | redis-server -
@@ -446,6 +446,7 @@ start: stunnel cleanup
 	echo "$$REDIS_UDS" | redis-server -
 	echo "$$REDIS_UNAVAILABLE_CONF" | redis-server -
 	redis-cli -a cluster --cluster create 127.0.0.1:7479 127.0.0.1:7480 127.0.0.1:7481 --cluster-yes
+	docker run -p 6479:6379 --name jedis-stack -d redis/redis-stack-server:edge
 
 cleanup:
 	- rm -vf /tmp/redis_cluster_node*.conf 2>/dev/null
@@ -455,6 +456,7 @@ stunnel:
 	@if [ -e "$$STUNNEL_BIN" ]; then\
 	    echo "$$STUNNEL_CONF" | stunnel -fd 0;\
 	fi
+
 stop:
 	kill `cat /tmp/redis1.pid`
 	kill `cat /tmp/redis2.pid`
@@ -496,29 +498,32 @@ stop:
 	rm -f /tmp/redis_stable_cluster_node1.conf
 	rm -f /tmp/redis_stable_cluster_node2.conf
 	rm -f /tmp/redis_stable_cluster_node3.conf
+	docker rm -f jedis-stack
+
+test: | start mvn-test stop
 
-test: compile-module start
-	sleep 2
+mvn-test:
 	mvn -Dtest=${SKIP_SSL}${TEST} clean compile test
-	make stop
 
-package: start
+package: | start mvn-package stop
+
+mvn-package:
 	mvn clean package
-	make stop
 
-deploy: start
+deploy: | start mvn-deploy stop
+
+mvn-deploy:
 	mvn clean deploy
-	make stop
 
 format:
 	mvn java-formatter:format
 
-release:
-	make start
+release: | start mvn-release stop
+
+mvn-release:
 	mvn release:clean
 	mvn release:prepare
 	mvn release:perform -DskipTests
-	make stop
 
 system-setup:
 	sudo apt install -y gcc g++

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

@@ -6,6 +6,8 @@
 import java.net.URISyntaxException;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import redis.clients.jedis.commands.jedis.JedisCommandsTestBase;
 import redis.clients.jedis.util.RedisVersionUtil;
@@ -15,6 +17,7 @@
  * <p>
  * This test is only executed when the server/cluster is Redis 6. or more.
  */
+@RunWith(Parameterized.class)
 public class ACLJedisTest extends JedisCommandsTestBase {
 
   /**
@@ -27,6 +30,10 @@ public static void prepare() throws Exception {
         RedisVersionUtil.checkRedisMajorVersionNumber(6));
   }
 
+  public ACLJedisTest(RedisProtocol redisProtocol) {
+    super(redisProtocol);
+  }
+
   @Test
   public void useWithoutConnecting() {
     try (Jedis j = new Jedis()) {

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

@@ -50,7 +50,7 @@ protected String encode(byte[] raw) {
         });
         thread.start();
 
-        assertTrue(countDownLatch.await(10, TimeUnit.MILLISECONDS));
+        assertTrue(countDownLatch.await(20, TimeUnit.MILLISECONDS));
 
     }
 }

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

@@ -17,15 +17,22 @@
 import java.util.Map;
 
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import redis.clients.jedis.exceptions.InvalidURIException;
 import redis.clients.jedis.exceptions.JedisConnectionException;
 import redis.clients.jedis.exceptions.JedisException;
 import redis.clients.jedis.commands.jedis.JedisCommandsTestBase;
 import redis.clients.jedis.util.SafeEncoder;
 
+@RunWith(Parameterized.class)
 public class JedisTest extends JedisCommandsTestBase {
 
+  public JedisTest(RedisProtocol protocol) {
+    super(protocol);
+  }
+
   @Test
   public void useWithoutConnecting() {
     try (Jedis j = new Jedis()) {

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

@@ -14,10 +14,14 @@
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
 import redis.clients.jedis.commands.jedis.JedisCommandsTestBase;
 import redis.clients.jedis.exceptions.JedisDataException;
 import redis.clients.jedis.params.MigrateParams;
 
+@RunWith(Parameterized.class)
 public class MigratePipeliningTest extends JedisCommandsTestBase {
 
   private static final byte[] bfoo = { 0x01, 0x02, 0x03 };
@@ -39,6 +43,10 @@ public class MigratePipeliningTest extends JedisCommandsTestBase {
   private Jedis dest;
   private Jedis destAuth;
 
+  public MigratePipeliningTest(RedisProtocol protocol) {
+    super(protocol);
+  }
+
   @Before
   @Override
   public void setUp() throws Exception {

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

@@ -24,20 +24,28 @@
 import org.hamcrest.Matcher;
 import org.hamcrest.Matchers;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
 import redis.clients.jedis.commands.ProtocolCommand;
 import redis.clients.jedis.commands.jedis.JedisCommandsTestBase;
 import redis.clients.jedis.exceptions.JedisDataException;
 import redis.clients.jedis.params.SetParams;
 import redis.clients.jedis.resps.Tuple;
 import redis.clients.jedis.util.SafeEncoder;
 
+@RunWith(Parameterized.class)
 public class PipeliningTest extends JedisCommandsTestBase {
 
   private static final byte[] bfoo = { 0x01, 0x02, 0x03, 0x04 };
   private static final byte[] bfoo1 = { 0x01, 0x02, 0x03, 0x04, 0x11, 0x12, 0x13, 0x14 };
   private static final byte[] bbar = { 0x05, 0x06, 0x07, 0x08 };
   private static final byte[] bbaz = { 0x09, 0x0A, 0x0B, 0x0C };
 
+  public PipeliningTest(RedisProtocol protocol) {
+    super(protocol);
+  }
+
   @Test
   public void pipeline() {
     Pipeline p = jedis.pipelined();

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

@@ -6,9 +6,13 @@
 import java.util.Collections;
 import java.util.List;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
 import redis.clients.jedis.resps.Tuple;
 import redis.clients.jedis.commands.jedis.JedisCommandsTestBase;
 
+@RunWith(Parameterized.class)
 public class TupleSortedSetTest extends JedisCommandsTestBase {
   final byte[] bfoo = { 0x01, 0x02, 0x03, 0x04 };
   final byte[] ba = { 0x0A };
@@ -18,6 +22,10 @@ public class TupleSortedSetTest extends JedisCommandsTestBase {
   final byte[] be = { 0x0E };
   final byte[] bf = { 0x0F };
 
+  public TupleSortedSetTest(RedisProtocol protocol) {
+    super(protocol);
+  }
+
   @Test
   public void testBinary() {
     List<Tuple> array = new ArrayList<Tuple>();

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

@@ -0,0 +1,22 @@
+package redis.clients.jedis.commands;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import redis.clients.jedis.RedisProtocol;
+
+public class CommandsTestsParameters {
+
+  /**
+   * RESP protocol versions we want our commands related tests to run against.
+   * {@code null} means to use the default protocol which is assumed to be RESP2.
+   */
+  public static Collection<Object[]> respVersions() {
+    return Arrays.asList(
+        new Object[]{ null },
+        new Object[]{ RedisProtocol.RESP2 },
+        new Object[]{ RedisProtocol.RESP3 }
+    );
+  }
+
+}

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

@@ -18,10 +18,13 @@
 import org.junit.After;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import redis.clients.jedis.CommandArguments;
 import redis.clients.jedis.Jedis;
 import redis.clients.jedis.Protocol;
+import redis.clients.jedis.RedisProtocol;
 import redis.clients.jedis.Transaction;
 import redis.clients.jedis.exceptions.JedisAccessControlException;
 import redis.clients.jedis.exceptions.JedisDataException;
@@ -33,6 +36,7 @@
 /**
  * TODO: properly define and test exceptions
  */
+@RunWith(Parameterized.class)
 public class AccessControlListCommandsTest extends JedisCommandsTestBase {
 
   public static final String USER_NAME = "newuser";
@@ -45,6 +49,10 @@ public static void prepare() throws Exception {
         RedisVersionUtil.checkRedisMajorVersionNumber(6));
   }
 
+  public AccessControlListCommandsTest(RedisProtocol protocol) {
+    super(protocol);
+  }
+
   @After
   @Override
   public void tearDown() throws Exception {

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

@@ -18,6 +18,8 @@
 import org.hamcrest.Matchers;
 import org.junit.Assume;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import redis.clients.jedis.HostAndPort;
 import redis.clients.jedis.Jedis;
@@ -35,8 +37,8 @@
 import redis.clients.jedis.params.SetParams;
 import redis.clients.jedis.util.AssertUtil;
 import redis.clients.jedis.util.KeyValue;
-import redis.clients.jedis.util.RedisProtocolUtil;
 
+@RunWith(Parameterized.class)
 public class AllKindOfValuesCommandsTest extends JedisCommandsTestBase {
   final byte[] bfoo = { 0x01, 0x02, 0x03, 0x04 };
   final byte[] bfoo1 = { 0x01, 0x02, 0x03, 0x04, 0x0A };
@@ -57,6 +59,10 @@ public class AllKindOfValuesCommandsTest extends JedisCommandsTestBase {
 
   private static final HostAndPort lfuHnp = HostAndPorts.getRedisServers().get(7);
 
+  public AllKindOfValuesCommandsTest(RedisProtocol redisProtocol) {
+    super(redisProtocol);
+  }
+
   @Test
   public void ping() {
     String status = jedis.ping();
@@ -984,7 +990,7 @@ public void encodeCompleteResponsePing() {
 
   @Test
   public void encodeCompleteResponseHgetall() {
-    Assume.assumeFalse(RedisProtocolUtil.getRedisProtocol() == RedisProtocol.RESP3);
+    Assume.assumeFalse(protocol == RedisProtocol.RESP3);
 
     HashMap<String, String> entries = new HashMap<>();
     entries.put("foo", "bar");
@@ -1002,7 +1008,7 @@ public void encodeCompleteResponseHgetall() {
 
   @Test
   public void encodeCompleteResponseHgetallResp3() {
-    Assume.assumeTrue(RedisProtocolUtil.getRedisProtocol() == RedisProtocol.RESP3);
+    Assume.assumeTrue(protocol == RedisProtocol.RESP3);
 
     HashMap<String, String> entries = new HashMap<>();
     entries.put("foo", "bar");
@@ -1019,7 +1025,7 @@ public void encodeCompleteResponseHgetallResp3() {
 
   @Test
   public void encodeCompleteResponseXinfoStream() {
-    Assume.assumeFalse(RedisProtocolUtil.getRedisProtocol() == RedisProtocol.RESP3);
+    Assume.assumeFalse(protocol == RedisProtocol.RESP3);
 
     HashMap<String, String> entry = new HashMap<>();
     entry.put("foo", "bar");
@@ -1046,7 +1052,7 @@ public void encodeCompleteResponseXinfoStream() {
 
   @Test
   public void encodeCompleteResponseXinfoStreamResp3() {
-    Assume.assumeTrue(RedisProtocolUtil.getRedisProtocol() == RedisProtocol.RESP3);
+    Assume.assumeTrue(protocol == RedisProtocol.RESP3);
 
     HashMap<String, String> entry = new HashMap<>();
     entry.put("foo", "bar");

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

@@ -18,12 +18,16 @@
 
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import redis.clients.jedis.Protocol;
+import redis.clients.jedis.RedisProtocol;
 import redis.clients.jedis.exceptions.JedisDataException;
 import redis.clients.jedis.params.GetExParams;
 import redis.clients.jedis.util.SafeEncoder;
 
+@RunWith(Parameterized.class)
 public class BinaryValuesCommandsTest extends JedisCommandsTestBase {
   byte[] bfoo = { 0x01, 0x02, 0x03, 0x04 };
   byte[] bbar = { 0x05, 0x06, 0x07, 0x08 };
@@ -35,6 +39,10 @@ public class BinaryValuesCommandsTest extends JedisCommandsTestBase {
   long expireMillis = expireSeconds * 1000;
   byte[] binaryValue;
 
+  public BinaryValuesCommandsTest(RedisProtocol protocol) {
+    super(protocol);
+  }
+
   @Before
   public void startUp() {
     StringBuilder sb = new StringBuilder();