-
-
Notifications
You must be signed in to change notification settings - Fork 221
fix: Source context for class libraries when running on Android in Release mode #4294
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
Conversation
explorerAssembly = new(explorer, assembly); | ||
return true; | ||
} | ||
} | ||
_logger?.Invoke("No best assembly for {0} in APK AssemblyStore", name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we have one logger but shouldn't this be a level Error log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have to refactor the AssemblyReader classes that we vendored in fomr MS in order to be able to do that (they use a different pattern for logging).
Probably the easiest would be to replace DebugLogger with our own IDiagnosticLogger
... would involve quite a few changes, but then we've made so many changes to these already that the only way to merge in changes from new code is by hand. So a few more changes wouldn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bruno-garcia one remaining question on this PR is whether we want to refactor all the vendored in code to use our IDiagnosticLogger
instead of the DebugLogger
delegate that is used currently:
public delegate void DebugLogger(string message, params object?[] args); |
The existing delegate doesn't have any notion of "log levels" (so doesn't support what you suggested above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't add that as a requirement to unblock this. But it's not unreasonable to add if it'll help us debug this better going forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k, I'll track that in a separate issue as I think we should get this fix out ASAP:
Note This behaviour only manifests in CI (with the headless runners). Device tests work fine when run locally on a simulator. Something very unusual happening with the device tests. Running against API levels 27 and 33, 27 always fails. 33 fails frequently (but not always). Since Google require folks target 34 and higher to be able to release anything on the play store now, I figured I'd bump the API levels we test against to 34 (oldest supported) and 36 (latest). The also both seem to fail. Looking at the logs, there don't appear to be any failed tests. In one run, I noticed the last line that gets logged from .NET SDK is:
Shortly after that I see some logs from sentry-native:
So it looks like potentially there is a low level crash that sentry-native is picking up and sending as an envelope. To where though and from which test? Even then, this was only on the Run against API level 34... level 36 doesn't show any native errors. |
The caching transport tests can fail when running the device tests if the simulator looses network connectivity. This PR changes the test to use a `FakeReliableNetworkStatusListener ` implementation of `INetworkStatusListener` that ignores the real network status and just pretends that everything is OK, so that test reliability isn't impacted by network connectivity. Extracted from #4294 #skip-changelog
The issues running android device tests in CI seem to manifest when running the About my only hypothesis is that, by default, SentryOptions creates a new sentry-dotnet/src/Sentry/SentryOptions.cs Line 1326 in 723c84e
How this is in any way related to the changes this PR makes to the Android Assembly Readers in .NET 9 escapes me... but I'll try substituting in a Fake reliable network status listener. In a MAUI app, a |
The caching transport tests can fail when running the device tests if the simulator looses network connectivity. This PR changes the test to use a `FakeReliableNetworkStatusListener ` implementation of `INetworkStatusListener` that ignores the real network status and just pretends that everything is OK, so that test reliability isn't impacted by network connectivity. Extracted from #4294 #skip-changelog
OK after days of fluffing about disabling and enabling different tests in a protracted divide and conquer algorithm, it looks like this is the culprit (assuming there are no other flaky tests)... so I've disabled this in CI on Android: sentry-dotnet/test/Sentry.Maui.Tests/SentryMauiScreenshotTests.cs Lines 184 to 189 in 9346258
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Resolves: #4278, #4342
Note
Introducing this change caused the Android Device Tests to start failing on CI only. After a lot of trial and error, I eventually narrowed it down to this test (which has therefore been disabled in CI on Android):
sentry-dotnet/test/Sentry.Maui.Tests/SentryMauiScreenshotTests.cs
Lines 184 to 189 in 9346258