I can speak to the breakage from https://github.com/apache/beam/pull/6151: here, the root cause was that an interface in an unmodified file was refactored in the interim.
On Fri, Sep 21, 2018 at 2:26 PM Ryan Williams <[email protected]> wrote: > > On Fri, Sep 21, 2018 at 5:08 PM Robert Burke <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 < >>>> [email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>> 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é < >>>>>>> [email protected]> 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 < >>>>>>>> [email protected] >>>>>>>> > <mailto:[email protected]>> 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é >>>>>>>> [email protected] >>>>>>>> http://blog.nanthrax.net >>>>>>>> Talend - http://www.talend.com >>>>>>>> >>>>>>> >>>
