GEODE-8269: Improve test coverage (apache#5275)

geode-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java

@@ -24,7 +24,6 @@
 import org.apache.geode.cache.CacheFactory;
 import org.apache.geode.cache.GemFireCache;
 import org.apache.geode.redis.internal.GeodeRedisServer;
-import org.apache.geode.redis.internal.RegionProvider;
 import org.apache.geode.test.junit.rules.serializable.SerializableExternalResource;
 
 public class GeodeRedisServerRule extends SerializableExternalResource {
@@ -73,8 +72,4 @@ public GeodeRedisServerRule withPassword(String password) {
   public int getPort() {
     return server.getPort();
   }
-
-  public RegionProvider getRegionProvider() {
-    return server.getRegionProvider();
-  }
 }

geode-redis/src/distributedTest/java/org/apache/geode/redis/GeodeRedisServerStartupDUnitTest.java

@@ -192,7 +192,7 @@ public void whenNoRedisServers_unsupportedRedisCommandWillError() throws Excepti
   public void whenUnsupportedCommandsEnabledDynamicallyWithGfsh_newGeodeRedisServersWillRetainConfig()
       throws Exception {
     MemberVM locator = cluster.startLocatorVM(0);
-    MemberVM server1 = cluster.startServerVM(1, s -> s
+    cluster.startServerVM(1, s -> s
         .withProperty(REDIS_PORT, "0")
         .withProperty(REDIS_BIND_ADDRESS, "localhost")
         .withProperty(REDIS_ENABLED, "true")

geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HMsetDUnitTest.java

@@ -96,7 +96,6 @@ public static void tearDown() {
 
   @Test
   public void shouldDistributeDataAmongMultipleServers_givenMultipleClients() {
-
     String key = "key";
 
     Map<String, String> testMap = makeHashMap(HASH_SIZE, "field-", "value-");

geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/UnknownIntegrationTest.java

@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.redis.GeodeRedisServerRule;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+
+public class UnknownIntegrationTest {
+
+  public static Jedis jedis;
+  public static int REDIS_CLIENT_TIMEOUT =
+      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+
+  @ClassRule
+  public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+
+  @BeforeClass
+  public static void setUp() {
+    jedis = new Jedis("localhost", server.getPort(), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void flushAll() {
+    jedis.flushAll();
+  }
+
+  @AfterClass
+  public static void tearDown() {
+    jedis.close();
+  }
+
+  @Test
+  public void shouldReturnUnknownCommandError() {
+    assertThatThrownBy(() -> jedis.sendCommand(() -> "fhqwhgads".getBytes()))
+        .hasMessageContaining("Unable to process unknown command fhqwhgads");
+  }
+}

geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AuthIntegrationTest.java

@@ -28,7 +28,6 @@
 import org.junit.experimental.categories.Category;
 import redis.clients.jedis.Jedis;
 import redis.clients.jedis.Protocol;
-import redis.clients.jedis.exceptions.JedisDataException;
 
 import org.apache.geode.cache.CacheFactory;
 import org.apache.geode.cache.GemFireCache;
@@ -48,8 +47,6 @@ public class AuthIntegrationTest {
   Random rand;
   int port;
 
-  int runs = 150;
-
   @Before
   public void setUp() {
     rand = new Random();
@@ -93,16 +90,11 @@ public void testAuthConfig() {
   @Test
   public void testAuthRejectAccept() {
     setupCacheWithPassword();
-    Exception ex = null;
-    try {
-      jedis.auth("wrongpwd");
-    } catch (JedisDataException e) {
-      ex = e;
-    }
-    assertThat(ex).isNotNull();
 
-    String res = jedis.auth(PASSWORD);
-    assertThat(res).isEqualTo("OK");
+    assertThatThrownBy(() -> jedis.auth("wrongpwd"))
+        .hasMessageContaining("Attemping to authenticate with an invalid password");
+
+    assertThat(jedis.auth(PASSWORD)).isEqualTo("OK");
   }
 
   @Test
@@ -115,25 +107,16 @@ public void testAuthNoPwd() {
     server = new GeodeRedisServer("localhost", port);
     server.start();
 
-    Exception ex = null;
-    try {
-      jedis.auth(PASSWORD);
-    } catch (JedisDataException e) {
-      ex = e;
-    }
-    assertThat(ex).isNotNull();
+    assertThatThrownBy(() -> jedis.auth(PASSWORD))
+        .hasMessageContaining("Attempting to authenticate when no password has been set");
   }
 
   @Test
   public void testAuthAcceptRequests() {
     setupCacheWithPassword();
-    Exception ex = null;
-    try {
-      jedis.set("foo", "bar");
-    } catch (JedisDataException e) {
-      ex = e;
-    }
-    assertThat(ex).isNotNull();
+
+    assertThatThrownBy(() -> jedis.set("foo", "bar"))
+        .hasMessageContaining("Must authenticate before sending any requests");
 
     String res = jedis.auth(PASSWORD);
     assertThat(res).isEqualTo("OK");
@@ -144,28 +127,17 @@ public void testAuthAcceptRequests() {
   @Test
   public void testSeparateClientRequests() {
     setupCacheWithPassword();
-    Jedis authorizedJedis = null;
-    Jedis nonAuthorizedJedis = null;
-    try {
-      authorizedJedis = new Jedis("localhost", port, 100000);
-      nonAuthorizedJedis = new Jedis("localhost", port, 100000);
-      String res = authorizedJedis.auth(PASSWORD);
-      assertThat(res).isEqualTo("OK");
-      authorizedJedis.set("foo", "bar"); // No exception for authorized client
-
-      authorizedJedis.auth(PASSWORD);
-      Exception ex = null;
-      try {
-        nonAuthorizedJedis.set("foo", "bar");
-      } catch (JedisDataException e) {
-        ex = e;
-      }
-      assertThat(ex).isNotNull();
-    } finally {
-      if (authorizedJedis != null)
-        authorizedJedis.close();
-      if (nonAuthorizedJedis != null)
-        nonAuthorizedJedis.close();
-    }
+    Jedis nonAuthorizedJedis = new Jedis("localhost", port, 100000);
+    Jedis authorizedJedis = new Jedis("localhost", port, 100000);
+
+    assertThat(authorizedJedis.auth(PASSWORD)).isEqualTo("OK");
+    authorizedJedis.set("foo", "bar"); // No exception for authorized client
+    authorizedJedis.auth(PASSWORD);
+
+    assertThatThrownBy(() -> nonAuthorizedJedis.set("foo", "bar"))
+        .hasMessageContaining("Must authenticate before sending any requests");
+
+    authorizedJedis.close();
+    nonAuthorizedJedis.close();
   }
 }

geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/ExpireIntegrationTest.java

@@ -25,7 +25,6 @@
 import redis.clients.jedis.Jedis;
 
 import org.apache.geode.redis.GeodeRedisServerRule;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
 
 public class ExpireIntegrationTest {
 
@@ -51,7 +50,7 @@ public static void classLevelTearDown() {
   }
 
   @Test
-  public void Should_SetExipration_givenKeyTo_StringValue() {
+  public void Should_SetExpiration_givenKeyTo_StringValue() {
 
     String key = "key";
     String value = "value";
@@ -120,27 +119,6 @@ public void should_setExpiration_givenKeyTo_BitMapValue() {
     assertThat(timeToLive).isGreaterThanOrEqualTo(15);
   }
 
-  @Test
-  public void should_removeKey_AfterExpirationPeriod() {
-    String key = "key";
-    String value = "value";
-    jedis.set(key, value);
-
-    jedis.pexpire(key, 10);
-
-    GeodeAwaitility.await().until(() -> jedis.get(key) == null);
-  }
-
-  @Test
-  public void should_removeSetKey_AfterExpirationPeriod() {
-    String key = "key";
-    String value = "value";
-    jedis.sadd(key, value);
-
-    jedis.pexpire(key, 10);
-    GeodeAwaitility.await().until(() -> !jedis.exists(key));
-  }
-
   @Test
   public void settingAnExistingKeyToANewValue_ShouldClearExpirationTime() {
 
@@ -356,7 +334,7 @@ public void SettingExpirationToNegativeValue_ShouldDeleteKey() {
 
 
   @Test
-  public void CallingExpireOnAKeyThatAlreadyHasAnExiprationTime_ShouldUpdateTheExpirationTime() {
+  public void CallingExpireOnAKeyThatAlreadyHasAnExpirationTime_ShouldUpdateTheExpirationTime() {
     String key = "key";
     String value = "value";
     jedis.set(key, value);
@@ -367,12 +345,4 @@ public void CallingExpireOnAKeyThatAlreadyHasAnExiprationTime_ShouldUpdateTheExp
     Long timeToLive = jedis.ttl(key);
     assertThat(timeToLive).isGreaterThan(21);
   }
-
-  @Test
-  public void should_passivelyExpireKeys() {
-    jedis.sadd("key", "value");
-    jedis.pexpire("key", 100);
-
-    GeodeAwaitility.await().until(() -> jedis.keys("key").isEmpty());
-  }
 }

geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/PexpireIntegrationTest.java

@@ -17,13 +17,15 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import org.junit.After;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Test;
 import redis.clients.jedis.Jedis;
 
 import org.apache.geode.redis.GeodeRedisServerRule;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
 
 public class PexpireIntegrationTest {
 
@@ -38,6 +40,11 @@ public static void setUp() {
     jedis = new Jedis("localhost", server.getPort(), REDIS_CLIENT_TIMEOUT);
   }
 
+  @After
+  public void flushAll() {
+    jedis.flushAll();
+  }
+
   @AfterClass
   public static void classLevelTearDown() {
     jedis.close();
@@ -60,4 +67,33 @@ public void should_SetExpiration_givenKeyTo_StringValueInMilliSeconds() {
     assertThat(timeToLive).isLessThanOrEqualTo(20);
     assertThat(timeToLive).isGreaterThanOrEqualTo(15);
   }
+
+  @Test
+  public void should_removeKey_AfterExpirationPeriod() {
+    String key = "key";
+    String value = "value";
+    jedis.set(key, value);
+
+    jedis.pexpire(key, 10);
+
+    GeodeAwaitility.await().until(() -> jedis.get(key) == null);
+  }
+
+  @Test
+  public void should_removeSetKey_AfterExpirationPeriod() {
+    String key = "key";
+    String value = "value";
+    jedis.sadd(key, value);
+
+    jedis.pexpire(key, 10);
+    GeodeAwaitility.await().until(() -> !jedis.exists(key));
+  }
+
+  @Test
+  public void should_passivelyExpireKeys() {
+    jedis.sadd("key", "value");
+    jedis.pexpire("key", 100);
+
+    GeodeAwaitility.await().until(() -> jedis.keys("key").isEmpty());
+  }
 }

geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java

@@ -49,7 +49,6 @@
 import org.apache.logging.log4j.Logger;
 
 import org.apache.geode.annotations.Experimental;
-import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.annotations.internal.MakeNotStatic;
 import org.apache.geode.cache.Cache;
 import org.apache.geode.cache.CacheClosedException;
@@ -342,7 +341,6 @@ private void startGemFire() {
     this.cache = cache;
   }
 
-  @VisibleForTesting
   public RegionProvider getRegionProvider() {
     return regionProvider;
   }
@@ -355,7 +353,6 @@ public EventLoopGroup getSubscriberGroup() {
     return subscriberGroup;
   }
 
-
   private void initializeRedis() {
     synchronized (cache) {
 

geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisService.java

@@ -17,7 +17,6 @@
 import org.apache.logging.log4j.Logger;
 
 import org.apache.geode.cache.Cache;
-import org.apache.geode.distributed.internal.DistributionConfig;
 import org.apache.geode.distributed.internal.InternalDistributedSystem;
 import org.apache.geode.distributed.internal.ResourceEvent;
 import org.apache.geode.distributed.internal.ResourceEventsListener;
@@ -73,15 +72,11 @@ private void startRedisServer(InternalCache cache) {
       int port = system.getConfig().getRedisPort();
       String bindAddress = system.getConfig().getRedisBindAddress();
       assert bindAddress != null;
-      if (bindAddress.equals(DistributionConfig.DEFAULT_REDIS_BIND_ADDRESS)) {
-        logger.info(
-            String.format("Starting GeodeRedisServer on port %s",
-                new Object[] {port}));
-      } else {
-        logger.info(
-            String.format("Starting GeodeRedisServer on bind address %s on port %s",
-                new Object[] {bindAddress, port}));
-      }
+
+      logger.info(
+          String.format("Starting GeodeRedisServer on bind address %s on port %s",
+              new Object[] {bindAddress, port}));
+
       this.redisServer = new GeodeRedisServer(bindAddress, port);
       this.redisServer.start();
     }

geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java

@@ -122,7 +122,7 @@ public enum RedisCommandType {
 
   /*************** Connection ****************/
   AUTH(new AuthExecutor(), SUPPORTED, new ExactParameterRequirements(2)),
-  PING(new PingExecutor(), SUPPORTED),
+  PING(new PingExecutor(), SUPPORTED, new MaximumParameterRequirements(2)),
   QUIT(new QuitExecutor(), SUPPORTED),
 
   /*************** Keys ******************/