[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5985: GEODE-8903: test hget and move to supported list

[GitBox <gi...@apache.org>]

jdeppe-pivotal commented on a change in pull request #5985:
URL: https://github.com/apache/geode/pull/5985#discussion_r568154282



##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
##########
@@ -504,6 +504,50 @@ public void hvalsFails_withIncorrectParameters() {
         .hasMessage("ERR wrong number of arguments for 'hvals' command");
   }
 
+  @Test
+  public void hget_shouldThrowError_givenWrongNumberOfArguments() {
+    assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HGET))
+        .hasMessageContaining("ERR wrong number of arguments for 'hget' command");
+    assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HGET, "1"))
+        .hasMessageContaining("ERR wrong number of arguments for 'hget' command");
+    assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HGET, "1", "2", "3"))
+        .hasMessageContaining("ERR wrong number of arguments for 'hget' command");
+  }
+
+  @Test
+  public void hgetFailsForNonHash() {
+    jedis.sadd("farm", "chicken");
+    assertThatThrownBy(() -> jedis.hget("farm", "chicken"))
+        .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+
+    jedis.set("tractor", "John Deere");
+    assertThatThrownBy(() -> jedis.hget("tractor", "John Deere"))
+        .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+  }
+
+  @Test
+  public void hgetReturnsExpectedValue() {
+    String key = "key";
+    String field = "field";
+    String value = "value";
+
+    jedis.hset(key, field, value);
+
+    assertThat(jedis.hget(key, field)).isEqualTo(value);
+  }
+
+  @Test
+  public void hgetReturnsNewValue_whileUpdatingValue() {

Review comment:
       I don't quite understand what this test is intending - the method name contains `while` which to me implies some kind of concurrency going on, but it's strictly a serial flow of operations. Could you change the name or even just remove the test.




----------------------------------------------------------------
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