fix: Add WinUIUnhandledExceptionIntegration on Windows only
#3055
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
==========================================
- Coverage 76.42% 76.42% -0.01%
==========================================
Files 351 351
Lines 13262 13263 +1
Branches 2646 2646
==========================================
Hits 10136 10136
- Misses 2449 2450 +1
Partials 677 677 ☔ View full report in Codecov by Sentry. |
src/Sentry/SentryOptions.cs
Outdated
| @@ -186,7 +186,7 @@ internal IEnumerable<ISdkIntegration> Integrations | |||
| } | |||
| #endif | |||
|
|
|||
| #if NET5_0_OR_GREATER && !__MOBILE__ | |||
| #if NET5_0_OR_GREATER && WINDOWS | |||
There was a problem hiding this comment.
Does this mean building for net8.0 for example won't have this handler and is that by design?
Sounds right to me, but checking, if a WInUI app has to always be built with net8.0-windowsXYZ or something.
There was a problem hiding this comment.
@lucas-zimerman might know
There was a problem hiding this comment.
Note: we don't have a net8.0-windows target in the SDK so I think this isn't getting what you expect
There was a problem hiding this comment.
This is a big TIL. So we have to check during runtime whether to add it depending on the current platform.
There was a problem hiding this comment.
to be honest I don't follow about the support of UnhandledException on .NET 8.
There was a problem hiding this comment.
This is a big TIL. So we have to check during runtime whether to add it depending on the current platform.
yeah all the compiler flags affect the DLLs we output. And net8.0 is platform agnostic so ANDROID/WINDOWS/etc wil not be set.
| public Task Integrations_default_ones_are_properly_registered() | ||
| { | ||
| // Windows additionally adds `WinUIUnhandledExceptionIntegration` | ||
| Skip.If(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)); |
There was a problem hiding this comment.
should we have two tests then? one that tests we have this on Windows? Instead of skipping
There was a problem hiding this comment.
I had that but it messed with the verify. I guess I need to run this on windows then too to generate the expected file?
There was a problem hiding this comment.
yeah you need to have a file per platform, just saw this on another PR:
There was a problem hiding this comment.
one of those per OS I imagine
@bitsandfoxes is there a ticket to track this? good to link to this PR |
Follor up PR is here #3063 |
Fixes #3050