Newly introduced tests that are flaky: what to do?

[apostasie]

What is the problem you're trying to solve

It looks like some recently introduced tests are flaky.

Some of it is because the tests were written incorrectly (failure to EnsureContainerStarted after a run -d - too short delays failing to account for slow environments - etc).
Some of it is because of bugs in the code (mostly TOCTOU on filesystem or other concurrency issues).
Overall, these may escape pure code review ^.

This is of course a problem, as these things will then require follow-on work to fix them (which no one wants to do, because, it is objectively painful and ungrateful grunt work :-)), and meanwhile contribute to growing CI instability, further preventing clean testing for new code, making it more likely to further merge more problematic tests, etc.

Of course it is tempting for maintainers to just rerun failed pipelines in all cases until they pass, so that certain PRs that are OK with code review can be merged.

But then, this is just making the problem worse unfortunately...

So, what can we do?

Describe the solution you'd like

Suggesting that we adopt a new (informal) policy for PRs: if a run fails, make sure the failure is looked into and documented (instead of being ignored + re-run).

Authors can be encouraged to identify the failure themselves, or a reviewer can do that.

There is a (hopefully) comprehensive list of known test failures in #4120 that can be used as a quick reference.

If the failure is already known (hence likely unrelated to the PR):

• systematically add a comment saying so and linking to the ticket (<- that helps keeping tallies)
• then rerun the pipeline

If the failure is not known, suggesting that we do not rerun the pipeline.
There is a high probability that the issue is coming from the PR, and (especially if it is flaky) it should not be ran again until the root cause is pinpointed.
Such a situation should either prevent merging the PR and/or requiring more scrutiny.

I am always happy to help with that when I see them (eg: like in #4368 (comment)).

What do you folks think?

Additional context

Alternatively, maybe we could consider building some form of reporting from the pipeline, that would automatically add a comment on the PR with the name of the tests that failed (or ideally automatically retrieve a link to the documented failure and post it as well).

Not quite sure how feasible ^, but that would go a long way in surfacing these problems.

[AkihiroSuda]

If the failure is not known, suggesting that we do not rerun the pipeline.

This may have a negative impact on contributors' motivation, as they are not responsible for a failure of CI that is not relevant to their own contributions.

[AkihiroSuda]

Probably no need to collect the failures of the PRs, but we should have a dashboard for the failures of the merge commits

[apostasie]

Appreciate there is a balance to find between quality and keeping it friendly for contributors.

Obviously, quality is my (pet peeve) concern. I need nerdctl to not fail randomly.

So, what about having the CI automatically comment on PRs with the list of failures, and matching that with known open issues for flaky tests?

example:

Your PR last CI run failed on these tests:

- TestSomething: this is a known flaky test: see "[ticket number matching ticket title "[CI] TestSomething is flaky]")
- TestSomethingTwo: this is a known flaky test: see "[ticket number matching ticket title "[CI] TestSomethingTwo is flaky]")

- TestSomethingElse is not known to be flaky. If this is a test you created, it might be faulty. Otherwise, a reviewer can help you assess if the issue is due to your code change or a CI issue

To reduce the noise, a single comment would be used (eg: reused over CI runs).

It does achieve:

• being more helpful to contributors, by telling them they are "fine" for certain failures instead of having them scratch their head
• outline more clearly that something "new" may be a problem
• track prevalence of flaky tests (to focus effort - this is why I like linking CI failure to their tickets)
• raise awareness for everybody about what parts of the codebase / test are the most urgent problems

WDYT?

---

"Dashboard for failures": yeah, need to think a bit more about how to do this, but I definitely like that.

[AkihiroSuda]

So, what about having the CI automatically comment on PRs with the list of failures, and matching that with known open issues for flaky tests?

👍

Is there any existing tool that we can use for this?

[apostasie]

So, what about having the CI automatically comment on PRs with the list of failures, and matching that with known open issues for flaky tests?

👍

Is there any existing tool that we can use for this?

Exploring.
Github API might be enough if this is small enough in scope, but let’s see.

[apostasie]

So, what about having the CI automatically comment on PRs with the list of failures, and matching that with known open issues for flaky tests?

👍
Is there any existing tool that we can use for this?

Exploring. Github API might be enough if this is small enough in scope, but let’s see.

^ bummer - this is just a no-go.
The token for PRs from forks is explicitly restricted to not allow issues: write, and using a PAT would have heinous consequences (issues: write does allow far too much to be safe).

The only realistic approach is to create a Github app. This is a bit heavy-handed and would require maintaining infrastructure for it + would require installation in the org - which might face pushback.

The best I can do right now is to enrich the PR summary or maybe abuse annotations (<- need to explore that last aspect).

On the other hand, when we merge-on-main, the GH token likely has the right permissions, so, I should be able to build that dashboard and write it inside an issue.