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_change_queue_unittest.cc b/buffet/states/state_change_queue_unittest.cc
index 8205f77..7aa6c9f 100644
--- a/buffet/states/state_change_queue_unittest.cc
+++ b/buffet/states/state_change_queue_unittest.cc
@@ -28,63 +28,111 @@
}
TEST_F(StateChangeQueueTest, UpdateOne) {
- StateChange change;
- change.timestamp = base::Time::Now();
- change.property_set.emplace("prop.name", int{23});
- ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change));
+ StateChange change{
+ base::Time::Now(),
+ chromeos::VariantDictionary{{"prop.name", int{23}}}
+ };
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change.timestamp,
+ change.changed_properties));
EXPECT_FALSE(queue_->IsEmpty());
auto changes = queue_->GetAndClearRecordedStateChanges();
ASSERT_EQ(1, changes.size());
EXPECT_EQ(change.timestamp, changes.front().timestamp);
- EXPECT_EQ(change.property_set, changes.front().property_set);
+ EXPECT_EQ(change.changed_properties, changes.front().changed_properties);
EXPECT_TRUE(queue_->IsEmpty());
EXPECT_TRUE(queue_->GetAndClearRecordedStateChanges().empty());
}
TEST_F(StateChangeQueueTest, UpdateMany) {
- StateChange change1;
- change1.timestamp = base::Time::Now();
- change1.property_set.emplace("prop.name1", int{23});
- ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change1));
- StateChange change2;
- change2.timestamp = base::Time::Now();
- change2.property_set.emplace("prop.name1", int{17});
- change2.property_set.emplace("prop.name2", double{1.0});
- change2.property_set.emplace("prop.name3", bool{false});
- ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change2));
+ StateChange change1{
+ base::Time::Now(),
+ chromeos::VariantDictionary{{"prop.name1", int{23}}}
+ };
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change1.timestamp,
+ change1.changed_properties));
+ StateChange change2{
+ base::Time::Now(),
+ chromeos::VariantDictionary{
+ {"prop.name1", int{17}},
+ {"prop.name2", double{1.0}},
+ {"prop.name3", bool{false}},
+ }
+ };
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change2.timestamp,
+ change2.changed_properties));
EXPECT_FALSE(queue_->IsEmpty());
auto changes = queue_->GetAndClearRecordedStateChanges();
ASSERT_EQ(2, changes.size());
- EXPECT_EQ(change1.timestamp, changes.front().timestamp);
- EXPECT_EQ(change1.property_set, changes.front().property_set);
- EXPECT_EQ(change2.timestamp, changes.back().timestamp);
- EXPECT_EQ(change2.property_set, changes.back().property_set);
+ EXPECT_EQ(change1.timestamp, changes[0].timestamp);
+ EXPECT_EQ(change1.changed_properties, changes[0].changed_properties);
+ EXPECT_EQ(change2.timestamp, changes[1].timestamp);
+ EXPECT_EQ(change2.changed_properties, changes[1].changed_properties);
EXPECT_TRUE(queue_->IsEmpty());
EXPECT_TRUE(queue_->GetAndClearRecordedStateChanges().empty());
}
+TEST_F(StateChangeQueueTest, GroupByTimestamp) {
+ base::Time timestamp = base::Time::Now();
+ base::TimeDelta time_delta = base::TimeDelta::FromMinutes(1);
+
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ timestamp,
+ chromeos::VariantDictionary{{"prop.name1", int{1}}}));
+
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ timestamp,
+ chromeos::VariantDictionary{{"prop.name2", int{2}}}));
+
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ timestamp,
+ chromeos::VariantDictionary{{"prop.name1", int{3}}}));
+
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ timestamp + time_delta,
+ chromeos::VariantDictionary{{"prop.name1", int{4}}}));
+
+ auto changes = queue_->GetAndClearRecordedStateChanges();
+ ASSERT_EQ(2, changes.size());
+
+ chromeos::VariantDictionary expected1{
+ {"prop.name1", int{3}},
+ {"prop.name2", int{2}},
+ };
+ chromeos::VariantDictionary expected2{
+ {"prop.name1", int{4}},
+ };
+ EXPECT_EQ(timestamp, changes[0].timestamp);
+ EXPECT_EQ(expected1, changes[0].changed_properties);
+ EXPECT_EQ(timestamp + time_delta, changes[1].timestamp);
+ EXPECT_EQ(expected2, changes[1].changed_properties);
+}
+
TEST_F(StateChangeQueueTest, MaxQueueSize) {
queue_.reset(new StateChangeQueue(2));
base::Time start_time = base::Time::Now();
- base::TimeDelta time_delta = base::TimeDelta::FromMinutes(1);
+ base::TimeDelta time_delta1 = base::TimeDelta::FromMinutes(1);
+ base::TimeDelta time_delta2 = base::TimeDelta::FromMinutes(3);
- StateChange change;
- change.timestamp = start_time;
- change.property_set.emplace("prop.name1", int{1});
- change.property_set.emplace("prop.name2", int{2});
- ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change));
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ start_time,
+ chromeos::VariantDictionary{
+ {"prop.name1", int{1}},
+ {"prop.name2", int{2}},
+ }));
- change.timestamp += time_delta;
- change.property_set.clear();
- change.property_set.emplace("prop.name1", int{3});
- change.property_set.emplace("prop.name3", int{4});
- ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change));
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ start_time + time_delta1,
+ chromeos::VariantDictionary{
+ {"prop.name1", int{3}},
+ {"prop.name3", int{4}},
+ }));
- change.timestamp += time_delta;
- change.property_set.clear();
- change.property_set.emplace("prop.name10", int{10});
- change.property_set.emplace("prop.name11", int{11});
- ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change));
+ ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
+ start_time + time_delta2,
+ chromeos::VariantDictionary{
+ {"prop.name10", int{10}},
+ {"prop.name11", int{11}},
+ }));
auto changes = queue_->GetAndClearRecordedStateChanges();
ASSERT_EQ(2, changes.size());
@@ -94,15 +142,15 @@
{"prop.name2", int{2}},
{"prop.name3", int{4}},
};
- EXPECT_EQ(start_time + time_delta, changes.front().timestamp);
- EXPECT_EQ(expected1, changes.front().property_set);
+ EXPECT_EQ(start_time + time_delta1, changes[0].timestamp);
+ EXPECT_EQ(expected1, changes[0].changed_properties);
chromeos::VariantDictionary expected2{
{"prop.name10", int{10}},
{"prop.name11", int{11}},
};
- EXPECT_EQ(start_time + 2 * time_delta, changes.back().timestamp);
- EXPECT_EQ(expected2, changes.back().property_set);
+ EXPECT_EQ(start_time + time_delta2, changes[1].timestamp);
+ EXPECT_EQ(expected2, changes[1].changed_properties);
}
} // namespace buffet
