You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Alex Amato <aj...@google.com> on 2019/01/24 01:38:53 UTC
Precomit broken due to style violation. Why are failures getting past precommit?
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
Re: Precomit broken due to style violation. Why are failures getting
past precommit?
Posted by Scott Wegner <sc...@apache.org>.
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 <michael.j.luckey@gmail.com
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 <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 <aj...@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
>
>
>
Re: Precomit broken due to style violation. Why are failures getting
past precommit?
Posted by Michael Luckey <mi...@gmail.com>.
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 <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 <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 <ajamato@google.com <ma...@google.com>> 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>
Re: Precomit broken due to style violation. Why are failures getting
past precommit?
Posted by Kenneth Knowles <kl...@google.com>.
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 <aj...@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
>
Re: Precomit broken due to style violation. Why are failures getting
past precommit?
Posted by Scott Wegner <sc...@apache.org>.
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 <aj...@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