You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/01/21 22:19:14 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #5945: GEODE-8865: Create additional dunit and integration tests for Redis HMGET
jdeppe-pivotal opened a new pull request #5945:
URL: https://github.com/apache/geode/pull/5945
- Add additional integration test for HMSET.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #5945: GEODE-8865: Create additional dunit and integration tests for Redis HMGET
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #5945:
URL: https://github.com/apache/geode/pull/5945
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #5945: GEODE-8865: Create additional dunit and integration tests for Redis HMGET
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5945: GEODE-8865: Create additional dunit and integration tests for Redis HMGET
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5945:
URL: https://github.com/apache/geode/pull/5945#discussion_r562730365
##########
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:
I think I just did this in the HVALS 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:
users@infra.apache.org