[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1017: Align the expire option behavior of redis

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

torwig commented on code in PR #1017:
URL: https://github.com/apache/incubator-kvrocks/pull/1017#discussion_r1000549329


##########
tests/gocase/unit/type/strings/strings_test.go:
##########
@@ -141,33 +141,107 @@ func TestString(t *testing.T) {
 		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5*time.Second, 10*time.Second)
 	})
 
+	t.Run("GETEX Duplicate EX option", func(t *testing.T) {
+		require.NoError(t, rdb.Del(ctx, "foo").Err())
+		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
+		require.NoError(t, rdb.GetEx(ctx, "foo", 10*time.Second).Err())
+		require.NoError(t, rdb.Do(ctx, "getex", "foo", "ex", 1, "ex", 10).Err())
+		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5*time.Second, 10*time.Second)
+	})
+
 	t.Run("GETEX PX option", func(t *testing.T) {
 		require.NoError(t, rdb.Del(ctx, "foo").Err())
 		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
 		require.NoError(t, rdb.GetEx(ctx, "foo", 10000*time.Millisecond).Err())
 		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5000*time.Millisecond, 10000*time.Millisecond)
 	})
 
+	t.Run("GETEX Duplicate PX option", func(t *testing.T) {
+		require.NoError(t, rdb.Del(ctx, "foo").Err())
+		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
+		require.NoError(t, rdb.Do(ctx, "getex", "foo", "px", 1, "px", 10000).Err())
+		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5000*time.Millisecond, 10000*time.Millisecond)
+	})
+
 	t.Run("GETEX EXAT option", func(t *testing.T) {
 		require.NoError(t, rdb.Del(ctx, "foo").Err())
 		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
 		require.NoError(t, rdb.Do(ctx, "getex", "foo", "exat", time.Now().Add(10*time.Second).Unix()).Err())
 		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5*time.Second, 10*time.Second)
 	})
 
+	t.Run("GETEX Duplicate EXAT option", func(t *testing.T) {
+		require.NoError(t, rdb.Del(ctx, "foo").Err())
+		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
+		require.NoError(t, rdb.Do(ctx, "getex", "foo", "exat", time.Now().Add(100*time.Second).Unix(), "exat", time.Now().Add(10*time.Second).Unix()).Err())
+		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5*time.Second, 10*time.Second)
+	})
+
 	t.Run("GETEX PXAT option", func(t *testing.T) {
 		require.NoError(t, rdb.Del(ctx, "foo").Err())
 		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
 		require.NoError(t, rdb.Do(ctx, "getex", "foo", "pxat", time.Now().Add(10000*time.Millisecond).UnixMilli()).Err())
 		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5000*time.Millisecond, 10000*time.Millisecond)
 	})
 
+	t.Run("GETEX Duplicate PXAT option", func(t *testing.T) {
+		require.NoError(t, rdb.Del(ctx, "foo").Err())
+		require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
+		require.NoError(t, rdb.Do(ctx, "getex", "foo", "pxat", time.Now().Add(1000000*time.Millisecond).UnixMilli(), "pxat", time.Now().Add(10000*time.Millisecond).UnixMilli()).Err())
+		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5000*time.Millisecond, 10000*time.Millisecond)
+	})
+
 	t.Run("GETEX PERSIST option", func(t *testing.T) {
 		require.NoError(t, rdb.Del(ctx, "foo").Err())
 		require.NoError(t, rdb.Set(ctx, "foo", "bar", 10*time.Second).Err())
 		util.BetweenValues(t, rdb.TTL(ctx, "foo").Val(), 5*time.Second, 10*time.Second)
 		require.NoError(t, rdb.Do(ctx, "getex", "foo", "persist").Err())
 		require.EqualValues(t, -1, rdb.TTL(ctx, "foo").Val())
+		require.ErrorContains(t, rdb.Do(ctx, "getex", "foo", "ex", 10, "persist").Err(), "syntax err")
+		require.ErrorContains(t, rdb.Do(ctx, "getex", "foo", "px", 10000, "persist").Err(), "syntax err")
+		require.ErrorContains(t, rdb.Do(ctx, "getex", "foo", "pxat", time.Now().Add(10000*time.Millisecond).UnixMilli(), "persist").Err(), "syntax err")
+		require.ErrorContains(t, rdb.Do(ctx, "getex", "foo", "exat", time.Now().Add(100*time.Second).Unix(), "persist").Err(), "syntax err")
+	})
+
+	t.Run("GETEX with incorrect use of multi options should result in syntax err", func(t *testing.T) {
+		options := []string{"px", "ex", "pxat", "exat"}
+		for i := 0; i < (1 << 4); i++ {

Review Comment:
   I think since it's not part of a hot path somewhere in the production code, it would be more beneficial and clear to use a more explicit integer value instead of a shift operation.
   The same could be applied to the statement `i&(1<<j)` in the condition clause a few lines below and inside another loop.



-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org