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

test: reset file manager state in clearTestState() #3643

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Feb 16, 2024

Noticed there was some stale state causing test failures in #3529. I don't think we ever had something that clears disk state in between tests.

#skip-changelog

@armcknight armcknight changed the title reset file manager state in clearTestState() test: reset file manager state in clearTestState() Feb 16, 2024
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1206.73 ms 1222.17 ms 15.44 ms
Size 21.58 KiB 419.68 KiB 398.10 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
189b629 1245.53 ms 1270.06 ms 24.53 ms
cce35c6 1228.88 ms 1241.14 ms 12.26 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
4bdf3dc 1232.56 ms 1252.81 ms 20.25 ms
06548c0 1262.80 ms 1275.00 ms 12.20 ms
4d6f273 1195.63 ms 1232.22 ms 36.59 ms
2405ba5 1248.37 ms 1259.30 ms 10.93 ms
db33900 1232.87 ms 1250.35 ms 17.47 ms
39b1c35 1212.90 ms 1214.30 ms 1.41 ms
881a955 1221.63 ms 1243.38 ms 21.75 ms

App size

Revision Plain With Sentry Diff
189b629 20.76 KiB 399.69 KiB 378.93 KiB
cce35c6 20.76 KiB 425.80 KiB 405.04 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
4bdf3dc 20.76 KiB 434.94 KiB 414.18 KiB
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
4d6f273 20.76 KiB 426.93 KiB 406.17 KiB
2405ba5 20.76 KiB 435.23 KiB 414.47 KiB
db33900 21.58 KiB 418.40 KiB 396.82 KiB
39b1c35 22.85 KiB 408.88 KiB 386.03 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (83887af) 89.376% compared to head (d0f6e8c) 89.341%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3643       +/-   ##
=============================================
- Coverage   89.376%   89.341%   -0.036%     
=============================================
  Files          530       530               
  Lines        58305     58299        -6     
  Branches     20926     20921        -5     
=============================================
- Hits         52111     52085       -26     
+ Misses        5285      5189       -96     
- Partials       909      1025      +116     
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 97.916% <100.000%> (+0.044%) ⬆️
Sources/Sentry/SentryFileManager.m 92.828% <0.000%> (-0.559%) ⬇️

... and 36 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83887af...d0f6e8c. 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.

I think we once did, but I can't recall why we don't do this anymore. LGTM

@@ -710,6 +710,11 @@ - (BOOL)createDirectoryIfNotExists:(NSString *)path error:(NSError **)error
return YES;
}

- (void)clearDiskState
Copy link
Member

Choose a reason for hiding this comment

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

l: This is a duplicate of the existing deleteAllFolders.

Copy link
Member Author

@armcknight armcknight Feb 16, 2024

Choose a reason for hiding this comment

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

ah i hadn't noticed that, but it's not quite a duplicate, deleteAllFolders only deletes the sentry subfolder based on the current DSN; my implementation went one level higher and deleted all dsn-based folders.

that being said, i'm not sure why when i try to replace various calls to delete envelopes, sessions etc from test setUp/tearDown methods with mine, a bunch of tests start failing. I'm betting it has to do with the @synchronized scopes in a lot of those file manager methods, like deleteSession synchronizing on currentSessionFilePath, or deleteAppState synchronizing on appStateFilePath.

I don't feel like wading into all that right now though so i'm not going to attempt any further refactoring right now.

@philipphofmann
Copy link
Member

Ah, I think now I recall why we removed it. We had trouble because some time ago, the init method of SentryFileManger called deleteOldEnvelopeItems, which deleted some envelope items async impacting other tests. Now the SentryClient calls it when deleteOldEnvelopeItems is true.

@armcknight armcknight merged commit 6e1452d into main Feb 16, 2024
73 of 75 checks passed
@armcknight armcknight deleted the armcknight/test/reset-file-manager-state branch February 16, 2024 12:26
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.

2 participants