-
Notifications
You must be signed in to change notification settings - Fork 440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests to confirm observable counter aggregation bug #1516
Add tests to confirm observable counter aggregation bug #1516
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1516 +/- ##
=======================================
+ Coverage 62.4% 62.5% +0.1%
=======================================
Files 144 144
Lines 19891 20091 +200
=======================================
+ Hits 12426 12576 +150
- Misses 7465 7515 +50 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once conflict fixed.
We can also consider the use of parameterized tests for scenarios where only the inputs and expected outputs differ, to reduce redundant test code. Rust doesn't support it directly, but there are external crates providing this option e.g., - https://crates.io/crates/parameterized_test , or we can create a custom macro as suggested here - https://stackoverflow.com/questions/34662713/how-can-i-create-parameterized-tests-in-rust/ Not related to this PR, for future :) |
Observable counter has an aggregation bug. Most likely same issue exists for ObservableUpDownCounter as well.
This PR just adds a test that will fail, and hence ignored for now.
Will look at the fix separately.