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: flaky tests in CI #2101

Closed

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Aug 23, 2022

By chance I noticed one of the tests flaking uses notifications (here), so wanted to try fixing all of them by adding XCTExpectations/waits for them to post before proceeding with assertions.

The temp commit forces them to run 50 times in both xcode 12/13, hopefully that would fail at least a couple times if this doesn't fix the flakiness.

Possibly fixes #2062

#skip-changelog

@armcknight

This comment was marked as resolved.

@armcknight
Copy link
Member Author

Running just the flaky test cases 50 times without a failure, maybe this is the fix https://github.com/getsentry/sentry-cocoa/runs/7982082603?check_suite_focus=true

Some functions in TestNotificationCenter had no compiled implementation on macOS due to conditionals; instead, move the conditionals outside so the functions themselves aren't compiled at all. Delete a usage that appeared in a macOS-only test case; for all others, gate compilation of the calls.
@armcknight armcknight force-pushed the armcknight/add-expectations-for-notifications branch from f20c2da to ae8c921 Compare August 23, 2022 20:20
@armcknight
Copy link
Member Author

OK so that didn't work: https://github.com/getsentry/sentry-cocoa/runs/7982225398?check_suite_focus=true

Looking at what tests run before it that may leave dirtied state that corrupts these tests.

@armcknight armcknight changed the title fix: add expectations to ensure correct timing of notification receipt fix: flaky tests in CI Aug 23, 2022
@philipphofmann
Copy link
Member

@armcknight, what is the current state of this PR? Do you need any input?

@armcknight
Copy link
Member Author

@philipphofmann I'm stumped here currently, haven't circled back to look at what state could be getting corrupted before these tests run.

@philipphofmann
Copy link
Member

I don't understand why the tests fail in CI and not locally. If the state would be a real problem, the tests should also fail locally sometimes.

@armcknight
Copy link
Member Author

Closing this as I haven't had time to look at it. Can always resurrect the effort later.

@armcknight armcknight closed this Sep 9, 2022
@armcknight armcknight deleted the armcknight/add-expectations-for-notifications branch September 26, 2022 18:57
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 SentryTests.SentrySessionTrackerTests Flaky tests
2 participants