buffet: Fix issues in StateChangeQueue
Addressed additional code review issues brought after the original
code has been published.
- NotifyPropertiesUpdated() now takes timestamp and property set
as separate parameters to simplify implementation of StateManager.
- StateChangeQueue::max_queue_size_ is made 'const'
- StateChangeQueue now maintains a time-to-event map instead of
a vector of StateChange. This allows to keep the events sorted
by time stamp. Also adding discrete events with the same time stamp
coalesces the property changes.
- added a unit test for coalescing property set changes based on
the timestamp.
BUG=None
TEST=FEATURES=test emerge-link buffet
Change-Id: I309816d1f040558620fa68a844b05251d0e4319b
Reviewed-on: https://chromium-review.googlesource.com/226300
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Reviewed-by: Anton Muhin <antonm@chromium.org>
Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
Tested-by: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/states/state_manager_unittest.cc b/buffet/states/state_manager_unittest.cc
index b4c3094..df2260a 100644
--- a/buffet/states/state_manager_unittest.cc
+++ b/buffet/states/state_manager_unittest.cc
@@ -44,11 +44,6 @@
})");
}
-MATCHER_P(IsStateChange, prop_set, "") {
- return arg.property_set == prop_set &&
- std::abs((arg.timestamp - base::Time::Now()).InSeconds()) < 2;
-}
-
} // anonymous namespace
class StateManagerTest : public ::testing::Test {
@@ -56,7 +51,8 @@
void SetUp() override {
// Initial expectations.
EXPECT_CALL(mock_state_change_queue_, IsEmpty()).Times(0);
- EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_)).Times(0);
+ EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_, _))
+ .Times(0);
EXPECT_CALL(mock_state_change_queue_, GetAndClearRecordedStateChanges())
.Times(0);
mgr_.reset(new StateManager(&mock_state_change_queue_));
@@ -109,33 +105,22 @@
chromeos::VariantDictionary expected_prop_set{
{"terminator.target", std::string{"John Connor"}},
};
+ base::Time timestamp = base::Time::Now();
EXPECT_CALL(mock_state_change_queue_,
- NotifyPropertiesUpdated(IsStateChange(expected_prop_set)))
+ NotifyPropertiesUpdated(timestamp, expected_prop_set))
.WillOnce(Return(true));
ASSERT_TRUE(mgr_->SetPropertyValue("terminator.target",
- std::string{"John Connor"}, nullptr));
+ std::string{"John Connor"},
+ timestamp,
+ nullptr));
EXPECT_EQ("{'base':{'manufacturer':'Skynet','serialNumber':'T1000'},"
"'terminator':{'target':'John Connor'}}",
ValueToString(mgr_->GetStateValuesAsJson(nullptr).get()));
}
-TEST_F(StateManagerTest, UpdateProperties) {
- chromeos::VariantDictionary prop_set{
- {"base.serialNumber", std::string{"T1000.1"}},
- {"terminator.target", std::string{"Sarah Connor"}},
- };
- EXPECT_CALL(mock_state_change_queue_,
- NotifyPropertiesUpdated(IsStateChange(prop_set)))
- .WillOnce(Return(true));
- ASSERT_TRUE(mgr_->UpdateProperties(prop_set, nullptr));
- EXPECT_EQ("{'base':{'manufacturer':'Skynet','serialNumber':'T1000.1'},"
- "'terminator':{'target':'Sarah Connor'}}",
- ValueToString(mgr_->GetStateValuesAsJson(nullptr).get()));
-}
-
TEST_F(StateManagerTest, SetPropertyValue_Error_NoName) {
chromeos::ErrorPtr error;
- ASSERT_FALSE(mgr_->SetPropertyValue("", int{0}, &error));
+ ASSERT_FALSE(mgr_->SetPropertyValue("", int{0}, base::Time::Now(), &error));
EXPECT_EQ(errors::state::kDomain, error->GetDomain());
EXPECT_EQ(errors::state::kPropertyNameMissing, error->GetCode());
EXPECT_EQ("Property name is missing", error->GetMessage());
@@ -143,7 +128,8 @@
TEST_F(StateManagerTest, SetPropertyValue_Error_NoPackage) {
chromeos::ErrorPtr error;
- ASSERT_FALSE(mgr_->SetPropertyValue("target", int{0}, &error));
+ ASSERT_FALSE(mgr_->SetPropertyValue("target", int{0}, base::Time::Now(),
+ &error));
EXPECT_EQ(errors::state::kDomain, error->GetDomain());
EXPECT_EQ(errors::state::kPackageNameMissing, error->GetCode());
EXPECT_EQ("Package name is missing in the property name",
@@ -152,7 +138,8 @@
TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownPackage) {
chromeos::ErrorPtr error;
- ASSERT_FALSE(mgr_->SetPropertyValue("power.level", int{0}, &error));
+ ASSERT_FALSE(mgr_->SetPropertyValue("power.level", int{0}, base::Time::Now(),
+ &error));
EXPECT_EQ(errors::state::kDomain, error->GetDomain());
EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode());
EXPECT_EQ("Unknown state property package 'power'", error->GetMessage());
@@ -160,28 +147,33 @@
TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownProperty) {
chromeos::ErrorPtr error;
- ASSERT_FALSE(mgr_->SetPropertyValue("base.level", int{0}, &error));
+ ASSERT_FALSE(mgr_->SetPropertyValue("base.level", int{0}, base::Time::Now(),
+ &error));
EXPECT_EQ(errors::state::kDomain, error->GetDomain());
EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode());
EXPECT_EQ("State property 'base.level' is not defined", error->GetMessage());
}
-TEST_F(StateManagerTest, RetrievePropertyChanges) {
- EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_))
+TEST_F(StateManagerTest, GetAndClearRecordedStateChanges) {
+ base::Time timestamp = base::Time::Now();
+ EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(timestamp, _))
.WillOnce(Return(true));
ASSERT_TRUE(mgr_->SetPropertyValue("terminator.target",
- std::string{"John Connor"}, nullptr));
+ std::string{"John Connor"},
+ timestamp,
+ nullptr));
std::vector<StateChange> expected_val;
- expected_val.emplace_back();
- expected_val.back().timestamp = base::Time::Now();
- expected_val.back().property_set.emplace("terminator.target",
- std::string{"John Connor"});
+ expected_val.emplace_back(
+ timestamp,
+ chromeos::VariantDictionary{{"terminator.target",
+ std::string{"John Connor"}}});
EXPECT_CALL(mock_state_change_queue_, GetAndClearRecordedStateChanges())
.WillOnce(Return(expected_val));
auto changes = mgr_->GetAndClearRecordedStateChanges();
ASSERT_EQ(1, changes.size());
EXPECT_EQ(expected_val.back().timestamp, changes.back().timestamp);
- EXPECT_EQ(expected_val.back().property_set, changes.back().property_set);
+ EXPECT_EQ(expected_val.back().changed_properties,
+ changes.back().changed_properties);
}
