I am in favor of requiring precommit green before merge. To make this better, we should:
(1) run fewer irrelevant tests, for example almost every language-based presubmit tests vastly more code than could possibly be affected. (2) run more relevant tests, for example when an IO is touched running slightly more heavyweight integration tests before merge is reasonable. I worry that requiring total line coverage in unit tests / presubmits will encourage mock-based tests that don't demonstrate any meaningful correctness. But I am OK to try it. I assume we would have a way to override it in case the repo gets into a bad state (for example needing to modify the workflow definitions in order to get them to pass). Kenn On Mon, Jun 27, 2022 at 2:58 PM Ahmet Altay <al...@google.com> wrote: > Just an opinion: If we have a precommit (including codecov) we should be > willing to enforce those passing before merging a PR. Selectively applying > when/which precommits would be blockers eventually leads to more opinions. > I think codecov is actually providing value by checking that delta coverage > is not dropped. In my experience, for PRs that fail the codecov precommit > there tends to be an opportunity for adding unit tests. > > On Mon, Jun 27, 2022 at 2:14 PM Danny McCormick <dannymccorm...@google.com> > wrote: > >> Brian, you keep beating my emails by an instant :) I think codecov >> provides value independent of blocking a PR - it is a good sanity check >> (e.g. if codecov drops from 50% to 5%) and it gives us a nice metric to >> track over time. >> >> On Mon, Jun 27, 2022 at 5:12 PM Danny McCormick < >> dannymccorm...@google.com> wrote: >> >>> Yeah, codecov is the one exception I had in mind, there may be others >>> that are reasonable to exclude as well >>> >>> On Mon, Jun 27, 2022 at 4:59 PM Jack McCluskey <jrmcclus...@google.com> >>> wrote: >>> >>>> +1 for enforcing some precommit checks as required, but making them all >>>> required would be a bit of a nightmare with how Codecov works. Any Python >>>> or Go patch gets flagged as failing codecov if its changes aren't covered >>>> at or above the current overall coverage level at master (around 75% right >>>> now.) This is a nice goal, but ultimately not every change lends itself to >>>> that level of unit testing. But I would be in favor of things like the >>>> various ValidatesRunner suites being required. >>>> >>>> Thanks, >>>> >>>> Jack McCluskey >>>> >>>> On Mon, Jun 27, 2022 at 4:51 PM Danny McCormick < >>>> dannymccorm...@google.com> wrote: >>>> >>>>> Brian beat me by a few seconds, but I actually agree that there is a >>>>> problem here - when Damon's PR was submitted there was basically no way of >>>>> knowing that checks would break without having detailed knowledge of how >>>>> the build system works (so I'm on board with your >>>>> Subject/Background/Assessment). With that said, I disagree with the >>>>> recommended path forward - our automation failed us here and I don't think >>>>> the correct response is to make our manual validation better. That shifts >>>>> the burden of verifying correctness from robots (easy to validate, >>>>> basically free) to humans (expensive, forgetful, and hard to validate, >>>>> especially for newer contributors). Instead, I think we should fix the >>>>> underlying technical problems, which are IMO: >>>>> >>>>> 1) Flaky tests unrelated to the PR caused it to fail >>>>> 2) There were no guardrails in place to avoid merging a PR that wasn't >>>>> fully validated. >>>>> >>>>> Things that would've made this better which I think we should focus >>>>> our attention on: >>>>> >>>>> 1) (easy) Split the linting task out from the test step. >>>>> 2) This one is more controversial, but I would vote we start enforcing >>>>> some (or all?) precommit checks as required. I don't really blame anyone >>>>> for merging this PR since it had repeatedly run into unrelated flakes, but >>>>> that's kinda the point - we need to feel some of that pain to avoid >>>>> accidentally merging bad code. Blocking PRs on failed precommit suites >>>>> would force us to deal with these kinds of flakes and incentivize a lot of >>>>> good behavior in the repo (and disincentivize bad tests). >>>>> >>>>> Thanks, >>>>> Danny >>>>> >>>>> On Mon, Jun 27, 2022 at 4:48 PM Brian Hulette <bhule...@google.com> >>>>> wrote: >>>>> >>>>>> Hi Damon, >>>>>> I think our CI system was working as intended for your PR. We select >>>>>> which PreCommits to run based on the files that are edited, and the >>>>>> author/reviewer should collaborate to get those green before merging the >>>>>> PR. Sometimes that will involve inspecting the logs in Jenkins to find >>>>>> the >>>>>> failing gradle tasks and run them locally to reproduce. >>>>>> >>>>>> It looks like what broke down in this case was that the PR was >>>>>> accidentally merged despite the failing Java PreCommit run. >>>>>> >>>>>> Brian >>>>>> >>>>>> On Mon, Jun 27, 2022 at 12:02 PM Damon Douglas < >>>>>> damondoug...@google.com> wrote: >>>>>> >>>>>>> I write this to prevent a recent experience I encountered >>>>>>> contributing to the Apache Beam repository. To address a system issue, >>>>>>> I >>>>>>> write in SBAR >>>>>>> <https://www.hopkinsmedicine.org/antimicrobial-stewardship/nursing-toolkit/_docs/sbar-communication-tool.pdf> >>>>>>> format for clarity. If there are no objections to the recommendation, >>>>>>> I'm >>>>>>> happy to fulfill the first recommendation i.e. create GitHub issues. >>>>>>> >>>>>>> *Subject* >>>>>>> >>>>>>> I submitted a PR <https://github.com/apache/beam/pull/21953> that >>>>>>> was subsequently reverted >>>>>>> <https://github.com/apache/beam/pull/22044>. >>>>>>> >>>>>>> *Background* >>>>>>> >>>>>>> Despite passing the following gradle tasks, >>>>>>> the PreCommit_Java_Commit failed with a test unrelated to the code >>>>>>> in the PR >>>>>>> <https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/23066/testReport/junit/org.apache.beam.sdk.io.gcp.bigquery/BigQueryServicesImplTest/testInsertWithinRowCountLimits/>. >>>>>>> The PR was merged and then reverted due to discovered vendor dependency >>>>>>> issues and failed style checks. >>>>>>> >>>>>>> ./gradlew rat >>>>>>> ./gradlew spotlessCheck >>>>>>> ./gradlew sdks:java:io:google-cloud-platform:test >>>>>>> >>>>>>> *Assessment* >>>>>>> >>>>>>> Output from the following gradle commands did not surface prior to >>>>>>> the PR merge. >>>>>>> >>>>>>> ./gradlew sdks:java:io:google-cloud-platform:checkStyleMain >>>>>>> ./gradlew sdks:java:io:google-cloud-platform:checkStyleTest >>>>>>> >>>>>>> Both the aforementioned failing PreCommit_Java_Commit test and a >>>>>>> lack of knowledge contributed to the root cause. >>>>>>> >>>>>>> *Recommendation / Proposal* >>>>>>> >>>>>>> I propose updating PR template description >>>>>>> <https://github.com/apache/beam/blob/master/.github/PULL_REQUEST_TEMPLATE.md> >>>>>>> that >>>>>>> prescribes gradle tasks to be successfully executed prior to submitting >>>>>>> a >>>>>>> PR. Toward this aim: >>>>>>> >>>>>>> 1. Four separate GitHub issues in https://github.com/apache/beam >>>>>>> representative of *common*, and language specific: *Java*, *Python*, >>>>>>> and *Go*, prescribed gradle tasks >>>>>>> 2. Don't reply to this email with specifics on how; delegation of >>>>>>> comments/discussion within the relevant context of the GitHub issues in >>>>>>> 1 >>>>>>> 3. PRs that fix the GitHub issues in 1 >>>>>>> >>>>>>> -- >>>>>>> >>>>>>> >>>>>>> >>>>>>> *Damon Douglas* >>>>>>> >>>>>>> Strategic Cloud Engineer, Data & Analytics, Google Cloud >>>>>>> >>>>>>> damondoug...@google.com >>>>>>> >>>>>>
