DonalEvans commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r761366823
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -60,10 +61,10 @@ public RedisResponse executeCommand(Command command,
ExecutionHandlerContext con
* refactor so it implements doSetOp
*/
+ // Make a static method then calculate diff add all create the resulting
set
Review comment:
This comment should probably be removed.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -61,53 +63,83 @@ public void sdiffErrors_givenTooFewArguments() {
@Test
public void sdiff_returnsAllValuesInSet() {
String[] values = createKeyValuesSet();
- assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ assertThat(jedis.sdiff(setKey)).containsExactlyInAnyOrder(values);
}
@Test
public void sdiffWithNonExistentSet_returnsEmptySet() {
- assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ assertThat(jedis.sdiff(nonExistentSetKey)).isEmpty();
}
@Test
public void sdiffWithMultipleNonExistentSet_returnsEmptySet() {
assertThat(jedis.sdiff("{user1}nonExistentSet1",
"{user1}nonExistentSet2")).isEmpty();
- assertThat(jedis.smembers("{user1}nonExistentSet1").isEmpty());
- assertThat(jedis.smembers("{user1}nonExistentSet2").isEmpty());
+ }
+
+ @Test
+ public void sdiffWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff(nonExistentSetKey, setKey)).isEmpty();
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(values);
Review comment:
These smembers assertions are unnecessary, I think. This test isn't
trying to assert that the contents of the sets are unchanged, just that we get
the expected return value from SDIFF. If we want to have a test that SDIFF
doesn't modify the sets its called on, that should be a separate test case.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -61,53 +63,83 @@ public void sdiffErrors_givenTooFewArguments() {
@Test
public void sdiff_returnsAllValuesInSet() {
String[] values = createKeyValuesSet();
- assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ assertThat(jedis.sdiff(setKey)).containsExactlyInAnyOrder(values);
}
@Test
public void sdiffWithNonExistentSet_returnsEmptySet() {
- assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ assertThat(jedis.sdiff(nonExistentSetKey)).isEmpty();
}
@Test
public void sdiffWithMultipleNonExistentSet_returnsEmptySet() {
assertThat(jedis.sdiff("{user1}nonExistentSet1",
"{user1}nonExistentSet2")).isEmpty();
- assertThat(jedis.smembers("{user1}nonExistentSet1").isEmpty());
- assertThat(jedis.smembers("{user1}nonExistentSet2").isEmpty());
+ }
+
+ @Test
+ public void sdiffWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff(nonExistentSetKey, setKey)).isEmpty();
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(values);
}
@Test
public void sdiffWithSetAndNonExistentSet_returnsAllValuesInSet() {
String[] values = createKeyValuesSet();
- assertThat(jedis.sdiff("{user1}setkey", "{user1}nonExistentSet"))
+ assertThat(jedis.sdiff(setKey, nonExistentSetKey))
.containsExactlyInAnyOrder(values);
-
assertThat(jedis.smembers("{user1}setkey")).containsExactlyInAnyOrder(values);
- assertThat(jedis.smembers("{user1}nonExistentSet")).isEmpty();
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(values);
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
Review comment:
These smembers asserts are also unnecessary, I think.
--
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]
