Merge pull request #22655 from veblush/time-convert

Fix flaky ChannelzChannelTest::LastCallStartedTime test

src/core/lib/gpr/time.cc

@@ -254,6 +254,10 @@ gpr_timespec gpr_convert_clock_type(gpr_timespec t, gpr_clock_type clock_type) {
     return gpr_time_add(gpr_now(clock_type), t);
   }
 
+  // If the given input hits this code, the same result is not guaranteed for
+  // the same input because it relies on `gpr_now` to calculate the difference
+  // between two different clocks. Please be careful when you want to use this
+  // function in unit tests. (e.g. https://github.com/grpc/grpc/pull/22655)
   return gpr_time_add(gpr_now(clock_type),
                       gpr_time_sub(t, gpr_now(t.clock_type)));
 }

test/core/channel/channelz_test.cc

@@ -48,11 +48,11 @@ namespace testing {
 class CallCountingHelperPeer {
  public:
   explicit CallCountingHelperPeer(CallCountingHelper* node) : node_(node) {}
-  grpc_millis last_call_started_millis() const {
+
+  gpr_timespec last_call_started_time() const {
     CallCountingHelper::CounterData data;
     node_->CollectData(&data);
-    gpr_timespec ts = gpr_cycle_counter_to_time(data.last_call_started_cycle);
+    return gpr_cycle_counter_to_time(data.last_call_started_cycle);
-    return grpc_timespec_to_millis_round_up(ts);
   }
 
  private:
@@ -243,9 +243,9 @@ void ValidateServer(ServerNode* server, const ValidateChannelDataArgs& args) {
   gpr_free(core_api_json_str);
 }
 
-grpc_millis GetLastCallStartedMillis(CallCountingHelper* channel) {
+gpr_timespec GetLastCallStartedTime(CallCountingHelper* channel) {
   CallCountingHelperPeer peer(channel);
-  return peer.last_call_started_millis();
+  return peer.last_call_started_time();
 }
 
 void ChannelzSleep(int64_t sleep_us) {
@@ -301,28 +301,28 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) {
   ValidateChannel(channelz_channel, {3, 3, 3});
 }
 
-TEST_P(ChannelzChannelTest, LastCallStartedMillis) {
+TEST_P(ChannelzChannelTest, LastCallStartedTime) {
   grpc_core::ExecCtx exec_ctx;
   CallCountingHelper counter;
   // start a call to set the last call started timestamp
   counter.RecordCallStarted();
-  grpc_millis millis1 = GetLastCallStartedMillis(&counter);
+  gpr_timespec time1 = GetLastCallStartedTime(&counter);
   // time gone by should not affect the timestamp
   ChannelzSleep(100);
-  grpc_millis millis2 = GetLastCallStartedMillis(&counter);
+  gpr_timespec time2 = GetLastCallStartedTime(&counter);
-  EXPECT_EQ(millis1, millis2);
+  EXPECT_EQ(gpr_time_cmp(time1, time2), 0);
   // calls succeeded or failed should not affect the timestamp
   ChannelzSleep(100);
   counter.RecordCallFailed();
   counter.RecordCallSucceeded();
-  grpc_millis millis3 = GetLastCallStartedMillis(&counter);
+  gpr_timespec time3 = GetLastCallStartedTime(&counter);
-  EXPECT_EQ(millis1, millis3);
+  EXPECT_EQ(gpr_time_cmp(time1, time3), 0);
   // another call started should affect the timestamp
   // sleep for extra long to avoid flakes (since we cache Now())
   ChannelzSleep(5000);
   counter.RecordCallStarted();
-  grpc_millis millis4 = GetLastCallStartedMillis(&counter);
+  gpr_timespec time4 = GetLastCallStartedTime(&counter);
-  EXPECT_NE(millis1, millis4);
+  EXPECT_NE(gpr_time_cmp(time1, time4), 0);
 }
 
 class ChannelzRegistryBasedTest : public ::testing::TestWithParam<size_t> {