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

Mark transaction as internal_error in case of unhandled errors #1218

Merged
merged 5 commits into from Jan 12, 2023

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Jan 10, 2023

📜 Description

💡 Motivation and Context

Closes #1182

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (v7.0.0@dfa335f). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             v7.0.0    #1218   +/-   ##
=========================================
  Coverage          ?   89.04%           
=========================================
  Files             ?      111           
  Lines             ?     3487           
  Branches          ?        0           
=========================================
  Hits              ?     3105           
  Misses            ?      382           
  Partials          ?        0           

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.


## 7.0.0-alpha.1

### Various fixes & improvements
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixing the changelog.

);
}
/// Parse and raise an event out of the Isolate error.
@visibleForTesting
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved from a standalone function to inside of the class.

Comment on lines +70 to +74
// marks the span status if none to `internal_error` in case there's an
// unhandled error
hub.configureScope((scope) => {
scope.span?.status ??= const SpanStatus.internalError(),
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the added code.

@@ -20,6 +22,40 @@ class RunZonedGuardedIntegration extends Integration {
/// Needed to check if we somehow caused a `print()` recursion
bool _isPrinting = false;

@visibleForTesting
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved to the class, to be tested.

Comment on lines +50 to +54
// marks the span status if none to `internal_error` in case there's an
// unhandled error
hub.configureScope((scope) => {
scope.span?.status ??= const SpanStatus.internalError(),
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added code.

@@ -9,134 +9,170 @@ import '../mocks.mocks.dart';
import 'mock_platform_dispatcher.dart';

void main() {
TestWidgetsFlutterBinding.ensureInitialized();
group(OnErrorIntegration, () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved within the group

final throwableMechanism = event.throwableMechanism as ThrowableMechanism;
expect(throwableMechanism.mechanism.handled, true);
});
test('marks transaction as internal error if no status', () async {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test

@marandaneto marandaneto marked this pull request as ready for review January 11, 2023 14:10
Copy link
Collaborator

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@marandaneto marandaneto merged commit 795f47f into v7.0.0 Jan 12, 2023
@marandaneto marandaneto deleted the chore/internalerror-transaction branch January 12, 2023 09:42
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