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 <[email protected]> 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
