Feat fixed unit basic #1640

wang1309 · 2023-06-23T02:44:43Z

修复单测 basic 类型的错误，这个pr修复如下几个错误
[err]: DBSIZE in tests/unit/basic.tcl
[err]: DBSIZE should be 10101 now in tests/unit/basic.tcl
[err]: INCRBYFLOAT does not allow NaN or Infinity in tests/unit/basic.tcl

cr-gpt · 2023-06-23T02:44:48Z

.gitignore

@@ -71,3 +71,4 @@ deps
 # include codis fe javascript lib files
 !codis/cmd/fe/assets/**

+tests/tmp


The provided code patch only adds a new line to the file, which includes the path "tests/tmp". It is difficult to provide any insights regarding potential bugs or improvements without more context about the purpose and functionality of this file.

cr-gpt · 2023-06-23T02:44:55Z

src/pika_admin.cc

@@ -2032,6 +2032,7 @@ void DbsizeCmd::Do(std::shared_ptr<Slot> slot) {
 if (!db) {
 res_.SetRes(CmdRes::kInvalidDB);
 } else {
+ db->RunKeyScan();
 KeyScanInfo key_scan_info = db->GetKeyScanInfo();
 std::vector<storage::KeyInfo> key_infos = key_scan_info.key_infos;
 if (key_infos.size() != 5) {


The code patch seems to add a call to db->RunKeyScan() before retrieving the key scan information using db->GetKeyScanInfo(). It's hard to tell if this is appropriate without seeing the implementation of these methods, but assuming they are implemented correctly with proper error handling, this change should not introduce any bugs.

As for improvement suggestions, it would be good to replace the hard-coded value 5 with a constant or variable that has a meaningful name and is defined elsewhere in the code, so that it is easier to change in the future if necessary. Additionally, if key_infos is only being used to check its size, you could use key_scan_info.key_count instead of creating a vector just to get the size. Finally, it might be worth considering adding some comments to explain the purpose of db->RunKeyScan(), since it is not obvious from the name what it does.

cr-gpt · 2023-06-23T02:44:59Z

tests/unit/basic.tcl

@@ -211,7 +211,7 @@ start_server {tags {"basic"}} {
 # p.s. no way I can force NaN to test it from the API because
 # there is no way to increment / decrement by infinity nor to
 # perform divisions.
- } {ERR*would produce*}
+ } {ERR*increment or decrement would overflow*}

 test {INCRBYFLOAT decrement} {
 r set foo 1


The code patch only includes one modification, changing an error message string from "ERRwould produce" to "ERRincrement or decrement would overflow". This change is reasonable and seems to be well-focused on a specific issue.

However, it's hard to tell whether there are other bugs or improvement suggestions without more context about the surrounding code.

machinly · 2023-06-23T04:53:08Z

tests/unit/basic.tcl

非差异化命令的错误信息应与 redis 对齐，因为客户端可能会依赖错误信息进行进一步的处理。
可以修改 pika 的错误信息，而不是单测。

…est case warning info

cr-gpt · 2023-06-24T07:06:16Z

.gitignore

@@ -71,3 +71,4 @@ deps
 # include codis fe javascript lib files
 !codis/cmd/fe/assets/**

+tests/tmp


It looks like a simple Git patch that contains two changes: adding a file pattern to include in the deps section and adding a directory pattern to exclude.

The new file pattern !codis/cmd/fe/assets/** will be included in the deps section, which means that the files matching this pattern will be added as dependencies and included in the build process. As long as this is intended and necessary for the project, there's no bug risk or improvement suggestion to make.

The new directory pattern tests/tmp will be excluded from the build process. It's hard to say if this is the desired behavior without more context, but assuming that tests/tmp is a temporary directory used during testing, it does make sense to exclude it. Again, as long as this is intended and necessary for the project, there's no bug risk or improvement suggestion to make.

cr-gpt · 2023-06-24T07:06:20Z

include/pika_command.h

+ case KIncrByOverFlow:
+ result = "-ERR increment would produce NaN or Infinity";
+ result.append(message_);
+ result.append(kNewLine);
 default:
 break;
 }


There seems to be an issue with the case statement in the CmdRes class:

There is a missing "break" statement after the "KIncrByOverFlow" case. This could cause unintended behavior if this case falls through to the next case.

Additionally, it is difficult to provide improvement suggestions or assess potential bug risk without further context about the usage and purpose of the code.

cr-gpt · 2023-06-24T07:06:25Z

src/pika_kv.cc

@@ -250,7 +250,7 @@ void IncrbyfloatCmd::Do(std::shared_ptr<Slot> slot) {
 } else if (s.IsCorruption() && s.ToString() == "Corruption: Value is not a vaild float") {
 res_.SetRes(CmdRes::kInvalidFloat);
 } else if (s.IsInvalidArgument()) {
- res_.SetRes(CmdRes::kOverFlow);
+ res_.SetRes(CmdRes::KIncrByOverFlow);
 } else {
 res_.SetRes(CmdRes::kErrOther, s.ToString());
 }


The code patch seems to be a simple update of an error response value. Instead of returning CmdRes::kOverflow, it returns CmdRes::KIncrByOverFlow. Without seeing the original code, it's hard to say whether this is a necessary change or if it introduces any risk. However, assuming that the previous command response was overloaded, it seems like a reasonable change.

One suggestion for improvement would be to add some comments to the code. Specifically, it might be helpful to explain why the error response was changed and what specific conditions led to that change. Additionally, any relevant coding standards or best practices could be noted in the comments, as well as any potential future improvements that could be made to the code.

machinly · 2023-06-24T16:08:28Z

include/pika_command.h

+ case KIncrByOverFlow:
+ result = "-ERR increment would produce NaN or Infinity";
+ result.append(message_);
+ result.append(kNewLine);


case 语句缺少 break

machinly · 2023-06-24T16:58:48Z

src/pika_admin.cc

@@ -2032,6 +2032,7 @@ void DbsizeCmd::Do(std::shared_ptr<Slot> slot) {
 if (!db) {
 res_.SetRes(CmdRes::kInvalidDB);
 } else {
+ db->RunKeyScan();


pika 设计并不支持实时统计，因为扫描有锁且耗时，最好还是在后台起线程做。可以参考 docs/ops/bestPractice.md pika最佳实践之十六.

针对测试修复，可以在 tcl 脚本的 dbsize 前添加 r info keyspace 1 来触发 keyscan。
另外由于 keyscan 为异步操作，可能触发后并不会立即完成，可以在r dbsize 前添加 after 1000 来 +1s。
更优雅的做法是实现一个函数来等待扫描结束，函数内一直循环直到 is_scaning_keyspace 为 No，可以参考 tests/support/util.tcl 中的 proc waitForBgsave，不过测试数据量不大，after即可。

cr-gpt · 2023-06-24T23:15:26Z

include/pika_command.h

+ result = "-ERR increment would produce NaN or Infinity";
+ result.append(message_);
+ result.append(kNewLine);
+ break;
 default:
 break;
 }


The code patch seems to be adding a new error enum value for KIncrByOverFlow and handling it appropriately in the CmdRes::replyErrorMsg() method. It looks like a possible improvement could be to follow consistent naming conventions, as the new enum value starts with a capital letter while the rest use lowercase. Additionally, it may be useful to ensure that the specific conditions under which KIncrByOverFlow is returned are well-documented and easily understandable.

Without knowing more about the context of the code, it's difficult to say whether there are any bug risks or further improvements that could be made, but overall the changes themselves seem reasonable.

cr-gpt · 2023-06-24T23:15:34Z

tests/unit/basic.tcl

+ # after 1100
+ # assert_equal 0 [r del keyExpire]
+ # r debug set-active-expire 1
+ # }

 test {EXISTS} {
 set res {}


The code patch seems to be a set of tests for Redis server functionality. Here are some brief comments on the patch:

In start_server procedure, it might be helpful to validate if the server has started successfully and handle potential errors.

In DEL all keys block, calling r info keyspace 1 before r del $key may improve performance, as it avoids calling info after every single deletion, which can be time-consuming in large keyspaces.

Similarly, in SET with EXPIRE, calling r info keyspace 1 before r dbsize may also improve performance, as it minimizes the number of info calls.

The commented out test for DEL against expired key should either be removed or uncommented after confirming that it is no longer required.

Finally, the last test for EXISTS does not seem to have assertions or expectations defined, so it might need to be reviewed or updated.

Overall, the code patch seems to be well-structured and straightforward. More detailed analysis would require additional context on the server's behavior, data model, and available resources.

yaoyinnan

LGTM

* fixed: dbsize ret 0 * fixed: modify INCRBYFLOAT error prompt info * fixed: comment debug unit test case and modify incrbyfloat overflow test case warning info * fixed: dbsize get zero fix

wang1309 added 2 commits June 23, 2023 10:21

fixed: dbsize ret 0

49b7341

fixed: modify INCRBYFLOAT error prompt info

da5e241

cr-gpt bot reviewed Jun 23, 2023

View reviewed changes

wang1309 mentioned this pull request Jun 23, 2023

unit basic 问题汇总 #1641

Closed

machinly mentioned this pull request Jun 23, 2023

Unit Test To-Be-Fixed List | 单元测试待修复列表 #1605

Closed

machinly reviewed Jun 23, 2023

View reviewed changes

fixed: comment debug unit test case and modify incrbyfloat overflow t…

9c6e423

…est case warning info

cr-gpt bot reviewed Jun 24, 2023

View reviewed changes

machinly requested changes Jun 24, 2023

View reviewed changes

fixed: dbsize get zero fix

4901cfa

cr-gpt bot reviewed Jun 24, 2023

View reviewed changes

machinly approved these changes Jun 25, 2023

View reviewed changes

machinly requested review from chenbt-hz and yaoyinnan June 25, 2023 03:52

AlexStocks approved these changes Jun 25, 2023

View reviewed changes

yaoyinnan approved these changes Jun 25, 2023

View reviewed changes

AlexStocks merged commit 1559b55 into OpenAtomFoundation:unstable Jun 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat fixed unit basic #1640

Feat fixed unit basic #1640

wang1309 commented Jun 23, 2023

cr-gpt bot Jun 23, 2023

cr-gpt bot Jun 23, 2023

cr-gpt bot Jun 23, 2023

machinly Jun 23, 2023

wang1309 Jun 24, 2023

cr-gpt bot Jun 24, 2023

cr-gpt bot Jun 24, 2023

cr-gpt bot Jun 24, 2023

machinly Jun 24, 2023

wang1309 Jun 24, 2023

wang1309 Jun 24, 2023

machinly Jun 24, 2023

wang1309 Jun 24, 2023

wang1309 Jun 24, 2023

cr-gpt bot Jun 24, 2023

cr-gpt bot Jun 24, 2023

yaoyinnan left a comment

Feat fixed unit basic #1640

Feat fixed unit basic #1640

Conversation

wang1309 commented Jun 23, 2023

cr-gpt bot Jun 23, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 23, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr-gpt bot Jun 24, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 24, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr-gpt bot Jun 24, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 24, 2023

Choose a reason for hiding this comment

yaoyinnan left a comment

Choose a reason for hiding this comment