mapbox-navigation-android icon indicating copy to clipboard operation
mapbox-navigation-android copied to clipboard

Disable the flaky test and make instrumentation-tests build required

Open dzinad opened this issue 3 years ago • 10 comments

There is one flaky instrumentation test, described here: https://github.com/mapbox/mapbox-navigation-android/issues/5931. Because of it the instrumentation-tests CI build is optional. And some real failure can be easily missed since the build is red quite often. In order to avoid merging PRs that break instrumentation tests we could do the following:

  1. Temporarily disable RouteRefreshTest#routeRefreshesWorksAfterSettingsNewRoutes;
  2. Make the instrumentation-tests build required;
  3. Enable RouteRefreshTest#routeRefreshesWorksAfterSettingsNewRoutes after fix (should probably add this point to the mentioned ticket).

dzinad avatar Jun 27 '22 12:06 dzinad

I'm not sure that RouteRefreshTest#routeRefreshesWorksAfterSettingsNewRoutes the only one that makes our test suite flaky. But the solution could work I think

VysotskiVadim avatar Jul 05 '22 14:07 VysotskiVadim

Enable RouteRefreshTest#routeRefreshesWorksAfterSettingsNewRoutes after fix (should probably add this point to the mentioned ticket).

Do we understand the root cause for the flakiness of this test? I'd like to understand it better before moving forward with disabling it - which is otherwise a reasonable approach to make instrumentation tests mandatory.

zugaldia avatar Jul 05 '22 19:07 zugaldia

As I understand it, it fails by timeout. Maybe some emulators are lagging but we'd need to investigate further.

dzinad avatar Jul 06 '22 11:07 dzinad

Do we understand the root cause for the flakiness of this test?

As far as I understand this ticket, @dzinad please correct me if you meant something different, we need to apply a different approach for handling of flaky instrumentation test. We used to make the instrumentation tests run not required if the tests fail so often that it's hard to merge a PR. New suggestion is to always keep instrumentation tests run mandatary, but to disable individual tests if they fail, to make sure we run and verify at least subset of tests.

VysotskiVadim avatar Jul 07 '22 12:07 VysotskiVadim

As far as I understand this ticket, @dzinad please correct me if you meant something different, we need to apply a different approach for handling of flaky instrumentation test. We used to make the instrumentation tests run not required if the tests fail so often that it's hard to merge a PR. New suggestion is to always keep instrumentation tests run mandatary, but to disable individual tests if they fail, to make sure we run and verify at least subset of tests.

Exactly. If we want, we can create a separate build for flaky tests which will be optional. But the main point is that the majority of the tests are mandatory for every PR.

dzinad avatar Jul 07 '22 12:07 dzinad

Seems like failures are fixed. However, in several PRs I get "inconclusive" results. Does anybody know what to do with it? @mapbox/navigation-android Example: https://circleci.com/gh/mapbox/mapbox-navigation-android/94624?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

dzinad avatar Jul 27 '22 08:07 dzinad

"inconclusive" looks like a Firebase infra issue. Documentation says:

Inconclusive: Test results were inconclusive, possibly due to a Test Lab error.

I think we can ignore the inconclusive results (or re-run a job if needed). If this becomes much more common, we can get in touch with the Firebase support team to get more information.

LukasPaczos avatar Jul 27 '22 09:07 LukasPaczos

I think we can ignore the inconclusive results (or re-run a job if needed).

But this way we won't be able to make the build required. Because the build itself fails.

dzinad avatar Jul 27 '22 09:07 dzinad

What's the frequency of these inconsistent results that you noticed?

In case of an occurrence, we'll have to re-run the test. If this happens often enough to cause significant pain, let's get in touch with support, maybe there's some configuration parameter that we can set or specific devices we can choose to mitigate the issue.

cc @MrSwimmer for visibility

LukasPaczos avatar Jul 27 '22 09:07 LukasPaczos

I noticed it in 2 of 5 of my PRs. But the most recent ones. Maybe it's a new issue. Let's observe for some time.

dzinad avatar Jul 27 '22 13:07 dzinad