Skip to content
This repository has been archived by the owner on Jun 10, 2021. It is now read-only.

Remove clear_measurements_on_publish #31

Merged
merged 2 commits into from Dec 11, 2019
Merged

Remove clear_measurements_on_publish #31

merged 2 commits into from Dec 11, 2019

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Dec 10, 2019

As alluded to at #16 (comment), this change removes the clear_measurements_on_publish argument from PeriodicMeasurementNode, treating it as if it was always true.

@mm318 mm318 requested a review from dabonnie December 10, 2019 00:48
Copy link
Member

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mainly LGTM, would be nice to clean the time constants a bit before merging

Comment on lines 120 to 124
static constexpr std::chrono::milliseconds INFREQUENT_PUBLISH_PERIOD = 2 *
test_constants::TEST_DURATION;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a comment to explain why this value is OK would be helpful, not sure if I understand very well what all those constants mean, and why this is safe and this test won't be flaky.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, it's unclear what's the value here there. Once every 10ms, 10 seconds? INFREQUENT is not really helpful in this context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomas-moulard and @dabonnie, can you take a look again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mm318 - before merging could you run this with --gtest_repeat=100000 to make sure it's not flaky?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to repeat the test 14000+ times without fail before running into some unrelated RTPS error (like this aliasrobotics/aztarna#41)...

I am confident that the test is not flaky, as we should absolutely not see any publishing if the publishing period is set to twice that of the entire allotted test duration, even at the millisecond scale.

If we do see some publishing occur given this much margin (of the entire test duration), I would be concerned about the ROS2 executor.

@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #31 into master will decrease coverage by 0.37%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   42.04%   41.66%   -0.38%     
==========================================
  Files          26       26              
  Lines         880      876       -4     
  Branches      506      504       -2     
==========================================
- Hits          370      365       -5     
- Misses         50       52       +2     
+ Partials      460      459       -1
Flag Coverage Δ
#unittests 41.66% <66.66%> (-0.38%) ⬇️
Impacted Files Coverage Δ
...em_metrics_collector/periodic_measurement_node.hpp 100% <ø> (ø) ⬆️
...trics_collector/test_periodic_measurement_node.cpp 35.71% <100%> (-5.67%) ⬇️
...em_metrics_collector/periodic_measurement_node.cpp 44.68% <50%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9391954...1e25c44. Read the comment docs.

@mm318 mm318 merged commit 8f9cc75 into master Dec 11, 2019
@mm318 mm318 deleted the miaofei/remove-arg branch December 11, 2019 19:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants