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

ci: Remove test retries #2242

Merged
merged 7 commits into from
Oct 4, 2022
Merged

ci: Remove test retries #2242

merged 7 commits into from
Oct 4, 2022

Conversation

philipphofmann
Copy link
Member

Instead of retrying the failing tests multiple times, we should fix them or disable them. Retry can hide flaky tests. Furthermore, retries slow down CI.

#skip-changelog

Instead of retrying the failing tests multiple times, we
should fix them or disable them. Retry can hide flaky tests.
Furthermore, retries slow down CI.
@philipphofmann philipphofmann changed the title The flakiest tests SentrySessionTrackerTests were fixed. ci: Remove test retries Sep 29, 2022
@kevinrenskers
Copy link
Contributor

Instead of retrying the failing tests multiple times, we should fix them or disable them. Retry can hide flaky tests.

I'm afraid we'll always have situations where tests sometimes randomly fail in CI but not locally, looking back at previous projects at big clients as well. Of course fixing those tests would be preferable but honestly it's not always possible or cost effective. Some tests are just.. stubborn and difficult but still important to have.

Furthermore, retries slow down CI.

Manually having to retry them over and over slows it down way more though ;)

I'm not sure if removing the retry logic really helps us or hinders us, but not really seeing the positive to removing it at the moment? Definitely not while we still have flaky tests in our suite.

@brustolin
Copy link
Contributor

You only remove the retry and CI is already failing. I would rather keep it for now.

@philipphofmann
Copy link
Member Author

With the test retries, we are fighting symptoms, not the actual root cause. Better to have a few stable tests than plenty with a few flaky ones. If CI fails, you should take a step back and investigate. At the moment, I mostly ignore the failing tests and keep retrying still with our retry logic. We should disable the flaky ones to have a reliable CI again. Currently, it just is red for almost every PR.

@kevinrenskers
Copy link
Contributor

I don't think we'll get to 100% non-flaky tests, unless we start disabling a whole bunch of them.

I think at least this PR should also disable the known flaky tests when removing the retry logic, or it's just going to cause a lot of pain.

@philipphofmann
Copy link
Member Author

I don't think we'll get to 100% non-flaky tests, unless we start disabling a whole bunch of them.

I think at least this PR should also disable the known flaky tests when removing the retry logic, or it's just going to cause a lot of pain.

What about whenever you see a flaky test failing your build, you just open an issue an disable it? Otherwise, it's going to be hard for me to disable all flaky tests in one PR.

@kevinrenskers
Copy link
Contributor

kevinrenskers commented Oct 3, 2022

I don't think we'll get to 100% non-flaky tests, unless we start disabling a whole bunch of them.
I think at least this PR should also disable the known flaky tests when removing the retry logic, or it's just going to cause a lot of pain.

What about whenever you see a flaky test failing your build, you just open an issue an disable it? Otherwise, it's going to be hard for me to disable all flaky tests in one PR.

There are 5 6 open issues about flaky tests, so at least we can disable them in this PR when you remove the retry logic?
https://github.com/getsentry/sentry-cocoa/issues?q=is:issue+is:open+label:%22Type:+Flaky+Test%22

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.

I agree with Philipp here. If a test is flaky, then how can I trust it's testing anything meaningful when it "passes"? It's better to just disable or remove the test, or figure out what's wrong with it.

@armcknight
Copy link
Member

How about the SauceLabs test runs? 😉

@philipphofmann
Copy link
Member Author

How about the SauceLabs test runs? 😉

SauceLabs itself is flaky, so I think we have to keep the retries there.

@philipphofmann philipphofmann merged commit 347a8e9 into master Oct 4, 2022
@philipphofmann philipphofmann deleted the ci/remove-test-retry branch October 4, 2022 15:23
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