DonalEvans commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r807177685
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -293,8 +293,8 @@ public void
numsub_shouldReturnSubscriberCount_whenCalledWithPatternAndSubscribe
@Test
public void numpat_shouldError_givenTooManyArguments() {
assertAtMostNArgsForSubCommand(introspector,
- Protocol.Command.PUBSUB,
- PUBSUB_NUM_PAT.getBytes(),
+ PUBSUB,
+ Protocol.Keyword.NUMPAT.toString().getBytes(),
Review comment:
This can be simplified to `NUMPAT.getRaw()`
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/ShutdownIntegrationTest.java
##########
@@ -53,10 +56,13 @@ public int getPort() {
@Test
public void shutdownIsDisabled_whenOnlySupportedCommandsAreAllowed() {
- // Unfortunately Jedis' shutdown() doesn't seem to throw a
JedisDataException when the command
- // returns an error.
- jedis.shutdown();
+ final String EXPECTED_ERROR_MSG =
+ String.format(ERROR_UNKNOWN_COMMAND, "SHUTDOWN", "");
+ assertThatThrownBy(
+ () -> jedis.shutdown())
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining(EXPECTED_ERROR_MSG);
Review comment:
Could this assertion be changed to `hasMessage()` to make it more
explicit about the exact message we expect to see here please?
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -253,7 +255,7 @@ public void
shouldOverwriteDestinationKey_givenDestinationExists() {
jedis.zadd(KEY1, 1.0, "key1Member");
assertThat(jedis.zinterstore(NEW_SET, KEY1)).isEqualTo(1L);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0L, 1L);
+ final List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0L, 1L);
assertThat(results).containsExactlyInAnyOrderElementsOf(expectedResults);
Review comment:
With the change of the return type from Set to List, these assertions
should be changed to `containsExactly()` since we expect that sorted sets
preserve their ordering correctly in these tests.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -234,20 +235,20 @@ public void
shouldUnionize_givenBoundaryScoresAndWeights() {
jedis.zunionstore(NEW_SET, new ZParams().weights(1), KEY1);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
- assertThat(results).containsExactlyElementsOf(expectedResults);
+ List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
+ assertThat(results).containsExactlyInAnyOrderElementsOf(expectedResults);
Review comment:
These tests should also be using a List for their `expectedResults`
rather than a set, since that's what Jedis returns now. This is another case
where simple find/replace can make the conversion fairly painless.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -797,7 +799,7 @@ Tuple tupleMinOfScoresWithWeights(String memberName,
Map<String, Double> scores1
@Test
public void test_assertThatActualScoresAreVeryCloseToExpectedScores() {
- Set<Tuple> actualResult = new HashSet<>(3);
+ List<Tuple> actualResult = new ArrayList<>(3);
Set<Tuple> expectedResult = new HashSet<>(2);
Review comment:
If the returned type from Jedis has changed from a Set to a List, then
these tests should create their expected result as a List too. Luckily, in this
class, this is a simple matter of doing a find/replace for `Set<Tuple>` ->
`List<Tuple>` and `HashSet` and `LinkedHashSet` -> `ArrayList`.
##########
File path:
geode-for-redis/src/commonTest/java/org/apache/geode/redis/mocks/MockSubscriber.java
##########
@@ -133,16 +144,6 @@ public void onPong(String pattern) {
receivedPings.add(pattern);
}
- // JedisPubSub ping with message is not currently possible, will submit a PR
- // (https://github.com/xetorthio/jedis/issues/2049)
- public void ping(String message) {
Review comment:
This method was previously being called in
`AbstractSubscriptionsIntegrationTest` which has a comment referencing the
issue of not being able to ping with a message. Since this issue has been
resolved in Jedis 4.1.1, the comment in that test can be removed along with the
extra call to `ping()`.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/string/StringsKillMultipleServersDUnitTest.java
##########
@@ -56,11 +56,16 @@ public static void classSetup() {
int redisServerPort1 = cluster.getRedisPort(1);
jedisCluster =
- new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1),
10_000);
+ new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1),
10_000, 20);
// This sequence ensures that servers 1, 2 and 3 are hosting all the
buckets and server 4
// has no buckets.
cluster.startRedisVM(4, locatorPort);
+
+ cluster.enableDebugLogging(1);
+ cluster.enableDebugLogging(2);
+ cluster.enableDebugLogging(3);
+ cluster.enableDebugLogging(4);
Review comment:
This should probably be removed.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
