admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix RepeatingTask unittest flakiness

Browse Source 
Use a fake task queue to test the RepeatingTask rather than a real
task queue, which removes the need for Sleep(). This fixes the flakiness
issues as the class is no deterministic.

BUG=webrtc:12808

Change-Id: I8c6a8535165b076f5fe6ec3e65ebcf7f07008737
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/237803
Reviewed-by: Tommi <tommi@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35349}
This commit is contained in:
Evan Shrubsole 2021-11-15 17:24:45 +01:00 committed by WebRTC LUCI CQ
parent d3251968d1
commit f0f474373c
2 changed files with 113 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
rtc_base/task_utils/BUILD.gn
View File

@ -68,9 +68,13 @@ if (rtc_include_tests) {
sources = [ "repeating_task_unittest.cc" ]
deps = [
":repeating_task",
":to_queued_task",
"..:rtc_base_approved",
"..:rtc_task_queue",
"..:task_queue_for_test",
"../../api/task_queue",
"../../api/units:timestamp",
"../../system_wrappers:system_wrappers",
"../../test:test_support",
]
}

166
rtc_base/task_utils/repeating_task_unittest.cc
View File

@ -11,12 +11,15 @@
#include "rtc_base/task_utils/repeating_task.h"
#include <atomic>
#include <chrono> // Not allowed in production per Chromium style guide.
#include <memory>
#include <thread> // Not allowed in production per Chromium style guide.
#include "api/task_queue/queued_task.h"
#include "api/task_queue/task_queue_base.h"
#include "api/units/timestamp.h"
#include "rtc_base/event.h"
#include "rtc_base/task_queue_for_test.h"
#include "rtc_base/task_utils/to_queued_task.h"
#include "system_wrappers/include/clock.h"
#include "test/gmock.h"
#include "test/gtest.h"
@ -32,12 +35,6 @@ using ::testing::Return;
constexpr TimeDelta kTimeout = TimeDelta::Millis(1000);
void Sleep(TimeDelta time_delta) {
// Note that Chromium style guide prohibits use of <thread> and <chrono> in
// production code, used here since webrtc::SleepMs may return early.
std::this_thread::sleep_for(std::chrono::microseconds(time_delta.us()));
}
class MockClosure {
public:
MOCK_METHOD(TimeDelta, Call, ());
@ -59,6 +56,51 @@ class MockTaskQueue : public TaskQueueBase {
CurrentTaskQueueSetter task_queue_setter_;
};
class FakeTaskQueue : public TaskQueueBase {
public:
explicit FakeTaskQueue(SimulatedClock* clock)
: task_queue_setter_(this), clock_(clock) {}
void Delete() override {}
void PostTask(std::unique_ptr<QueuedTask> task) override {
PostDelayedTask(std::move(task), 0);
}
void PostDelayedTask(std::unique_ptr<QueuedTask> task,
uint32_t milliseconds) override {
last_task_ = std::move(task);
last_delay_ = milliseconds;
}
bool AdvanceTimeAndRunLastTask() {
EXPECT_TRUE(last_task_);
EXPECT_TRUE(last_delay_);
clock_->AdvanceTimeMilliseconds(last_delay_.value_or(0));
last_delay_.reset();
auto task = std::move(last_task_);
bool delete_task = task->Run();
if (!delete_task) {
// If the task should not be deleted then just release it.
task.release();
}
return delete_task;
}
bool IsTaskQueued() { return !!last_task_; }
uint32_t last_delay() const {
EXPECT_TRUE(last_delay_.has_value());
return last_delay_.value_or(-1);
}
private:
CurrentTaskQueueSetter task_queue_setter_;
SimulatedClock* clock_;
std::unique_ptr<QueuedTask> last_task_;
absl::optional<uint32_t> last_delay_;
};
class MoveOnlyClosure {
public:
explicit MoveOnlyClosure(MockClosure* mock) : mock_(mock) {}
@ -79,65 +121,75 @@ class MoveOnlyClosure {
TEST(RepeatingTaskTest, TaskIsStoppedOnStop) {
const TimeDelta kShortInterval = TimeDelta::Millis(50);
const TimeDelta kLongInterval = TimeDelta::Millis(200);
const int kShortIntervalCount = 4;
const int kMargin = 1;
TaskQueueForTest task_queue("TestQueue");
SimulatedClock clock(Timestamp::Zero());
FakeTaskQueue task_queue(&clock);
std::atomic_int counter(0);
auto handle = RepeatingTaskHandle::Start(task_queue.Get(), [&] {
if (++counter >= kShortIntervalCount)
return kLongInterval;
auto handle = RepeatingTaskHandle::Start(&task_queue, [&] {
counter++;
return kShortInterval;
});
// Sleep long enough to go through the initial phase.
Sleep(kShortInterval * (kShortIntervalCount + kMargin));
EXPECT_EQ(counter.load(), kShortIntervalCount);
EXPECT_EQ(task_queue.last_delay(), 0u);
EXPECT_FALSE(task_queue.AdvanceTimeAndRunLastTask());
EXPECT_EQ(counter.load(), 1);
task_queue.PostTask(
[handle = std::move(handle)]() mutable { handle.Stop(); });
// Sleep long enough that the task would run at least once more if not
// stopped.
Sleep(kLongInterval * 2);
EXPECT_EQ(counter.load(), kShortIntervalCount);
// The handle reposted at the short interval.
EXPECT_EQ(task_queue.last_delay(), kShortInterval.ms());
// Stop the handle. This prevernts the counter from incrementing.
handle.Stop();
EXPECT_TRUE(task_queue.AdvanceTimeAndRunLastTask());
EXPECT_EQ(counter.load(), 1);
}
TEST(RepeatingTaskTest, CompensatesForLongRunTime) {
const int kTargetCount = 20;
const int kTargetCountMargin = 2;
const TimeDelta kRepeatInterval = TimeDelta::Millis(2);
// Sleeping inside the task for longer than the repeat interval once, should
// be compensated for by repeating the task faster to catch up.
const TimeDelta kSleepDuration = TimeDelta::Millis(20);
const int kSleepAtCount = 3;
std::atomic_int counter(0);
TaskQueueForTest task_queue("TestQueue");
RepeatingTaskHandle::Start(task_queue.Get(), [&] {
if (++counter == kSleepAtCount)
Sleep(kSleepDuration);
return kRepeatInterval;
});
Sleep(kRepeatInterval * kTargetCount);
// Execution time should not have affected the run count,
// but we allow some margin to reduce flakiness.
EXPECT_GE(counter.load(), kTargetCount - kTargetCountMargin);
SimulatedClock clock(Timestamp::Zero());
FakeTaskQueue task_queue(&clock);
RepeatingTaskHandle::Start(
&task_queue,
[&] {
++counter;
// Task takes longer than the repeat duration.
clock.AdvanceTime(kSleepDuration);
return kRepeatInterval;
},
&clock);
EXPECT_EQ(task_queue.last_delay(), 0u);
EXPECT_FALSE(task_queue.AdvanceTimeAndRunLastTask());
// Task is posted right away since it took longer to run then the repeat
// interval.
EXPECT_EQ(task_queue.last_delay(), 0u);
EXPECT_EQ(counter.load(), 1);
}
TEST(RepeatingTaskTest, CompensatesForShortRunTime) {
SimulatedClock clock(Timestamp::Millis(0));
FakeTaskQueue task_queue(&clock);
std::atomic_int counter(0);
TaskQueueForTest task_queue("TestQueue");
RepeatingTaskHandle::Start(task_queue.Get(), [&] {
++counter;
// Sleeping for the 100 ms should be compensated.
Sleep(TimeDelta::Millis(100));
return TimeDelta::Millis(300);
});
Sleep(TimeDelta::Millis(400));
RepeatingTaskHandle::Start(
&task_queue,
[&] {
// Simulate the task taking 100ms, which should be compensated for.
counter++;
clock.AdvanceTime(TimeDelta::Millis(100));
return TimeDelta::Millis(300);
},
&clock);
// We expect that the task have been called twice, once directly at Start and
// once after 300 ms has passed.
EXPECT_EQ(counter.load(), 2);
// Expect instant post task.
EXPECT_EQ(task_queue.last_delay(), 0u);
// Task should be retained by the handler since it is not cancelled.
EXPECT_FALSE(task_queue.AdvanceTimeAndRunLastTask());
// New delay should be 200ms since repeat delay was 300ms but task took 100ms.
EXPECT_EQ(task_queue.last_delay(), 200u);
}
TEST(RepeatingTaskTest, CancelDelayedTaskBeforeItRuns) {
@ -168,16 +220,16 @@ TEST(RepeatingTaskTest, CancelTaskAfterItRuns) {
TEST(RepeatingTaskTest, TaskCanStopItself) {
std::atomic_int counter(0);
TaskQueueForTest task_queue("TestQueue");
RepeatingTaskHandle handle;
task_queue.PostTask([&] {
handle = RepeatingTaskHandle::Start(task_queue.Get(), [&] {
++counter;
handle.Stop();
return TimeDelta::Millis(2);
});
SimulatedClock clock(Timestamp::Zero());
FakeTaskQueue task_queue(&clock);
RepeatingTaskHandle handle = RepeatingTaskHandle::Start(&task_queue, [&] {
++counter;
handle.Stop();
return TimeDelta::Millis(2);
});
Sleep(TimeDelta::Millis(10));
EXPECT_EQ(task_queue.last_delay(), 0u);
// Task cancelled itself so wants to be released.
EXPECT_TRUE(task_queue.AdvanceTimeAndRunLastTask());
EXPECT_EQ(counter.load(), 1);
}