Skip to content
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

Convert tests to from :sdk:metrics-testing to :sdk:testing #4444

Merged

Conversation

jack-berg
Copy link
Member

Wanted to increase confidence that :sdk:testing has all the tools needed for making metrics assertions after realizing that internally we're still using :sdk:metrics-testing everywhere. So I switched all our internal usage to :sdk:testing.

For the most part, the tools in :sdk:testing suffice. In rare cases like asserting the size of point data it was possible but more awkward. In cases where we test directly test low level features like exemplar we don't have the accessors to make assertions directly (everything flows through assertThat(MetricData)).

I found that HistogramPointAssert was missing methods to assert exemplars, so I added those.

I deleted everything in :sdk:metrics-testing that was no longer being used. What's left are some utilities for asserting exemplars directly, and for asserting exponential histograms.

@jack-berg jack-berg requested a review from a user May 7, 2022 13:39
@jack-berg jack-berg requested a review from Oberon00 as a code owner May 7, 2022 13:39
@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #4444 (5e138de) into main (89c6323) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4444      +/-   ##
============================================
+ Coverage     89.95%   90.01%   +0.06%     
- Complexity     4957     4970      +13     
============================================
  Files           569      570       +1     
  Lines         15358    15384      +26     
  Branches       1470     1472       +2     
============================================
+ Hits          13815    13848      +33     
+ Misses         1073     1067       -6     
+ Partials        470      469       -1     
Impacted Files Coverage Δ
...etry/sdk/testing/assertj/HistogramPointAssert.java 100.00% <100.00%> (+14.81%) ⬆️
...telemetry/sdk/testing/assertj/HistogramAssert.java 100.00% <0.00%> (ø)
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 100.00% <0.00%> (+5.88%) ⬆️
...telemetry/sdk/testing/assertj/LongPointAssert.java 100.00% <0.00%> (+15.38%) ⬆️

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 89c6323...5e138de. Read the comment docs.

@jsuereth
Copy link
Contributor

jsuereth commented May 8, 2022

My only comment here is that some of the tests used satisifiesExactly and some used anySatisfy for points. It seems the new testing library either doesn't support that use case, or we need some other assertions in addition here.

Specifically, some of the "satisfiesExactly" are in place to prevent certain bad behaviors, while the "anySatisfy" are because testing exact behavior is hard.

@anuraaga
Copy link
Contributor

anuraaga commented May 9, 2022

We can add assertions that seem useful indeed. I wouldn't expect to need satisfiesExactly - our data model / OTLP doesn't specify anything about the order of points within a metric AFAIK, so presumably we wouldn't want assertions with strict ordering. anySatisfy on the flipside seems very imprecise as it only matches one point so also not sure about adding this - it's always possible to break out of fluent assertions to use normal assertj matching, and if we see a lot of that in the wild then we'd want to add it.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks @jack-berg completely missed doing this

@jack-berg jack-berg merged commit e067223 into open-telemetry:main May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants