nonbinaryprogrammer commented on a change in pull request #6444:
URL: https://github.com/apache/geode/pull/6444#discussion_r628608822
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractGetRangeIntegrationTest.java
##########
@@ -59,36 +60,38 @@ public void errors_givenWrongNumberOfArguments() {
@Test
public void givenStartIndexIsNotAnInteger_returnsNotIntegerError() {
assertThatThrownBy(
- () -> jedis.sendCommand(Protocol.Command.GETRANGE, "key", "NaN", "5"))
+ () -> jedis.sendCommand("key", Protocol.Command.GETRANGE, "key",
"NaN", "5"))
Review comment:
might be worth extracting `"key"`
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrByFloatIntegrationTest.java
##########
@@ -102,50 +103,53 @@ public void
testCorrectErrorIsReturned_whenKeyIsAnIncorrectType() {
@Test
public void testCorrectErrorIsReturned_whenIncrByIsInvalid() {
+ String key = "number";
double number1 = 1.4;
- jedis.set("number", "" + number1);
+ jedis.set(key, "" + number1);
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.INCRBYFLOAT,
"number", " a b c"))
+ assertThatThrownBy(() -> jedis.sendCommand(key,
Protocol.Command.INCRBYFLOAT, key, " a b c"))
.hasMessage("ERR value is not a valid float");
}
@Test
public void testIncrByFloat_withInfinityAndVariants() {
- jedis.set("number", "1.4");
+ String key = "number";
+ jedis.set(key, "1.4");
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.INCRBYFLOAT,
"number", "+inf"))
+ assertThatThrownBy(() -> jedis.sendCommand(key,
Protocol.Command.INCRBYFLOAT, key, "+inf"))
.hasMessage("ERR increment would produce NaN or Infinity");
Review comment:
There is a constant for this error message:
`RedisConstants.ERROR_NAN_OR_INFINITY`
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractBitOpIntegrationTest.java
##########
@@ -53,64 +62,65 @@ public void bitOp_errors_givenTooFewArguments() {
@Test
public void bitop_givenInvalidOperationType_returnsSyntaxError() {
assertThatThrownBy(
- () -> jedis.sendCommand(Protocol.Command.BITOP, "invalidOp",
"destKey", "srcKey"))
- .hasMessageContaining(ERROR_SYNTAX);
+ () -> jedis.sendCommand(hashTag, Protocol.Command.BITOP, "invalidOp",
"destKey" + hashTag,
+ "srcKey" + hashTag)).hasMessageContaining(ERROR_SYNTAX);
}
@Test
public void bitop_givenSetFails() {
- jedis.sadd("foo", "m1");
- assertThatThrownBy(() -> jedis.bitop(BitOP.AND, "key", "foo"))
+ jedis.sadd(srcKey, "m1");
+ assertThatThrownBy(() -> jedis.bitop(BitOP.AND, destKey, srcKey))
.hasMessageContaining(ERROR_WRONG_TYPE);
- assertThatThrownBy(() -> jedis.bitop(BitOP.OR, "key", "foo"))
+ assertThatThrownBy(() -> jedis.bitop(BitOP.OR, destKey, srcKey))
.hasMessageContaining(ERROR_WRONG_TYPE);
- assertThatThrownBy(() -> jedis.bitop(BitOP.XOR, "key", "foo"))
+ assertThatThrownBy(() -> jedis.bitop(BitOP.XOR, destKey, srcKey))
.hasMessageContaining(ERROR_WRONG_TYPE);
- assertThatThrownBy(() -> jedis.bitop(BitOP.NOT, "key", "foo"))
+ assertThatThrownBy(() -> jedis.bitop(BitOP.NOT, destKey, srcKey))
.hasMessageContaining(ERROR_WRONG_TYPE);
}
@Test
public void bitopNOT_givenMoreThanOneSourceKey_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand(Protocol.Command.BITOP, "NOT", "destKey",
"srcKey", "srcKey2"))
- .hasMessageContaining(ERROR_BITOP_NOT);
+ () -> jedis.sendCommand(
+ hashTag, Protocol.Command.BITOP, "NOT", destKey, srcKey, "srcKey2"
+ hashTag))
+ .hasMessageContaining(ERROR_BITOP_NOT);
}
@Test
public void bitopNOT_givenNothingLeavesKeyUnset() {
- assertThat(jedis.bitop(BitOP.NOT, "key", "foo")).isEqualTo(0);
- assertThat(jedis.exists("key")).isFalse();
+ assertThat(jedis.bitop(BitOP.NOT, destKey, srcKey)).isEqualTo(0);
+ assertThat(jedis.exists(destKey)).isFalse();
}
@Test
public void bitopNOT_givenNothingDeletesKey() {
- jedis.set("key", "value");
- assertThat(jedis.bitop(BitOP.NOT, "key", "foo")).isEqualTo(0);
- assertThat(jedis.exists("key")).isFalse();
+ jedis.set(destKey, "value");
Review comment:
if you're making other changes, "value" could be extracted. it's used in
a few places
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractAppendIntegrationTest.java
##########
@@ -25,25 +25,27 @@
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
-import redis.clients.jedis.Jedis;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
public abstract class AbstractAppendIntegrationTest implements
RedisIntegrationTest {
- private Jedis jedis;
- private Jedis jedis2;
+ private JedisCluster jedis;
+ private static final int REDIS_CLIENT_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
Review comment:
this could be directly cast to an int
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractSetIntegrationTest.java
##########
@@ -513,49 +517,50 @@ public void
testSET_XX_NX_arguments_should_return_NULL_if_Not_Successful() {
@Test
public void testSET_withInvalidOptions() {
+ String key = "foo";
Review comment:
looks like we wouldn't need this if you had a key variable at the class
level
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrByFloatIntegrationTest.java
##########
@@ -102,50 +103,53 @@ public void
testCorrectErrorIsReturned_whenKeyIsAnIncorrectType() {
@Test
public void testCorrectErrorIsReturned_whenIncrByIsInvalid() {
+ String key = "number";
double number1 = 1.4;
- jedis.set("number", "" + number1);
+ jedis.set(key, "" + number1);
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.INCRBYFLOAT,
"number", " a b c"))
+ assertThatThrownBy(() -> jedis.sendCommand(key,
Protocol.Command.INCRBYFLOAT, key, " a b c"))
.hasMessage("ERR value is not a valid float");
Review comment:
There is a constant for this error message:
`RedisConstants.ERROR_NOT_A_VALID_FLOAT`
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractSetIntegrationTest.java
##########
@@ -42,90 +43,93 @@
public abstract class AbstractSetIntegrationTest implements
RedisIntegrationTest {
- private Jedis jedis;
- private Jedis jedis2;
+ private JedisCluster jedis;
private static final int ITERATION_COUNT = 4000;
private static final int REDIS_CLIENT_TIMEOUT =
Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
@Before
public void setUp() {
- jedis = new Jedis("localhost", getPort(), REDIS_CLIENT_TIMEOUT);
- jedis2 = new Jedis("localhost", getPort(), REDIS_CLIENT_TIMEOUT);
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()),
REDIS_CLIENT_TIMEOUT);
}
@After
public void tearDown() {
- jedis.flushAll();
+ flushAll();
jedis.close();
- jedis2.close();
}
@Test
public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET))
+ assertThatThrownBy(() -> jedis.sendCommand("any", Protocol.Command.SET))
.hasMessageContaining("ERR wrong number of arguments for 'set'
command");
}
@Test
public void givenValueNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key"))
+ assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SET,
"key"))
.hasMessageContaining("ERR wrong number of arguments for 'set'
command");
}
@Test
public void givenEXKeyword_withoutParameter_returnsSyntaxError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "EX"))
+ assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SET,
"key", "value", "EX"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
public void
givenEXKeyword_whenParameterIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "EX", "NaN"))
- .hasMessageContaining(ERROR_NOT_INTEGER);
+ assertThatThrownBy(
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"EX", "NaN"))
+ .hasMessageContaining(ERROR_NOT_INTEGER);
}
@Test
public void
givenEXKeyword_whenParameterIsZero_returnsInvalidExpireTimeError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "PX", "0"))
- .hasMessageContaining(ERROR_INVALID_EXPIRE_TIME);
+ assertThatThrownBy(
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"PX", "0"))
+ .hasMessageContaining(ERROR_INVALID_EXPIRE_TIME);
}
@Test
public void givenPXKeyword_withoutParameter_returnsSyntaxError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "PX"))
+ assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SET,
"key", "value", "PX"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
public void
givenPXKeyword_whenParameterIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "PX", "NaN"))
- .hasMessageContaining(ERROR_NOT_INTEGER);
+ assertThatThrownBy(
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"PX", "NaN"))
+ .hasMessageContaining(ERROR_NOT_INTEGER);
}
@Test
public void
givenPXKeyword_whenParameterIsZero_returnsInvalidExpireTimeError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "PX", "0"))
- .hasMessageContaining(ERROR_INVALID_EXPIRE_TIME);
+ assertThatThrownBy(
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"PX", "0"))
+ .hasMessageContaining(ERROR_INVALID_EXPIRE_TIME);
}
@Test
public void givenPXAndEXInSameCommand_returnsSyntaxError() {
assertThatThrownBy(
- () -> jedis.sendCommand(Protocol.Command.SET, "key", "value", "PX",
"3000", "EX", "3"))
- .hasMessageContaining(ERROR_SYNTAX);
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"PX", "3000", "EX",
+ "3"))
+ .hasMessageContaining(ERROR_SYNTAX);
}
@Test
public void givenNXAndXXInSameCommand_returnsSyntaxError() {
- assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.SET, "key",
"value", "NX", "XX"))
- .hasMessageContaining(ERROR_SYNTAX);
+ assertThatThrownBy(
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"NX", "XX"))
+ .hasMessageContaining(ERROR_SYNTAX);
}
@Test
public void givenInvalidKeyword_returnsSyntaxError() {
assertThatThrownBy(
- () -> jedis.sendCommand(Protocol.Command.SET, "key", "value",
"invalidKeyword"))
+ () -> jedis.sendCommand("key", Protocol.Command.SET, "key", "value",
"invalidKeyword"))
Review comment:
I think this class would be another good place to extract the `"key"`
strings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
