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

Fix fromjson serialization of breadcrumbs from android #3993

Merged
merged 2 commits into from
Feb 24, 2025

Conversation

aritchie
Copy link
Collaborator

Resolves #3851 should also prevent issues from cocoa SDK as well

Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Looks good. Would be great if we could add some unit tests. I think these would only run on the Android Device tests but that's OK.

@aritchie
Copy link
Collaborator Author

@jamescrosswell I'm not sure how we would unit test these. Thoughts? The value coming from native is a string. I don't see anyway to simulate all error conditions from native at this time either.

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Feb 24, 2025

@jamescrosswell I'm not sure how we would unit test these. Thoughts? The value coming from native is a string. I don't see anyway to simulate all error conditions from native at this time either.

We may not have bulletproof unit tests but perfect is the enemy of good, as they say.

Ultimately this method takes a JsonElement as input. It should be possible to test the method with various different inputs.

In the most basic case, that could be a JsonElement representing some JSON produced by the sentry-dotnet SDK when we serialize a breadcrumb.

For this particular change, it would also be good to test it with the JSON for a breadcrumb that had been serialised using the Native SDK. Worst case, we simply run one of our sample apps and capture that JSON in the console/debug logs then copy/paste into a test. Ideally we could run the most recent version of the Native code during the test to serialise one or more breadcrumbs to JSON and then parse that... which would ensure we were always testing against the JSON produced by the same version of the Native SDK that we embed with sentry-dotnet. Those would obviously have to be device tests.

@aritchie aritchie merged commit 8059366 into main Feb 24, 2025
22 checks passed
@aritchie aritchie deleted the 3851-android_breadcrumb_mismap branch February 24, 2025 21:58
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.

[Android] ArgumentException while processing Breadcrumbs
2 participants