Add test function MockTimeEnv.SleepForMicroseconds() #7293
Conversation
jay-zhuang
changed the title
jay-zhuang
force-pushed
the
mock_time
branch
2 times, most recently
from
f8fd334
to
95f8538
Compare
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -38,8 +40,7 @@ TEST_F(StatsDumpSchedulerTest, Basic) { | |||
| options.stats_dump_period_sec = kPeriodSec; | |||
| options.stats_persist_period_sec = kPeriodSec; | |||
| options.create_if_missing = true; | |||
| int mock_time_sec = 0; | |||
| mock_env_->set_current_time(mock_time_sec); | |||
| mock_env_->set_current_time(0); | |||
There was a problem hiding this comment.
Should not be necessary, right? From #7101,
Removed unnecessary resetting of mock times to 0 in many tests. RocksDB
deals in relative time. Any behaviors relying on absolute date/time are likely
a bug. (Theabovetest DBSSTTest.GetTotalSstFilesSize was the only one
clearly injecting a specific absolute time for actual testing convenience.) Just
in case I misunderstood some test, I put this note in each replacement:
// NOTE: Presumed unnecessary and removed: resetting mock time in env
(And more below)
monitoring/stats_history_test.cc
Outdated
| @@ -48,15 +48,16 @@ class StatsHistoryTest : public DBTestBase { | |||
| StatsDumpTestScheduler::Default(mock_env_.get()); | |||
| }); | |||
| } | |||
|
|
|||
| const int kSecond = 1000000; | |||
There was a problem hiding this comment.
Even though I originally named this 🤪, I now recommend kUsPerSec
util/timer_test.cc
Outdated
| timer.TEST_WaitForRun([&] { mock_env_->set_current_time(mock_time_sec); }); | ||
| uint64_t next_abs_interval_time_us = kInitDelayUs + kRepeatUs; | ||
| timer.TEST_WaitForRun([&] { | ||
| mock_env_->set_current_time(next_abs_interval_time_us / 1000000); |
There was a problem hiding this comment.
Recommend / kSecond (or renamed)
util/timer_test.cc
Outdated
|
|
||
| int mock_time_sec = 0; | ||
| mock_env_->set_current_time(mock_time_sec); | ||
| mock_env_->set_current_time(0); |
There was a problem hiding this comment.
OK to keep this one if it keeps this test simpler :)
| dbfull()->TEST_WaitForStatsDumpRun( | ||
| [&] { mock_env_->set_current_time(mock_time_sec); }); | ||
| [&] { mock_env_->SleepForMicroseconds(kPeriodSec * kSecond); }); |
There was a problem hiding this comment.
If we're still mixing seconds and microseconds in tests, we might as well add SleepForSeconds also (like SpecialEnv) to minimize conversions.
And change the internal time value from seconds to microseconds.
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@jay-zhuang merged this pull request in 187964a. |
Summary: And change the internal time value from seconds to microseconds. Pull Request resolved: facebook#7293 Reviewed By: pdillinger Differential Revision: D23253751 Pulled By: jay-zhuang fbshipit-source-id: 36aa9376b8801b85bd10163173590a17cf4f3a3a
And change the internal time value from seconds to microseconds.