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

Feat: Add breadcrumbs for the Apollo integration #1726

Merged
merged 4 commits into from
Sep 17, 2021
Merged

Conversation

maciejwalkowiak
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak commented Sep 16, 2021

📜 Description

Feat: Add breadcrumbs for the Apollo integration

💡 Motivation and Context

Fixes #1721

💚 How did you test it?

Unit tests.

📝 Checklist

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

@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review September 16, 2021 09:00
@Test
fun `adds breadcrumb when http calls succeeds`() {
executeQuery()
verify(fixture.hub).addBreadcrumb(com.nhaarman.mockitokotlin2.check<Breadcrumb> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
verify(fixture.hub).addBreadcrumb(com.nhaarman.mockitokotlin2.check<Breadcrumb> {
verify(fixture.hub).addBreadcrumb(check<Breadcrumb> {

we can just import the check extension method

@@ -30,8 +33,9 @@ class SentryApolloInterceptorTest {

class Fixture {
val server = MockWebServer()
val hub = spy(HubAdapter.getInstance())
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why spy instead of mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially it was meant to be full integration test, using coroutines Sentry context. But, since on Android Sentry context does not need to be used, I've changed this test to use mocked hub instead of verifying transport.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

left 2 comments but rather than that LGTM

@maciejwalkowiak maciejwalkowiak merged commit ae95ea3 into main Sep 17, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1721 branch September 17, 2021 10:41
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.

Add breadcrumbs for the Apollo integration as OkHttp does
2 participants