This is great, and exactly solves the problem we were having. FindBugs will fail the compilation, but test flakes from another subproject can potentially mask the failures.
On Tue, Jan 29, 2019, 3:56 PM Michael Luckey <[email protected] wrote: > 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 > [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 > > >
