[
https://issues.apache.org/jira/browse/GEODE-8865?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17270209#comment-17270209
]
ASF GitHub Bot commented on GEODE-8865:
---------------------------------------
sabbey37 commented on a change in pull request #5945:
URL: https://github.com/apache/geode/pull/5945#discussion_r562700659
##########
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
##########
@@ -133,34 +149,54 @@ public void testHSet() {
}
@Test
- public void testHMGetHDelHGetAllHVals() {
+ public void testHMGet_HDel_HGetAll_HVals() {
String key = "key";
- Map<String, String> hash = new HashMap<String, String>();
+ Map<String, String> hash = new HashMap<>();
for (int i = 0; i < 10; i++) {
hash.put("field_" + i, "member_" + i);
}
jedis.hmset(key, hash);
+
Set<String> keys = hash.keySet();
String[] keyArray = keys.toArray(new String[keys.size()]);
List<String> retList = jedis.hmget(key, keyArray);
- for (int i = 0; i < keys.size(); i++) {
- assertThat(hash.get(keyArray[i])).isEqualTo(retList.get(i));
- }
+ assertThat(retList).containsExactlyInAnyOrderElementsOf(hash.values());
Map<String, String> retMap = jedis.hgetAll(key);
assertThat(retMap).containsExactlyInAnyOrderEntriesOf(hash);
List<String> retVals = jedis.hvals(key);
- Set<String> retSet = new HashSet<String>(retVals);
+ Set<String> retSet = new HashSet<>(retVals);
assertThat(retSet.containsAll(hash.values())).isTrue();
jedis.hdel(key, keyArray);
assertThat(jedis.hlen(key)).isEqualTo(0);
}
+ @Test
+ public void testHMGet_givenWrongNumberOfArguments() {
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMGET))
+ .hasMessageContaining("wrong number of arguments");
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMGET, "1"))
+ .hasMessageContaining("wrong number of arguments");
+ }
+
+ @Test
+ public void testHMGetErrorMessage_givenIncorrectDataType() {
+ jedis.set("farm", "chicken");
+ assertThatThrownBy(() -> jedis.hmget("farm", "chicken"))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the
wrong kind of value");
+
+ jedis.sadd("zoo", "elephant");
+ assertThatThrownBy(() -> jedis.hmget("zoo", "chicken"))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the
wrong kind of value");
Review comment:
Same comment from earlier in the file: you could actually change this to
.hasMessage instead of .hasMessageContaining in case anything ever gets
appended to the beginning or end. We could also use the constant from
RedisConstants here:
.hasMessage("WRONGTYPE " + ERROR_WRONG_TYPE)
so if the message ever changed for some reason it would be less work to
update it throughout all the tests
##########
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
##########
@@ -111,6 +111,22 @@ public void testHMSet() {
assertThat(jedis.hlen(key)).isEqualTo(hash.size());
}
+ @Test
+ public void testHMSetErrorMessage_givenIncorrectDataType() {
+ Map<String, String> animalMap = new HashMap<>();
+ animalMap.put("chicken", "eggs");
+
+ jedis.set("farm", "chicken");
+ assertThatThrownBy(() -> jedis.hmset("farm", animalMap))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the
wrong kind of value");
+
+ jedis.sadd("zoo", "elephant");
+ assertThatThrownBy(() -> jedis.hmset("zoo", animalMap))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the
wrong kind of value");
Review comment:
Just realized you could actually change this to `.hasMessage` instead of
`.hasMessageContaining` in case anything ever gets appended to the beginning or
end. We could also use the constant from `RedisConstants` here:
```
.hasMessage("WRONGTYPE " + ERROR_WRONG_TYPE)
```
so if the message ever changed for some reason it would be less work to
update it throughout all the tests.
##########
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
##########
@@ -133,34 +149,54 @@ public void testHSet() {
}
@Test
- public void testHMGetHDelHGetAllHVals() {
+ public void testHMGet_HDel_HGetAll_HVals() {
String key = "key";
- Map<String, String> hash = new HashMap<String, String>();
+ Map<String, String> hash = new HashMap<>();
for (int i = 0; i < 10; i++) {
hash.put("field_" + i, "member_" + i);
}
jedis.hmset(key, hash);
+
Set<String> keys = hash.keySet();
String[] keyArray = keys.toArray(new String[keys.size()]);
List<String> retList = jedis.hmget(key, keyArray);
- for (int i = 0; i < keys.size(); i++) {
- assertThat(hash.get(keyArray[i])).isEqualTo(retList.get(i));
- }
+ assertThat(retList).containsExactlyInAnyOrderElementsOf(hash.values());
Map<String, String> retMap = jedis.hgetAll(key);
assertThat(retMap).containsExactlyInAnyOrderEntriesOf(hash);
List<String> retVals = jedis.hvals(key);
- Set<String> retSet = new HashSet<String>(retVals);
+ Set<String> retSet = new HashSet<>(retVals);
assertThat(retSet.containsAll(hash.values())).isTrue();
jedis.hdel(key, keyArray);
assertThat(jedis.hlen(key)).isEqualTo(0);
}
+ @Test
+ public void testHMGet_givenWrongNumberOfArguments() {
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMGET))
+ .hasMessageContaining("wrong number of arguments");
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMGET, "1"))
+ .hasMessageContaining("wrong number of arguments");
Review comment:
For most of the other commands (del, getset, exists, etc.) we check the
error message more closely, like:
```
.hasMessage("ERR wrong number of arguments for 'get' command");
```
I'm not sure if it's necessary, but maybe we could do the same here to be
consistent? I realize there are a few areas where we're still just checking
`wrong number of arguments`. We could eventually change those in a different PR.
----------------------------------------------------------------
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]
> Create additional dunit and integration tests for Redis HMGET
> -------------------------------------------------------------
>
> Key: GEODE-8865
> URL: https://issues.apache.org/jira/browse/GEODE-8865
> Project: Geode
> Issue Type: Test
> Components: redis
> Reporter: Jens Deppe
> Priority: Major
> Labels: pull-request-available
>
> Write integration tests for the following command:
> * HMGET
> *A.C.*
> * NativeRedisAcceptanceTest file present to run our tests against native
> Redis
> * Tests are passing, _or_
> * Stories in the backlog to fix the identified issues (with JIRA tickets)
> and problem tests ignored
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
