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

Fix and reenable testANRDetected_UpdatesAppStateToTrue #2526

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Dec 13, 2022

📜 Description

  • The test calls SentryWatchdogTerminationTrackingIntegration.anrDetected.
  • This calls self.appStateManager.updateAppState, but that used the TestFileManager under the hood!
  • updateAppState fails because [self.fileManager readAppState] returns nil.

Using the normal file manager fixes the test.

#skip-changelog

💡 Motivation and Context

Closes #2522

💚 How did you test it?

Unit test

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

- The test calls `SentryWatchdogTerminationTrackingIntegration.anrDetected`.
- This calls `self.appStateManager.updateAppState`, but that uses the TestFileManager under the hood!
- `updateAppState` fails because `[self.fileManager readAppState]` returns `nil`.

Closes #2522

#skip-changelog
@kevinrenskers
Copy link
Contributor Author

My first idea was to have the test filemanager call the super's methods when reading the app state, but that broke some other tests. So now instead we simply use the real file manager again, done.

@github-actions
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1238.40 ms 1256.08 ms 17.68 ms
Size 20.75 KiB 405.14 KiB 384.39 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
bbe81cb 1257.25 ms 1272.24 ms 14.99 ms
909e73a 1217.78 ms 1229.70 ms 11.92 ms
a9e77dc 1231.94 ms 1254.85 ms 22.91 ms
a9db8a6 1199.83 ms 1234.56 ms 34.73 ms
6b8ec2a 1229.15 ms 1253.80 ms 24.65 ms
88ac2c2 1223.04 ms 1243.12 ms 20.08 ms
7f1b08e 1252.20 ms 1257.70 ms 5.50 ms
bc27163 1215.45 ms 1249.46 ms 34.01 ms
4dc66f6 1202.59 ms 1228.34 ms 25.75 ms
f8045b6 1226.25 ms 1252.41 ms 26.16 ms

App size

Revision Plain With Sentry Diff
bbe81cb 20.75 KiB 381.81 KiB 361.06 KiB
909e73a 20.75 KiB 383.40 KiB 362.65 KiB
a9e77dc 20.75 KiB 379.12 KiB 358.36 KiB
a9db8a6 20.75 KiB 383.37 KiB 362.62 KiB
6b8ec2a 20.75 KiB 405.10 KiB 384.34 KiB
88ac2c2 20.75 KiB 373.61 KiB 352.86 KiB
7f1b08e 20.75 KiB 383.31 KiB 362.55 KiB
bc27163 20.75 KiB 405.10 KiB 384.34 KiB
4dc66f6 20.75 KiB 381.81 KiB 361.06 KiB
f8045b6 20.75 KiB 404.83 KiB 384.08 KiB

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #2526 (2a4b08b) into 8.0.0 (0ccb7ce) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##            8.0.0    #2526    +/-   ##
========================================
  Coverage   78.55%   78.56%            
========================================
  Files         242      241     -1     
  Lines       22288    22273    -15     
  Branches     9844     9831    -13     
========================================
- Hits        17508    17498    -10     
+ Misses       4331     4212   -119     
- Partials      449      563   +114     
Impacted Files Coverage Δ
Sources/Sentry/SentryAppStateManager.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryFileManager.m 94.95% <100.00%> (ø)
Sources/Sentry/SentryRequestOperation.m 67.56% <0.00%> (-10.82%) ⬇️
Sources/Sentry/SentryTime.mm 57.89% <0.00%> (-5.27%) ⬇️
Sources/Sentry/SentryThreadMetadataCache.cpp 91.30% <0.00%> (-4.35%) ⬇️
Sources/Sentry/SentryThreadHandle.cpp 69.39% <0.00%> (-3.28%) ⬇️
Sources/Sentry/SentryBacktrace.cpp 86.60% <0.00%> (-1.79%) ⬇️
Sources/Sentry/SentryNetworkTracker.m 95.20% <0.00%> (-0.19%) ⬇️
Sources/Sentry/SentryTracer.m 94.16% <0.00%> (ø)
Sources/Sentry/SentryDevice.mm 53.09% <0.00%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ccb7ce...2a4b08b. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -173,7 +173,7 @@ - (void)updateAppState:(void (^)(SentryAppState *))block
{
@synchronized(self) {
SentryAppState *appState = [self.fileManager readAppState];
if (nil != appState) {
if (appState != nil) {
Copy link
Member

Choose a reason for hiding this comment

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

Like I do that, get rid of style Yoda. 😁

@kevinrenskers kevinrenskers merged commit b3fd7e0 into 8.0.0 Dec 13, 2022
@kevinrenskers kevinrenskers deleted the fix/fix-testANRDetected_UpdatesAppStateToTrue branch December 13, 2022 14:45
kevinrenskers added a commit that referenced this pull request Dec 13, 2022
* 8.0.0:
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)

# Conflicts:
#	Sources/Sentry/SentryClient.m
#	Tests/SentryTests/SentryClientTests.swift
kevinrenskers added a commit that referenced this pull request Dec 16, 2022
* 8.0.0: (31 commits)
  tests: Reenable testAddAndRemoveData (#2533)
  feat: support SENTRY_DSN environment var on macOS (#2534)
  Remove duplicate entry (#2532)
  fix: ARC issue for FileManager (#2525)
  feat: Add SwiftUI performance tracking (#2271)
  fix: Remove all permission checks (#2529)
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)
  test: Fix disabled failing watchdog test (#2521)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#2516)
  Rename the watchdog option and integration (#2513)
  feat: Enable CaptureFailedRequests by default (#2507)
  test: Fix asserts for SentryCrashTestInstallation (#2500)
  meta: User interaction tracing enabled per default (#2506)
  ref: Rename OOM to Watchdog Terminations (#2499)
  feat: enableUserInteractionTracing is GA (#2503)
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  ...
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.

Fix broken test testANRDetected_UpdatesAppStateToTrue
2 participants