Add unit tests for duration metrics
As a side-effect, move utils::GetMonotonicTime() into the newly added
ClockInterface type.
BUG=None
TEST=Unit tests pass
Change-Id: I972a7e4ba37b63f96348fbeda901697b8ba2fc05
Reviewed-on: https://gerrit.chromium.org/gerrit/48814
Reviewed-by: Chris Sosa <sosa@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
diff --git a/SConstruct b/SConstruct
index 78e50b6..fe6170d 100644
--- a/SConstruct
+++ b/SConstruct
@@ -247,6 +247,7 @@
certificate_checker.cc
chrome_browser_proxy_resolver.cc
chrome_proxy_resolver.cc
+ clock.cc
connection_manager.cc
constants.cc
cycle_breaker.cc
diff --git a/clock.cc b/clock.cc
new file mode 100644
index 0000000..96979f9
--- /dev/null
+++ b/clock.cc
@@ -0,0 +1,28 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/clock.h"
+
+#include <time.h>
+
+namespace chromeos_update_engine {
+
+base::Time Clock::GetWallclockTime() {
+ return base::Time::Now();
+}
+
+base::Time Clock::GetMonotonicTime() {
+ struct timespec now_ts;
+ if (clock_gettime(CLOCK_MONOTONIC_RAW, &now_ts) != 0) {
+ // Avoid logging this as an error as call-sites may call this very
+ // often and we don't want to fill up the disk...
+ return base::Time();
+ }
+ struct timeval now_tv;
+ now_tv.tv_sec = now_ts.tv_sec;
+ now_tv.tv_usec = now_ts.tv_nsec/base::Time::kNanosecondsPerMicrosecond;
+ return base::Time::FromTimeVal(now_tv);
+}
+
+} // namespace chromeos_update_engine
diff --git a/clock.h b/clock.h
new file mode 100644
index 0000000..027bcff
--- /dev/null
+++ b/clock.h
@@ -0,0 +1,28 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_CLOCK_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_CLOCK_H__
+
+#include "update_engine/clock_interface.h"
+
+namespace chromeos_update_engine {
+
+// Implements a clock.
+class Clock : public ClockInterface {
+ public:
+ Clock() {}
+
+ virtual base::Time GetWallclockTime();
+
+ virtual base::Time GetMonotonicTime();
+
+ private:
+
+ DISALLOW_COPY_AND_ASSIGN(Clock);
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_CLOCK_H__
diff --git a/clock_interface.h b/clock_interface.h
new file mode 100644
index 0000000..c2b1a04
--- /dev/null
+++ b/clock_interface.h
@@ -0,0 +1,35 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_CLOCK_INTERFACE_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_CLOCK_INTERFACE_H__
+
+#include <string>
+
+#include <base/time.h>
+
+namespace chromeos_update_engine {
+
+// The clock interface allows access to various system clocks. The
+// sole reason for providing this as an interface is unit testing.
+// Additionally, the sole reason for the methods not being static
+// is also unit testing.
+class ClockInterface {
+ public:
+ // Gets the current time e.g. similar to base::Time::Now().
+ virtual base::Time GetWallclockTime() = 0;
+
+ // Returns monotonic time since some unspecified starting point. It
+ // is not increased when the system is sleeping nor is it affected
+ // by NTP or the user changing the time.
+ //
+ // (This is a simple wrapper around clock_gettime(2) / CLOCK_MONOTONIC_RAW.)
+ virtual base::Time GetMonotonicTime() = 0;
+
+ virtual ~ClockInterface() {}
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_CLOCK_INTERFACE_H__
diff --git a/fake_clock.h b/fake_clock.h
new file mode 100644
index 0000000..105d664
--- /dev/null
+++ b/fake_clock.h
@@ -0,0 +1,42 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_FAKE_CLOCK_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_FAKE_CLOCK_H__
+
+#include "update_engine/clock_interface.h"
+
+namespace chromeos_update_engine {
+
+// Implements a clock that can be made to tell any time you want.
+class FakeClock : public ClockInterface {
+ public:
+ FakeClock() {}
+
+ virtual base::Time GetWallclockTime() {
+ return wallclock_time_;
+ }
+
+ virtual base::Time GetMonotonicTime() {
+ return monotonic_time_;
+ }
+
+ void SetWallclockTime(const base::Time &time) {
+ wallclock_time_ = time;
+ }
+
+ void SetMonotonicTime(const base::Time &time) {
+ monotonic_time_ = time;
+ }
+
+ private:
+ base::Time wallclock_time_;
+ base::Time monotonic_time_;
+
+ DISALLOW_COPY_AND_ASSIGN(FakeClock);
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_FAKE_CLOCK_H__
diff --git a/mock_system_state.cc b/mock_system_state.cc
index b7ed27b..152347a 100644
--- a/mock_system_state.cc
+++ b/mock_system_state.cc
@@ -12,6 +12,7 @@
MockSystemState::MockSystemState()
: default_request_params_(this),
prefs_(&mock_prefs_) {
+ clock_ = &default_clock_;
request_params_ = &default_request_params_;
mock_payload_state_.Initialize(this);
mock_gpio_handler_ = new testing::NiceMock<MockGpioHandler>();
diff --git a/mock_system_state.h b/mock_system_state.h
index ac93d16..eccfd9b 100644
--- a/mock_system_state.h
+++ b/mock_system_state.h
@@ -13,6 +13,7 @@
#include "update_engine/mock_dbus_interface.h"
#include "update_engine/mock_gpio_handler.h"
#include "update_engine/mock_payload_state.h"
+#include "update_engine/clock.h"
#include "update_engine/prefs_mock.h"
#include "update_engine/system_state.h"
@@ -34,6 +35,10 @@
MOCK_CONST_METHOD0(device_policy, const policy::DevicePolicy*());
MOCK_METHOD0(system_rebooted, bool());
+ inline virtual ClockInterface* clock() {
+ return clock_;
+ }
+
inline virtual ConnectionManager* connection_manager() {
return connection_manager_;
}
@@ -69,6 +74,10 @@
return &mock_metrics_lib_;
}
+ inline void set_clock(ClockInterface* clock) {
+ clock_ = clock;
+ }
+
inline void set_prefs(PrefsInterface* prefs) {
prefs_ = prefs;
}
@@ -95,9 +104,11 @@
MockDbusGlib dbus_;
// These are the other object we own.
+ Clock default_clock_;
OmahaRequestParams default_request_params_;
// These are pointers to objects which caller can override.
+ ClockInterface* clock_;
PrefsInterface* prefs_;
ConnectionManager* connection_manager_;
OmahaRequestParams* request_params_;
diff --git a/payload_state.cc b/payload_state.cc
index 77652a7..390dd6a 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -10,6 +10,7 @@
#include "base/string_util.h"
#include <base/stringprintf.h>
+#include "update_engine/clock.h"
#include "update_engine/constants.h"
#include "update_engine/prefs.h"
#include "update_engine/system_state.h"
@@ -139,7 +140,7 @@
void PayloadState::UpdateSucceeded() {
// Send the relevant metrics that are tracked in this class to UMA.
CalculateUpdateDurationUptime();
- SetUpdateTimestampEnd(Time::Now());
+ SetUpdateTimestampEnd(system_state_->clock()->GetWallclockTime());
ReportBytesDownloadedMetrics();
ReportUpdateUrlSwitchesMetric();
ReportRebootMetrics();
@@ -483,7 +484,7 @@
SetUrlFailureCount(0);
SetUrlSwitchCount(0);
UpdateBackoffExpiryTime(); // This will reset the backoff expiry time.
- SetUpdateTimestampStart(Time::Now());
+ SetUpdateTimestampStart(system_state_->clock()->GetWallclockTime());
SetUpdateTimestampEnd(Time()); // Set to null time
SetUpdateDurationUptime(TimeDelta::FromSeconds(0));
ResetDownloadSourcesOnNewUpdate();
@@ -636,8 +637,9 @@
}
TimeDelta PayloadState::GetUpdateDuration() {
- Time end_time = update_timestamp_end_.is_null() ? Time::Now() :
- update_timestamp_end_;
+ Time end_time = update_timestamp_end_.is_null()
+ ? system_state_->clock()->GetWallclockTime() :
+ update_timestamp_end_;
return end_time - update_timestamp_start_;
}
@@ -647,7 +649,7 @@
CHECK(prefs_);
- Time now = Time::Now();
+ Time now = system_state_->clock()->GetWallclockTime();
if (!prefs_->Exists(kPrefsUpdateTimestampStart)) {
// The preference missing is not unexpected - in that case, just
@@ -747,11 +749,12 @@
}
void PayloadState::SetUpdateDurationUptime(const TimeDelta& value) {
- SetUpdateDurationUptimeExtended(value, utils::GetMonotonicTime(), true);
+ Time now = system_state_->clock()->GetMonotonicTime();
+ SetUpdateDurationUptimeExtended(value, now, true);
}
void PayloadState::CalculateUpdateDurationUptime() {
- Time now = utils::GetMonotonicTime();
+ Time now = system_state_->clock()->GetMonotonicTime();
TimeDelta uptime_since_last_update = now - update_duration_uptime_timestamp_;
TimeDelta new_uptime = update_duration_uptime_ + uptime_since_last_update;
// We're frequently called so avoid logging this write
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index e7b7ed2..28017dc 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -11,9 +11,11 @@
#include "gtest/gtest.h"
#include "update_engine/constants.h"
+#include "update_engine/fake_clock.h"
#include "update_engine/mock_system_state.h"
#include "update_engine/omaha_request_action.h"
#include "update_engine/payload_state.h"
+#include "update_engine/prefs.h"
#include "update_engine/prefs_mock.h"
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
@@ -745,4 +747,69 @@
EXPECT_EQ(0, payload_state.GetNumReboots());
}
+TEST(PayloadStateTest, DurationsAreCorrect) {
+ OmahaResponse response;
+ PayloadState payload_state;
+ MockSystemState mock_system_state;
+ FakeClock fake_clock;
+ Prefs prefs;
+ string temp_dir;
+
+ // Set the clock to a well-known time - 1 second on the wall-clock
+ // and 2 seconds on the monotonic clock
+ fake_clock.SetWallclockTime(Time::FromInternalValue(1000000));
+ fake_clock.SetMonotonicTime(Time::FromInternalValue(2000000));
+
+ // We need persistent preferences for this test
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateDurationTests.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+
+ mock_system_state.set_clock(&fake_clock);
+ mock_system_state.set_prefs(&prefs);
+ EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+
+ // Check that durations are correct for a successful update where
+ // time has advanced 7 seconds on the wall clock and 4 seconds on
+ // the monotonic clock.
+ SetupPayloadStateWith2Urls("Hash8593", &payload_state, &response);
+ fake_clock.SetWallclockTime(Time::FromInternalValue(8000000));
+ fake_clock.SetMonotonicTime(Time::FromInternalValue(6000000));
+ payload_state.UpdateSucceeded();
+ EXPECT_EQ(payload_state.GetUpdateDuration().InMicroseconds(), 7000000);
+ EXPECT_EQ(payload_state.GetUpdateDurationUptime().InMicroseconds(), 4000000);
+
+ // Check that durations are reset when a new response comes in.
+ SetupPayloadStateWith2Urls("Hash8594", &payload_state, &response);
+ EXPECT_EQ(payload_state.GetUpdateDuration().InMicroseconds(), 0);
+ EXPECT_EQ(payload_state.GetUpdateDurationUptime().InMicroseconds(), 0);
+
+ // Advance time a bit (10 secs), simulate download progress and
+ // check that durations are updated.
+ fake_clock.SetWallclockTime(Time::FromInternalValue(18000000));
+ fake_clock.SetMonotonicTime(Time::FromInternalValue(16000000));
+ payload_state.DownloadProgress(10);
+ EXPECT_EQ(payload_state.GetUpdateDuration().InMicroseconds(), 10000000);
+ EXPECT_EQ(payload_state.GetUpdateDurationUptime().InMicroseconds(), 10000000);
+
+ // Now simulate a reboot by resetting monotonic time (to 5000) and
+ // creating a new PayloadState object and check that we load the
+ // durations correctly (e.g. they are the same as before).
+ fake_clock.SetMonotonicTime(Time::FromInternalValue(5000));
+ PayloadState payload_state2;
+ EXPECT_TRUE(payload_state2.Initialize(&mock_system_state));
+ EXPECT_EQ(payload_state2.GetUpdateDuration().InMicroseconds(), 10000000);
+ EXPECT_EQ(payload_state2.GetUpdateDurationUptime().InMicroseconds(),10000000);
+
+ // Advance wall-clock by 7 seconds and monotonic clock by 6 seconds
+ // and check that the durations are increased accordingly.
+ fake_clock.SetWallclockTime(Time::FromInternalValue(25000000));
+ fake_clock.SetMonotonicTime(Time::FromInternalValue(6005000));
+ payload_state2.UpdateSucceeded();
+ EXPECT_EQ(payload_state2.GetUpdateDuration().InMicroseconds(), 17000000);
+ EXPECT_EQ(payload_state2.GetUpdateDurationUptime().InMicroseconds(),16000000);
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
}
diff --git a/real_system_state.h b/real_system_state.h
index 72a07ad..42d1509 100644
--- a/real_system_state.h
+++ b/real_system_state.h
@@ -7,6 +7,7 @@
#include <update_engine/system_state.h>
+#include <update_engine/clock.h>
#include <update_engine/connection_manager.h>
#include <update_engine/gpio_handler.h>
#include <update_engine/payload_state.h>
@@ -34,6 +35,10 @@
return device_policy_;
}
+ virtual inline ClockInterface* clock() {
+ return &clock_;
+ }
+
virtual inline ConnectionManager* connection_manager() {
return &connection_manager_;
}
@@ -73,6 +78,9 @@
bool Initialize(bool enable_gpio);
private:
+ // Interface for the clock.
+ Clock clock_;
+
// The latest device policy object from the policy provider.
const policy::DevicePolicy* device_policy_;
diff --git a/system_state.h b/system_state.h
index 6f06a34..313d670 100644
--- a/system_state.h
+++ b/system_state.h
@@ -17,6 +17,7 @@
// SystemState is the root class within the update engine. So we should avoid
// any circular references in header file inclusion. Hence forward-declaring
// the required classes.
+class ClockInterface;
class ConnectionManager;
class PrefsInterface;
class PayloadStateInterface;
@@ -43,6 +44,9 @@
virtual void set_device_policy(const policy::DevicePolicy* device_policy) = 0;
virtual const policy::DevicePolicy* device_policy() const = 0;
+ // Gets the interface object for the clock.
+ virtual ClockInterface* clock() = 0;
+
// Gets the connection manager object.
virtual ConnectionManager* connection_manager() = 0;
diff --git a/utils.cc b/utils.cc
index f0cea0d..3eb0679 100644
--- a/utils.cc
+++ b/utils.cc
@@ -15,7 +15,6 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
-#include <time.h>
#include <unistd.h>
#include <algorithm>
@@ -972,20 +971,6 @@
return result;
}
-
-Time GetMonotonicTime() {
- struct timespec now_ts;
- if (clock_gettime(CLOCK_MONOTONIC_RAW, &now_ts) != 0) {
- // Avoid logging this as an error as call-sites may call this very
- // often and we don't want to fill up the disk...
- return Time();
- }
- struct timeval now_tv;
- now_tv.tv_sec = now_ts.tv_sec;
- now_tv.tv_usec = now_ts.tv_nsec/Time::kNanosecondsPerMicrosecond;
- return Time::FromTimeVal(now_tv);
-}
-
} // namespace utils
} // namespace chromeos_update_engine
diff --git a/utils.h b/utils.h
index 72c6ea1..74c4d16 100644
--- a/utils.h
+++ b/utils.h
@@ -28,13 +28,6 @@
namespace utils {
-// Returns monotonic time since some unspecified starting point. It is
-// not increased when the system is sleeping nor is it affected by
-// NTP or the user changing the time.
-//
-// (This is a simple wrapper around clock_gettime(2) / CLOCK_MONOTONIC_RAW.)
-base::Time GetMonotonicTime();
-
// Returns true if this is an official Chrome OS build, false otherwise.
bool IsOfficialBuild();