fix: libmonosgen and libxamarin frames currently showing as inapp#4960
fix: libmonosgen and libxamarin frames currently showing as inapp#4960jamescrosswell merged 7 commits intomainfrom
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4960 +/- ##
==========================================
- Coverage 73.87% 73.87% -0.01%
==========================================
Files 496 497 +1
Lines 17951 17957 +6
Branches 3516 3516
==========================================
+ Hits 13262 13265 +3
- Misses 3831 3833 +2
- Partials 858 859 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Assert.Equal arguments are swapped in tests
- Swapped the two
Assert.Equalcalls to passexpectedInAppfirst andsut.InAppsecond so failure output labels are correct.
- Swapped the two
Or push these changes by commenting:
@cursor push ba9940a433
Preview (ba9940a433)
diff --git a/test/Sentry.Tests/Protocol/Exceptions/SentryStackFrameTests.cs b/test/Sentry.Tests/Protocol/Exceptions/SentryStackFrameTests.cs
--- a/test/Sentry.Tests/Protocol/Exceptions/SentryStackFrameTests.cs
+++ b/test/Sentry.Tests/Protocol/Exceptions/SentryStackFrameTests.cs
@@ -256,7 +256,7 @@
sut.ConfigureAppFrame(options);
// Assert
- Assert.Equal(sut.InApp, expectedInApp);
+ Assert.Equal(expectedInApp, sut.InApp);
}
[Theory]
@@ -276,7 +276,7 @@
sut.ConfigureAppFrame(options);
// Assert
- Assert.Equal(sut.InApp, expectedInApp);
+ Assert.Equal(expectedInApp, sut.InApp);
}
[Fact]
fix argument order
| #if NET9_0_OR_GREATER | ||
| [GeneratedRegex(LibMonoSgenPattern)] | ||
| internal static partial Regex LibMonoSgen { get; } | ||
|
|
||
| [GeneratedRegex(LibXamarinPattern)] | ||
| internal static partial Regex LibXamarin { get; } |
There was a problem hiding this comment.
question: use RegexOptions.ExplicitCapture and/or RegexOptions.CultureInvariant
re RegexOptions.ExplicitCapture
When the Regex-Pattern includes parentheses (( and )), these are also "Capture Groups" per default. But not sure if we actually consume these automatically captures Groups ... where we may do some work that is not necessarily needed ... in other words: impacting performance slightly.
We have three IStringOrRegexMatcher
DelimitedPrefixOrPatternMatcher- calls
Regex.Matches
- calls
PrefixOrPatternMatcher- calls
Regex.IsMatch
- calls
SubstringOrPatternMatcher- calls
Regex.IsMatch
- calls
On the other hand ... although we are not actively using Capture Groups right now ... we might in the future and then the In-App detection might not work as expected.
I believe I'm actually talking myself out of this change now ... considering it's only about performance, not correctness.
re RegexOptions.CultureInvariant
Not sure if adding this option would actually be about correctness in our case ... perhaps also only performance.
I guess keeping the Regex-Options as they are (i.e. RegexOptions.Compiled for non-source-generated Regex) seems to be fine.
What do you think?
Co-authored-by: Stefan Pölz <38893694+Flash0ver@users.noreply.github.com>
2 participants
fixes #4194