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

Improve test coverage for OpenTelemetry #2401

Merged
merged 6 commits into from Dec 6, 2022
Merged

Improve test coverage for OpenTelemetry #2401

merged 6 commits into from Dec 6, 2022

Conversation

adinauer
Copy link
Member

#skip-changelog

📜 Description

Add more tests for OpenTelemetry SentrySpanProcessor and SentryPropagator

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Nov 30, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 4681dac

@adinauer adinauer mentioned this pull request Dec 1, 2022
21 tasks
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

Base: 80.04% // Head: 80.38% // Increases project coverage by +0.33% 🎉

Coverage data is based on head (4681dac) compared to base (20d7d3d).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2401      +/-   ##
============================================
+ Coverage     80.04%   80.38%   +0.33%     
- Complexity     3765     3778      +13     
============================================
  Files           301      301              
  Lines         14206    14206              
  Branches       1883     1883              
============================================
+ Hits          11371    11419      +48     
+ Misses         2092     2045      -47     
+ Partials        743      742       -1     
Impacted Files Coverage Δ
...c/main/java/io/sentry/opentelemetry/TraceData.java 100.00% <0.00%> (+8.33%) ⬆️
...a/io/sentry/opentelemetry/SentrySpanProcessor.java 91.72% <0.00%> (+15.03%) ⬆️
...java/io/sentry/opentelemetry/SentryPropagator.java 79.48% <0.00%> (+69.23%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!


extractedContext.makeCurrent().use { _ ->
val otelSpan = givenSpanBuilder().startSpan()
thenTraceIdIsUsed(otelSpan)
Copy link
Member

Choose a reason for hiding this comment

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

nice helper methods 👍


@Test
fun `does nothing on start if Sentry has not been initialized`() {
val hub = mock<IHub>()
Copy link
Member

Choose a reason for hiding this comment

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

s: Any reason why you're not using the fixture's hub here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to make sure it's a clean mock that doesn't respond to anything but the wanted invocation on the hub. Rest is mocks that differ from other tests anyways. Guess it's also easier to read with the fresh mock instead of getting the one from the fixture that makes people read through all the setup code.

@adinauer adinauer merged commit 61132a2 into main Dec 6, 2022
@adinauer adinauer deleted the feat/otel-tests branch December 6, 2022 15:06
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.

None yet

3 participants