Enhance test for stream asio #528
Conversation
ZZhongge
commented
Aug 20, 2024
ZZhongge
commented
- client send requests to wrong endpoint
- client is closed after sending requests
- server timeout after sending requests
- server is closed after sending requests
1. client send requests to wrong endpoint 2. client is closed after sending requests 3. server timeout after sending requests 4. server is closed after sending requests
| : resp_log_index_(0) | ||
| , msg_mismatch_(false) | ||
| , reqs_out_of_order_(false) | ||
| , next_log_index_(1) |
| client_logger_ = cs_new<logger_wrapper>(client_log_file_name); | ||
| } | ||
|
|
||
| bool waiting_for_responses(int timeout_ms = 3000) { |
There was a problem hiding this comment.
I just realized that this needs to be renamed. Can you rename it to wait_for_responses? Also it will be better to modify it as follows:
int wait_for_responses(int timeout_ms = 3000) {
TestSuite::_msg("wait for responses (up to %d ms)\n", timeout_ms);
ea_.wait_ms(timeout_ms);
SimpleLogger* ll = client_logger_->getLogger();
_log_info(ll, "resp: %ld, message sent: %ld", next_log_index_.load(), num_messages_sent_);
CHK_EQ(next_log_index_.load(), num_messages_sent_ + 1);
return 0;
}and do
CHK_Z(wait_for_responses());| @@ -236,5 +418,13 @@ int main(int argc, char** argv) { | |||
|
|
|||
| ts.doTest("stream server happy path test", | |||
| stream_server_happy_path_test); | |||
| ts.doTest("client send msg to wrong endpoint test", | |||
| client_send_to_wrong_endpoint_test); | |||
| ts.doTest("cient close after sending test", | |||
| } | ||
|
|
||
| stream_stat_->req_ea_.invoke(); |
There was a problem hiding this comment.
Do you intend invoking req_ea_ at the first message? If so, put a comment at req_ea_ definition.
There was a problem hiding this comment.
And I also see rare test failure here
[ .... ] server timeout test
=== TEST MESSAGE (BEGIN) ===
sending req: 1000/1000 (100.0%)
wait for receiving requests (up to 1000 ms)
wait for responses (up to 3000 ms)
time: 2024-08-20 22:29:37.187010
thread: ea83
in: server_timeout_test()
at: /home/junahn/work/raft/tests/unit/asio_service_stream_test.cxx:370
value of: stat_ptr->waiting_for_responses()
expected: true
actual: false
[ FAIL ] server timeout test (3.1 s)
Haven't done detailed investigation, but probably be timing issue. You may need to either decrease send_req timeout (from 2s to 1s) or increase wait_for_response timeout (from 3s to 5s). Please take a look.
| class stream_msg_handler : public nuraft::msg_handler { | ||
| public: | ||
| stream_msg_handler(context* ctx, | ||
| const init_options& opt, | ||
| ptr<logger_wrapper> log_wrapper) | ||
| ptr<stream_statistic> stream_stat) |
There was a problem hiding this comment.
If stream_statistic is shared throughout the test, why don't you just define it as a global variable?
static stream_statistic stream_stat;And reset it at the beginning of each test, by implementing reset() API.
stream_stat.reset();Need to make sure ea_.reset() should be invoked before reusing it.
| @@ -29,57 +29,178 @@ using namespace raft_functional_common; | |||
| namespace asio_service_stream_test { | |||
| const std::string TEST_MSG = "stream-test-msg-str"; | |||
|
|
|||
| class stream_statistic { | |||
There was a problem hiding this comment.
I just realized that you put indent inside namespace. As you can see in other tests, no indent is needed. Please remove it.
| stream_stat_->error_req_count_++; | ||
| stream_stat_->next_log_index_++; | ||
| SimpleLogger* ll = stream_stat_->client_logger_->getLogger(); | ||
| _log_info(ll, "handle result err: %s", err->what()); |
There was a problem hiding this comment.
It will be better to put next_log_index_ and error_req_count_ in the log for easier debugging.
| open_logs(); | ||
| } | ||
|
|
||
| bool wait_for_responses(int timeout_ms = 3000) { |
There was a problem hiding this comment.
Return type should be int.
| // ts.doTest("stream server happy path test", | ||
| // stream_server_happy_path_test); | ||
| // ts.doTest("client send msg to wrong endpoint test", | ||
| // client_send_to_wrong_endpoint_test); | ||
| // ts.doTest("client close after sending test", | ||
| // client_close_after_sending_test); | ||
| ts.doTest("server timeout test", | ||
| server_timeout_test); | ||
| // ts.doTest("server close after sending test", | ||
| // server_close_after_sending_test); |
There was a problem hiding this comment.
You forgot to remove these comment marks.
