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.

Sign up for GitHub

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

Jump to bottom

feat: Allow setting context to null from beforeSend #381

Merged
merged 1 commit into from
Nov 2, 2018
Merged

feat: Allow setting context to null from beforeSend #381

merged 1 commit into from
Nov 2, 2018

Conversation

kattrali
Copy link
Contributor

Changeset

  • Moved automatic context detection from during serialization to capture-time, which is in Client
  • Deleted a test for the context logic in Error

Tests

  • Add new test scenario for overriding context
  • (Other scenarios cover automatic context assignment and setting context to a new value)

Review

  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

fractalwrench
fractalwrench reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@fractalwrench fractalwrench 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. I noticed one edge case where user-supplied context can get overridden, and think that we could also retain one of the unit test methods by restructuring it a bit. Once those points are addressed, I think this is ready to go.

sdk/src/androidTest/java/com/bugsnag/android/ErrorTest.java
public void testConfigContext() throws Exception {
String expected = "Junit test suite";
error.setContext(null);
config.setContext(expected);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be reworked so that config.setContext is called before the error is built.

@kattrali
feat: Allow setting context to null
d3ede0c 
Replaces tests for context logic on error with existing e2e tests
@kattrali kattrali force-pushed the kattrali/unset-context branch from ec10381 to d3ede0c Compare October 31, 2018 20:47
@kattrali kattrali merged commit d3ede0c into master Nov 2, 2018
@kattrali kattrali deleted the kattrali/unset-context branch November 14, 2018 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fractalwrench fractalwrench fractalwrench left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kattrali @fractalwrench