In the interest of moving things forward, here is what we would need (in
my opinion) to start enforcing this:
1. Get overall build failure percentage under a certain threshold
1. What is an acceptable number here?
2. How do we achieve this: wait until all of them are fixed,
disable everything that's flaky right away, etc
2. Come up with concrete policy rules so there's no confusion. I think
we need to agree on answers for these questions at least:
1. What happens if a new PR introduces a new test that is revealed to
be flaky?
2. What happens if a new PR makes an old test become flaky?
3. How long do we have to take action in the above cases?
4. How do we track failures?
5. How often does a test need to fail to be considered flaky enough
to take action?
Here's my take on these questions, but would love to hear from others:
1.1) Imo the failure rate has to be under 50% at the very highest. At 50
we still have half the builds failing, but 75% would pass after just one
retry, and 87.5% after two retries. Is that acceptable? Maybe we should aim
to kick things off with a higher success rate to give ourselves some wiggle
room over time, and have 50% be the absolute maximum failure rate -- if it
ever gets beyond that we trigger an emergency response (whether that be
blocking all feature work until the test failures are addressed, ending
this policy, etc)
1.2) I think we'd probably all agree that there's no way we'll be able to
triage all of the currently flaky tests in a reasonable time frame, but I'm
also wary of just disabling everything and forgetting them. Should we
consider making all of the currently-disabled tests a 3.7 release blocker?
Should we wait until after 3.7 to disable them and implement this policy to
give us a longer time to address them? Will we just procrastinate until the
next release deadline anyways? Many open questions here
2.1) We can either revert the entire PR or just disable the specific
test(s) that are failing. Personally I'm in favor of giving a short window
(see point #3) for the author to attempt a fix or else disable the specific
failing test, otherwise we revert the entire commit. This should guarantee
that tests aren't just introduced, disabled, and never looked at again. My
main concern here is that sometimes reverting a commit can mean resolving
merge conflicts that not everyone will have the context to do properly.
This is just another reason to keep the window short.
2.2) Again I think we should allow a (short) window of time for someone
with context to triage and take action, but after that, we need to either
revert the PR or disable the test. Neither of those feels like a good
option, since new changes can often induce timeout-related failures in my
experience, which are solely a problem with the test and not with the
change. Maybe if the test is failing very frequently/most of the time, then
we should revert the change, but if it becomes only occasionally flaky,
then we should disable the test, file a ticket, and mark it a blocker until
it can be triaged (the triaging can of course include demoting it from a
blocker if someone with context determines the test itself is at fault and
offers little value).
2.3) I think any longer than a day (or over the weekend) starts to become
an issue. Of course, we can/will be more lenient with tests that fail less
often, especially since it might just take a while to notice
2.4) Presumably the best way is to file tickets for every failure and
comment on that ticket for each subsequent failure. I used to do this quite
religiously, but I have to admit, it was actually a non-trivial timesuck.
If we can get to a low enough level of failure, however, this should be
more reasonable. We just need to trust everyone to pay attention and follow
up on failures they see. And will probably need to come up with a system to
avoid overcounting by different reviewers of the same PR/build
2.5) This one is actually the most tricky in my opinion. I think everyone
would agree that if a test is failing on all or most of the JDK runs within
a single build, it is problematic. And probably most of us would agree that
if it fails even once per build on almost all PRs, we need to take action.
On the other hand, a single test that fails once every 50 builds might feel
acceptable, but if we have 100 such tests, suddenly it's extremely
difficult to get a clean build. So what's the threshold for action?
So...any thoughts on how to implement this policy? And when to actually
start enforcing it?
On Tue, Nov 21, 2023 at 1:18 PM Sophie Blee-Goldman <sop...@responsive.dev>
wrote:
So...where did we land on this?
To take a few steps back and make sure we're all talking about the same
thing, I want to mention that I myself was responsible for merging a PR
that broke the build a few weeks ago. There was a warning that only
appeared in some of the versions, but when checking on the results I
immediately navigated to the "Tests" page before letting the main build
page load. I just assumed that if there were test failures that must mean
the build itself was fine, and with this tunnel-vision I missed that a
subset of JDK builds had failed.
I'm mentioning this now not to make excuses, but because I really hope
that it goes without saying that this was an honest (though sloppy)
mistake, and that no one genuinely believes it is acceptable to merge any
PR that causes any of the builds to fail to compile. Yet multiple people in
this thread so far have voiced support for "gating merges on the
successful completion of all parts of the build except tests". Just to be
totally clear, I really don't think that was ever in question -- though it
certainly doesn't hurt to remind everyone.
So, this thread is not about whether or not to merge with failing *builds,
*but it's whether it should be acceptable to merge with failing *tests.* It
seems like we're in agreement for the most part that the current system
isn't great, but I don't think we can (or rather, should) start enforcing
the new rule to only merge fully-green builds until we can actually get to
a reasonable percentage of green builds. Do we have any stats on how often
builds are failing right now due to flaky tests? Just speaking from
personal experience, it's been a while since I've seen a truly green build
with all tests passing.
Lastly, just to play devil's advocate for a moment, before we commit to
this I think we need to consider how such a policy would impact the
contributor experience. It's incredibly disheartening to go through all the
work of submitting and getting it reviewed only to be blocked from merging
at the last minute due to test failures completely outside that
contributor's control. We already struggle just getting some folks, new
contributors in particular, all the way through the review process without
abandoning their PRs. I do think we've been doing a better job lately, but
it's a cycle that ebbs and flows, and most community PRs still take an
admirable degree of patience and persistence just to get enough reviews to
reach the point of merging. So I just think we need to be careful not to
make this situation even worse by having to wait for a green build. I'm
just worried about our ability to stay on top of disabling tests,
especially if we need to wait for someone with enough context to make a
judgement call. Can we really rely on everyone to drop everything at any
time to check on a failing test? At the same time I would be hesitant to be
overly aggressive about reverting/disabling tests without having someone
who understands the context take a look.
That said, I do think it's worth trying this out as an experiment, as
long as we can be clear to frustrated contributors that this isn't
necessarily the new policy from here on out if it isn't going well.
On Thu, Nov 16, 2023 at 3:32 AM Igor Soarez <i...@soarez.me> wrote:
Hi all,
I think at least one of those is my fault, apologies.
I'll try to make sure all my tests are passing from now on.
It doesn't help that GitHub always shows that the tests have failed,
even when they have not. I suspect this is because Jenkins always
marks the builds as unstable, even when all tests pass, because
the "Archive JUnit-formatted test results" step seems to persistently
fail with "[Checks API] No suitable checks publisher found.".
e.g.
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-14770/1/pipeline/
Can we get rid of that persistent failure and actually mark successful
test runs as green?
--
Igor
