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: Fixed flaky testTimezoneChangeNotificationBreadcrumb #2335

Conversation

kevinrenskers
Copy link
Contributor

Locally it never failed; I can run it 1000 times repeatedly and it will succeed just fine. Maybe adding this tiny sleep will help on CI.

Closes #2247
#skip-changelog

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

The real way to solve our notification-based test issues is going to be to mock the components that should receive the notifications so that we can hook the handlers and add a call to XCTExpectation.fulfill().

@kevinrenskers
Copy link
Contributor Author

The real way to solve our notification-based test issues is going to be to mock the components that should receive the notifications so that we can hook the handlers and add a call to XCTExpectation.fulfill().

I'm not totally sure what you mean here. Shouldn't we rather mock away the Notification Center, instead of the components that should receive the notification?

@github-actions
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.39 ms 1236.60 ms 24.21 ms
Size 20.50 KiB 343.47 KiB 322.96 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
e43ce74 1235.77 ms 1252.06 ms 16.29 ms
0e2e593 1234.78 ms 1266.10 ms 31.32 ms
82e596a 1209.76 ms 1237.10 ms 27.35 ms
9ad4a5f 1261.12 ms 1270.12 ms 9.00 ms
8168e86 1210.14 ms 1233.16 ms 23.02 ms
4a188b8 1245.57 ms 1266.04 ms 20.47 ms
db5f62a 1234.47 ms 1257.80 ms 23.33 ms
7d3d1a3 1218.26 ms 1238.94 ms 20.68 ms
0fdf0b2 1243.92 ms 1250.86 ms 6.94 ms
e2f1150 1220.39 ms 1249.94 ms 29.55 ms

App size

Revision Plain With Sentry Diff
e43ce74 20.51 KiB 335.49 KiB 314.99 KiB
0e2e593 20.50 KiB 333.88 KiB 313.38 KiB
82e596a 20.50 KiB 337.76 KiB 317.26 KiB
9ad4a5f 20.50 KiB 342.24 KiB 321.73 KiB
8168e86 20.50 KiB 338.99 KiB 318.49 KiB
4a188b8 20.50 KiB 337.70 KiB 317.20 KiB
db5f62a 20.51 KiB 333.16 KiB 312.65 KiB
7d3d1a3 20.50 KiB 342.24 KiB 321.73 KiB
0fdf0b2 20.51 KiB 332.90 KiB 312.39 KiB
e2f1150 20.51 KiB 333.10 KiB 312.59 KiB

@kevinrenskers kevinrenskers merged commit bdfccaa into master Nov 2, 2022
@kevinrenskers kevinrenskers deleted the fix/2247-reenable-testTimezoneChangeNotificationBreadcrumb branch November 2, 2022 12:18
kevinrenskers added a commit that referenced this pull request Nov 7, 2022
* master:
  test: Delete empty OOMLogicTests (#2361)
  fix: profiling transaction timestamps (#2359)
  fix: profiling transaction thread IDs (#2358)
  test: Use flush for macOS-SPM sample (#2360)
  release: 7.30.0
  fix: SentryCrash writing nan for invalid number (#2348)
  HTTP Client errors (#2308)
  ci: Call make for analyze (#2353)
  fix: CoreData tracking entity name retrieval (#2329)
  fix: sampled profile format backend validation errors (#2350)
  build(deps): bump github/codeql-action from 2.1.29 to 2.1.30 (#2351)
  build(deps): bump github/codeql-action from 2.1.28 to 2.1.29 (#2344)
  fix: Fixed flaky testTimezoneChangeNotificationBreadcrumb (#2335)

# Conflicts:
#	Sentry.xcodeproj/project.pbxproj
@armcknight
Copy link
Member

Sorry I didn't circle back on this @kevinrenskers!

I'm not totally sure what you mean here. Shouldn't we rather mock away the Notification Center, instead of the components that should receive the notification?

Yes, that's a good strategy. @philipphofmann actually did such a thing here, fyi: #2198

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 flaky testTimezoneChangeNotificationBreadcrumb
3 participants