As currently we miss prominent exposures of check style warnings, I added another post build action to publish check style and findbugs warnings. [1]
This will lead to - show check style/findbugs status right on the status page - links to followup pages with some graphs This could help to not hide those warnings. Wdyt? Of course, if we find this useful, we might extend to other precommits and/or other tools. E.g. findbugs might be useless anyway, as error prone will fail compile anyway? [1] https://github.com/apache/beam/pull/7666 > On 24. Jan 2019, at 17:56, Scott Wegner <[email protected]> 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 > <https://github.com/apache/beam/pull/7571> > [2] https://github.com/apache/beam/pull/7571/commits > <https://github.com/apache/beam/pull/7571/commits> > [3] https://builds.apache.org/job/beam_PreCommit_Java_Phrase/577/ > <https://builds.apache.org/job/beam_PreCommit_Java_Phrase/577/> > [4] https://builds.apache.org/job/beam_PreCommit_Java_Phrase/577/testReport/ > <https://builds.apache.org/job/beam_PreCommit_Java_Phrase/577/testReport/> > [5] https://jira.apache.org/jira/browse/BEAM-6491 > <https://jira.apache.org/jira/browse/BEAM-6491> > [6] https://scans.gradle.com/s/s3wdusaicauee > <https://scans.gradle.com/s/s3wdusaicauee> > [7] https://beam.apache.org/contribute/precommit-policies/#pull-requests > <https://beam.apache.org/contribute/precommit-policies/#pull-requests> > [8] https://github.com/apache/beam/commits/master > <https://github.com/apache/beam/commits/master> > > On Wed, Jan 23, 2019 at 5:39 PM Alex Amato <[email protected] > <mailto:[email protected]>> wrote: > See: https://issues.apache.org/jira/browse/BEAM-6500 > <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 > <https://tinyurl.com/swegner-feedback>
