I strongly support making flakes more painful to get the incentives right. We can control the "blast radius" of precommit flakes by more focused test suites. Postcommit they still need triage and deflake.
Kenn On Thu, Jan 24, 2019 at 8:56 AM Scott Wegner <sc...@apache.org> wrote: > Yes, you're correct that PR#7571 [1] had a checkstyle violation when > merged. I didn't notice the checkstyle failure and I hit the merge button. > Sorry about that. > > Here's where I went wrong: > * The precommits showed one failing: Java. GitHub shows the status as a > green check or red X on the head commit for a PR [2]. > * Opening the Jenkins link [3], it shows "Test Result (1 failure/ +-0)", > and clicking on that shows the failing test was testMatchWatchForNewFiles > [4] > * I recognized this as BEAM-6491 [5] and decided not to block the PR since > it should be unrelated. So I hit merge. > > What I didn't do was click on the Gradle Build Scan link [6], which shows > that :beam-runners-direct-java:checkstyleMain failed as well. > > I take the blame for letting this in, and I'll follow-up to make sure it > gets fixed. I also think that this is an easy mistake to make. Some ideas > to decrease the chances: > > a) Split out checkstyle and other static analysis as a separate pre-commit > from running tests. Because Jenkins reports the test report separately and > more prominently, it's easy to miss other failures. We already split out > Spotless and Rat, which also provides the value of giving a faster signal > on those checks. > > b) Have a stronger policy about not merging when tests are red. I just > checked and this is actually the policy already [7], but exceptions are > regularly made for flaky or unrelated test failures (evidence: each red X > on the master commit history [8]). Right now it's up to a human to make the > call on, and us humans are prone to make mistakes. We could consider > enforcing the policy and configure GitHub to require all checks passing > before merge. This will make flaky tests more painful, though it will also > provide a stronger incentive to fix the flaky tests. > > [1] https://github.com/apache/beam/pull/7571 > [2] https://github.com/apache/beam/pull/7571/commits > [3] https://builds.apache.org/job/beam_PreCommit_Java_Phrase/577/ > [4] > https://builds.apache.org/job/beam_PreCommit_Java_Phrase/577/testReport/ > [5] https://jira.apache.org/jira/browse/BEAM-6491 > [6] https://scans.gradle.com/s/s3wdusaicauee > [7] https://beam.apache.org/contribute/precommit-policies/#pull-requests > [8] https://github.com/apache/beam/commits/master > > On Wed, Jan 23, 2019 at 5:39 PM Alex Amato <ajam...@google.com> wrote: > >> See: https://issues.apache.org/jira/browse/BEAM-6500 >> >> I think that this PR introduced the issue. Though I am not sure how to >> read the test status. It looks like its marked with an X for the postcommit >> status, but presumably the precommit was okay even though java precommit >> appears to be broken now? Is there any explanation as to how this PR got >> through? >> https://github.com/apache/beam/pull/7571 >> <https://www.google.com/url?q=https://github.com/apache/beam/pull/7571&sa=D&source=hangouts&ust=1548379913098000&usg=AFQjCNGv-fMKxEe4SK7EoZmH5vZoSHx_DA> >> >> Today there have been numerous issues with presubmit. I would just like >> to understand if there is some underlying issue going on here missing some >> checks when we merge. Any ideas why this keeps occurring? >> >> Thanks >> Alex >> > > > -- > > > > > Got feedback? tinyurl.com/swegner-feedback >
