Copilot commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4313676751
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -731,14 +732,33 @@ TEST_F(HistogramTest, window) {
}
TEST_F(HistogramTest, window_forgets_old_samples) {
+ // Own the unscheduled sampler until after h's destructor calls destroy().
+ std::unique_ptr<bvar::Histogram::sampler_type> sampler;
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
+ sampler = std::make_unique<bvar::Histogram::sampler_type>(&h);
Review Comment:
`std::make_unique` requires C++14; if this repo/tests are compiled as C++11
(common for older setups), this will fail to compile. If C++14 isn’t guaranteed
here, replace `std::make_unique<...>(&h)` with an equivalent C++11 construction
(e.g., `std::unique_ptr<...>(new ...)`) or use an existing project helper that
provides `make_unique` on C++11.
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -707,9 +707,10 @@ TEST_F(HistogramTest, window) {
// Window values are produced by the global sampler thread. Wait until it
// observes all writes instead of assuming a fixed sleep covers a sampling
- // round, which is not guaranteed when the test host is busy.
+ // round, which is not guaranteed when the test host is busy. Allow up to
+ // 30 seconds at 100 ms per retry.
bvar::Histogram::Value wv;
- for (int i = 0; i < 50; ++i) {
+ for (int i = 0; i < 300; ++i) {
wv = w.get_value();
if (wv.num == 3) {
break;
Review Comment:
The comment says “100 ms per retry” but this hunk doesn’t show any
sleep/backoff in the retry loop; if there isn’t a sleep/yield elsewhere in the
loop body, this becomes a tight busy-spin that both wastes CPU and may still
not “wait 30 seconds” in wall time. Add an explicit 100ms sleep/yield on
unsuccessful iterations (or update the comment if the sleep is implemented
below these lines).
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -731,14 +732,33 @@ TEST_F(HistogramTest, window) {
}
TEST_F(HistogramTest, window_forgets_old_samples) {
+ // Own the unscheduled sampler until after h's destructor calls destroy().
+ std::unique_ptr<bvar::Histogram::sampler_type> sampler;
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
+ sampler = std::make_unique<bvar::Histogram::sampler_type>(&h);
+ h._sampler = sampler.get();
Review Comment:
This test now directly assigns to `h._sampler`, which tightly couples it to
`bvar::Histogram` internals and makes the test brittle if the implementation
changes. Consider wrapping this in a small test-only RAII helper (in this test
file) that “installs” an explicit sampler and documents required
invariants/cleanup, or prefer a public/test-supported hook if one exists, so
future refactors don’t silently break the unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
