On Fri, Sep 21, 2018 at 5:08 PM Robert Burke <rob...@frantil.com> wrote:
> The issue is time from commit to merge, and without manual intervention, > commits from other PRs aren't accounted for, if there's a lag between LGTM > and merge. > it's not between LGTM and merge, it's between [last commit to the PR's branch] and merge, right? > > On Fri, Sep 21, 2018, 1:52 PM Ahmet Altay <al...@google.com> wrote: > >> I will suggest a rollback in this case, and in general as a good practice >> to unblock people. >> >> On Fri, Sep 21, 2018 at 1:02 PM, Charles Chen <c...@google.com> wrote: >> >>> Relatedly, https://github.com/apache/beam/pull/6151 also recently broke >>> the build ( >>> https://builds.apache.org/view/A-D/view/Beam/job/beam_PostCommit_Java_GradleBuild/1503/console) >>> because the Precommits were very out of date when merged. >>> >> >> Does not github change the test signals from green to yellow when new >> commits are added? >> > it does this if you add commits to your PR's branch, not if commits happen upstream. in the latter case, github only checks whether your PR can be merged cleanly (i.e. with no merge-conflicts) according to some line-diffing heuristic (presumably the same as git uses), but not whether the tests would still pass post-merge. the unwritten assumption is that a non-conflicting merge with upstream won't break any tests, but obviously there can be false-positives; thanks for documenting those cases, Valentyn and Charles. it would be nice to run precommits on potential post-merge commits, but i'd guess that our precommits take longer than the average time between upstream commits, so it would be hard to get anything merged that way! we'd need to get smarter about knowing exactly which tests need re-running when new commits happen upstream, and only re-run those tests, which we're pretty far from today. so, the possibility of this kind of breakage is always present, unfortunately. i would be curious to see exactly how the line-diffing heuristic was fooled in these cases. an easy way to construct such a case is to have one side rename a variable, and another side add a new reference to the variable's old name, in a newly-created file; each side can touch a disjoint set of files and have all its tests pass in isolation, and git{,hub} will let you merge them, but the tests will fail post-merge. > >> >> >>> >>> On Fri, Sep 21, 2018 at 12:50 PM Valentyn Tymofieiev < >>> valen...@google.com> wrote: >>> >>>> The change https://github.com/apache/beam/pull/6424 was not deemed >>>> particularly risky, and it's purpose was adding more tests to precommit >>>> test suite. >>>> There was a green Precommit signal on Jenkins, and I believe Postcommit >>>> test suite (at the same time) wouldn't catch this. >>>> >>>> The reason the breakage was introduced is that >>>> https://github.com/apache/beam/commit/7689f12db5 was committed after >>>> the PR 6424 was reviewed, but before it was merged. A combination of both >>>> introduced the breakage. >>>> >>>> Had we re-run the tests closer to the merge, we should have caught >>>> this. Can we automatically re-run precommits tests at merge time, when >>>> some of the files a PR is touching have changed since last precommit run? >>>> >>>> I suggest we proceed with https://github.com/apache/beam/pull/6464 or >>>> revert https://github.com/apache/beam/pull/6424 in the mean time, >>>> while we are iterating on the fix. >>>> >>>> On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <c...@google.com> wrote: >>>> >>>>> Do we happen to know the root cause for why this wasn't caught during >>>>> review / precommit? >>>>> >>>>> In the future, can we run manually run postcommits for risky changes >>>>> like these? That is, trigger it by commenting "Run Python PostCommit"? >>>>> >>>>> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pabl...@google.com> >>>>> wrote: >>>>> >>>>>> Robbe has prepared a better fix on >>>>>> https://github.com/apache/beam/pull/6465 >>>>>> Hopefully that'll be the last of this breakage : ) >>>>>> -P. >>>>>> >>>>>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>>>>> wrote: >>>>>> >>>>>>> By the way, it fails for me on my machine as well. >>>>>>> >>>>>>> Regards >>>>>>> JB >>>>>>> >>>>>>> On 21/09/2018 18:10, Pablo Estrada wrote: >>>>>>> > I investigated. This failure comes from the activation of >>>>>>> > apache_beam.pipeline_test in Python 3 unit tests. >>>>>>> > >>>>>>> > I have https://github.com/apache/beam/pull/6464 out to fix this. >>>>>>> > >>>>>>> > In any case, it's very exciting that we have some unit tests >>>>>>> running on >>>>>>> > Py3 : ) >>>>>>> > Best >>>>>>> > -P. >>>>>>> > >>>>>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <m...@apache.org >>>>>>> > <mailto:m...@apache.org>> wrote: >>>>>>> > >>>>>>> > Hi, >>>>>>> > >>>>>>> > The Python PreCommit tests are currently broken. Is anybody >>>>>>> looking >>>>>>> > into >>>>>>> > this? >>>>>>> > >>>>>>> > Example: >>>>>>> > >>>>>>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink >>>>>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458 >>>>>>> > >>>>>>> > I'm sure this was just an accident. No big deal but let's make >>>>>>> sure >>>>>>> > PreCommit passes when merging. A broken PreCommit means that >>>>>>> we can't >>>>>>> > merge any other changes with confidence. >>>>>>> > >>>>>>> > Thanks, >>>>>>> > Max >>>>>>> > >>>>>>> >>>>>>> -- >>>>>>> Jean-Baptiste Onofré >>>>>>> jbono...@apache.org >>>>>>> http://blog.nanthrax.net >>>>>>> Talend - http://www.talend.com >>>>>>> >>>>>> >>
