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 #784

Merged
merged 3 commits into from
Sep 21, 2018
Merged

Fix flaky tests #784

merged 3 commits into from
Sep 21, 2018

Conversation

fathyb
Copy link
Contributor

@fathyb fathyb commented Sep 19, 2018

This PR fixes 3 common source of failure for our test :

  1. Deadlock : sometimes the test runner freeze and needs a total clean to be run. We fix this by removing the need to call the main thread in SegmentIntegration Moved to Fix flaky tests #784
  2. Missing iOS simulator : Adding a command to list the simulators seems to magically fix the issue.
  3. Flaky maxQueueSize test : I rewrote this one to be more stable

We ran 21+ successful and consecutive test iterations : https://circleci.com/gh/segmentio/workflows/analytics-ios/tree/fix%2Fflaky-test

Ref : LIB-543

@@ -6,6 +6,7 @@ jobs:
xcode: "9.4.1"
steps:
- checkout
- run: xcrun simctl list
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I added xcrun simctl list we've miraculously never had the error again. If I remove it we get the missing simulator error sporadically. This could also help us debug it next time.

[self setupFlushTimer];
});
}
self.flushTimer = [NSTimer timerWithTimeInterval:self.configuration.flushInterval
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check which thread we are on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we manually add the timer in NSRunLoop.mainRunLoop so it'll always be scheduled to run on the main thread:

https://github.com/segmentio/analytics-ios/blob/9f0eecc9846ed123f378ca87bca1afad3053b68e/Analytics/Classes/Internal/SEGSegmentIntegration.m#L115-L116

We needed to check before because we used scheduledTimerWithTimeInterval instead of timerWithTimeInterval

Copy link
Contributor

Choose a reason for hiding this comment

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

We've had some issues around concurrency - particularly as the threads we initialize integrations is not guaranteed.

This causes some integrations and customers to rely on implementation details
that have caused a few issues in the past (#518, #714, #683) so I'd like to be a bit careful rolling this out. If you're confident in these changes as is - let's plan to cut this as a beta release to get a bit more testing through this change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! If customers are relying on this kind of implementation detail I think it’d be better to spend more time looking into the impact. Moving this to #785.

}
else {
sent = -1
waitUntil(timeout: 60) {done in
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the timeout to check for the messages in webhook? if so, you may want to increase it to 3 minutes as 1 minute is sometimes not enough to find them in webhook (3 mins is what library-e2e-tester currently does).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't this one, but I pushed a new commit to increase the webhook timeout to 3 minutes - thanks!

@fathyb
Copy link
Contributor Author

fathyb commented Sep 20, 2018

Note: I pushed a commit to use NSDefaultRunLoopMode instead of NSRunLoopCommonModes so that we keep the same timer behavior as before.

@codecov-io
Copy link

Codecov Report

Merging #784 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage    85.7%   85.55%   -0.15%     
==========================================
  Files          52       52              
  Lines        2672     2666       -6     
==========================================
- Hits         2290     2281       -9     
- Misses        382      385       +3

@fathyb fathyb merged commit 9970131 into master Sep 21, 2018
@fathyb fathyb deleted the fix/flaky-test branch September 21, 2018 21:05
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.

4 participants