Continuous Testing of Git on Windows

Hi team,

at the GitMerge conference, I heard a couple of times "I do not bother reporting bugs in `pu` or `next` on MacOSX and Windows anymore, only Linux seems to be truly supported" or some variation thereof.

This is a strong indicator that we have some room for improvement here.

Active readers of the Git mailing list will not be surprised to read that I think we have to react to build/test failures quicker, that it is not enough to declare it okay for those integration branches to fail the test suite or even to fail to build[*1*].

In that vein, I worked quite a bit on "Continuous Integration", or more appropriately: Continuous Testing. That is, I created an ensemble of jobs that build & test the four integration tests of upstream Git (`maint`, `master`, `next` and `pu`) to verify the *basic* validity of their respective current revisions.

Most CI integrations these days require custom configuration files to be committed, and certain knobs to be twisted on GitHub (which I cannot turn because I have no special privileges on git/git). After struggling with making it work *somehow* anyway (even trying to get in touch with Travis, but they have not bothered to reply to my requests in over a year...), I decided to go with the Visual Studio Team Services (or short, VSTS; it does come in handy that it is developed by distant colleagues of mine, so they *have* to reply to my requests) where the CI configuration can be separated from the source code.

The entire setup is a little bit more complex than your grandfather's CI setup: it has to orchestrate five separate Git repositories, two of them generated and updated from live 32/64-bit Git for Windows SDKs, using a custom pool of build agents due to high resource demands, using three separate Git for Windows installations to support 32/64-bit as well as updating git.exe via `git pull`[*2*].

There is currently only one downside to that setup: the ability to have publicly accessible build logs on VSTS is still in development.

This is not *such* a big downside: if the MacOSX/Linux CI based on Travis[*3*] is any indicator, few people, if any, give a flying, fornicating fly about public build logs.

However, we should strive to improve our software development practices, and one such well-known Best Practice is to use Continuous Testing more effectively, i.e. *not* to ignore it.

That is why I taught the Git for Windows CI job that tests the four upstream Git integration branches to *also* bisect test breakages and then upload comments to the identified commit on GitHub. See an example here (the identified breakage seems to have disappeared in the meantime):

https://github.com/git/git/commit/5a12b3d76973#commitcomment-20802488

The code that generates this comment can be seen here:

https://github.com/git-for-windows/build-extra/blob/50c392c7d107/please.sh#L1648-L1665

So here is hoping to a quicker turnaround from breakage to fix in the future!

Ciao, Johannes

P.S.: I realize that these commit comments may *still* be ignored, but I simply was not yet ready to annoy everybody by having automated mails sent out.

Footnote *1*: It would be kind of okay if, say, `pu` would simply pick up *all* patches so that they would not be forgotten. But that is not the case. Even worse: it was stated recently that the expectation is that the *submitters* of patches find bugs in their code, that the patches should essentially be bug-free by the time they were submitted. This reasoning falls flat on its face considering the very real failures, of course, demonstrating our dear need for Continuous Testing.

Footnote *2*: I will describe the entire setup, including links to the involved repositories, in a separate mail at a later stage.

Footnote *3*: Look at https://travis-ci.org/git/git/builds, and be happy if you have a red/green deficiency so you cannot see all that red.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Good. I do not think it is useful to try 'pu' as an aggregate and expect it to always build and work [*1*], but your "bisect and pinpoint" approach makes it useful to identify individual topic that brings in a breakage. I wouldn't be surprised if original submitter and I were the only two people who actually compiled the patches on a topic in isolation while a topic is in 'pu', and chances are that these two people didn't try their builds on Windows. A CI like this one will help the coverage to stop premature topics from advancing to 'pu' without getting any Windows exposure.

Thanks.

[Footnote]

*1* The reason why topics not in 'next' but in 'pu', especially the
    ones merged near the tip of 'pu', exist in 'pu' are because they
    are interesting enough and could be polished to become eligible
    for 'next' but known to be premature for 'next' yet.  They are
    there primarily to give human contributors an easier way to
    download them as a whole and help polish them.  And I have to be
    selective when I queue things on 'pu'; it is not like I have
    infinite amount of time to pick up any cruft that is sent to the
    list.

Hi,

On Mon, 13 Feb 2017, Junio C Hamano wrote:

Sadly the many different merge bases[*1*] between `next` and `pu` (which are the obvious good/bad points for bisecting automatically) bring my build agents to its knees. I may have to disable the bisecting feature as a consequence.

Ciao, Johannes

Footnote *1*: There are currently 21, some of which stemming back from a year ago. For bisecting, they all have to be tested individually, putting a major dent into bisect's otherwise speedy process.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Probably a less resource intensive approach is to find the tips of the topics not in 'next' but in 'pu' and test them. That would give you which topic(s) are problematic, which is a better starting point than "Oh, 'pu' does not build". After identifying which branch is problematic, bisection of individual topic would be of more manageable size.

  $ git log --first-parent --oneline 'pu^{/^### match next}..pu'

will you the merges of topics left outside 'next'. I often reorder to make the ones that look more OK than others closer to the bottom, and if the breakages caused by them are caught earlier than they hit 'next', that would be ideal.

This is one of these times I wish "git bisect --first-parent" were available. The above "log" gives me 27 merges right now, which should be bisectable within 5 rounds to identify a single broken topic (if there is only one breakage, that is).

On Tue, Feb 14, 2017 at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah, this is a bug in the bisect algorithm. Fixing it is in the GSoC 2017 Ideas.

It is still probably more resource intensive than it couls be.

[...]

Implementing "git bisect --first-parent" is also part of the GSoC 2017 Ideas.

By the way it should not be very difficult as a patch to do this and more was proposed a long time ago:

https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/

Christian Couder <christian.couder@gmail.com> writes:

Thanks for a link. The one I found most interesting in the thread is by Avery [*1*], where he explains why "first-parent" bisection makes sense in "many people develop topics of their own, and they are aggregated into an integration branch" environment:

    Basically, we push/fetch *all* the branches from *everybody* into a
    single repo, and build all of them as frequently as we can.  If you
    think about it, if you have all the branches that someone might have
    pulled/merged from, then you don't have to think of the git history
    as a whole complicated DAG; you can just think of it as a whole
    bunch of separate chunks of linear history.  Moreover, as long as
    people are careful to only pull from a branch when that branch is
    passing all tests - which you can easily see by looking at the
    gitbuilder console - then playing inside each of these chunks of
    linear history can help you figure out where particular bugs were
    introduced during "messy" branches.

    It also allows you a nice separation of concerns.  The owner of the
    mainline branch (the "integration manager" person) only really cares
    about which branch they merged that caused a problem, because that
    person doesn't want to fix bugs, he/she simply wants to know who
    owns the failing branch, so that person can fix *their* bug and
    their branch will merge without breaking things.

[Reference]

*1* https://public-inbox.org/git/AANLkTinwbm9gcZhGeQCbOEPov0_xV7uJyQvC7J13qO15@mail.gmail.com/

From: "Christian Couder" <christian.couder@gmail.com>

There are also a few ideas at the SO answers: http://stackoverflow.com/a/5652323/717355

-- Philip

"Philip Oakley" <philipoakley@iee.org> writes:

I vaguely recall that I saw somebody said the same "mark tips of topics as good" on the list and answered with why it does not quite work, though.

From: "Junio C Hamano" <gitster@pobox.com>

I think you may mean https://public-inbox.org/git/7v8vyam5la.fsf@alter.siamese.dyndns.org/

I think we are thinking of opposite abstractions.

For regular bisect, the assumption (to a first order) is that there is a single point of infection of a single persistent bug with a well defined test, and that the goal is to find the point of first infection, as all other incidents of the bug are in successor commits, which are all infected. The fail-fix-break again sequence you mentioned in that thread is to my mind a red herring as it contradicts the normal bisection assumptions (but see below).

In the next..pu case the abstraction is in the other direction, we have potentially multiple points of infection (from feature branches), and a broad test (the whole test suite). In this case I believe we would like to investigate initially the --first-parent line with a classic bisect for the first point of failure (obviously including feature branch merges). This would identify which feature merge, or regular commit, created the first breakage.

Once the first point of failure has been identified, for the next..pu case, each of the post-fail second parents of merge commits _could_ then also be checked (which is a linear search, not a bisection), to identify any additional feature branches that need attention. This second stage search would probably be an option, but if the merging sequence onto pu is generally from good to bad, then the search is likely to be short. At least for a CI system this 2nd stage could provide useful feedback to the authors of their mistakes...

I haven't looked back at the actual patches in that thread, so they may not have followed my expectation of the --multi-bug (TM) search algorithm. --

Philip

"Philip Oakley" <philipoakley@iee.org> writes:

If you are going first-parent, you would limit the bisection to a single-strand-of-pearls, and I agree that it is a good strategy to find which topic branch merge broke the tip of 'pu'.

If we assume that there is no funny interaction among topics that cancel a breakage brought in by one topic with another breakage by another topic, then no matter how many broken topics there are, I agree that we would get to the first broken topic.

A good thing that comes once we assume that topics are more-or-less independent is that we could rebuild 'pu' minus the broken topic identified by the above procedure and repeat it to find other broken topics, still using the --first-parent bisection, because master..pu is a linear sequence of merges of individual topics.

[sorry for the repeated emails - I'd prepared it off line, and then suffered 
a number of auto send actions]
From: "Junio C Hamano" <gitster@pobox.com>

For an integrator, or especially a CI tool, simply checking the second parents of each topic merge (post fail) should at least indicate if the basics of the feature actually passed the tests, though it doesn't check for interaction issues. This could give direct author feedback! -- Philip

"Philip Oakley" <philipoakley@iee.org> writes:

I think that is essentially what Avery advocated for in the old thread Christian gave us earlier in the thread, and I agree that is a useful way to help contributors.

While we are on the topic of testing efficiently and effectively, there are a few other things worth taking advantage of.

One obvious thing is that not all topic branches get updated every day, even though the tip of 'pu' will be different every day (or on days when I run the integration cycle more than once, after every pushout), because we constantly acquire new topics or updated ones. The tips of topics that are merged in 'pu^{/^### match next}..pu' can be enumerated by following the first-parent chain, and they can be tested all one-by-one, excluding the ones that have been tested already in the previous runs. When I counted these merges the other day I had 27 of them, and as of this writing I have 20 of them (because some topics were merged to 'next' in the meantime). Among them, only 3 are new. Everything else would have already been tested if such a test was done daily. So from that point of view, testing them all could be less expensive than bisecting; in order to bisect a first-parent chain of 20 merges, you'd need to test 5 or so.

Another thing that may help to prevent breakages from seeping into 'next' is that "pu^{/^### match next}..pu" are rebuilt every day, and the order of topics that are merged are updated. The topics that are closer to be merged to 'next' are moved down, so that testing the merge result would give us closer result to what would testing 'next' in the near future would give us [*1*]. So paying closer attention to the merges (not tips of topics) above the commit marked as "pu^{/^### match next}" when testing would catch potential breakage about to happen in 'next' due to unexpected interactions of topics when merged together.

There is another point in the history that may be worth paying closer attention, which is the tip of 'jch'. This always is ahead of "pu^{/^### match next}", and almost always has a handful more topics, many of which are considered to be merged to 'next' in near future. The branch is meant to be at least stable enough for my personal use (hence its name) in helping me run everyday integration cycles.

[Footnote]

*1* Suppose there are three topics A, B and C that are not yet in
    'next', and 'pu' merges them in that order.  Further suppose
    that C is a lot better cooked than others.  Merging C directly
    to 'next' however can expose a hidden issue that changes C
    introduces alone is not sufficient and depends on A or B to be
    present.  To avoid such a surprise after merging C to 'next',
    when 'pu' is rebuilt, I try to reorder them so that C is merged
    first on top of an equivalent of 'next', and then A and B.  For
    the same reason, topics in 'master..pu^{/^### match next}' are
    also reordered so that ones that are planned to be merged to
    'master' soon comes at the bottom.

    One natural consequence of this is that 'pu' is rebuilt directly
    on top of 'master' and 'next' does not fast-forwared to 'pu'.
    This arrangement also helps to spot mismerges to 'next' and
    avoid the same mismerge to affect 'master' when topics are
    merged to it.

From: "Junio C Hamano" <gitster@pobox.com>

I think you may mean https://public-inbox.org/git/7v8vyam5la.fsf@alter.siamese.dyndns.org/

I think we are thinking of opposite abstractions.

For regular bisect, the assumption (to a first order) is that there is a single point of infection of a single persistent bug with a well defined test, and that the goal is to find the point of first infection, as all other incidents of the bug are in successor commits, which are all infected. The fail-fix-break again sequence you mentioned in that thread is to my mind a red herring as it contradicts the normal bisection assumptions (but see below).

In the next..pu case the abstraction is in the other direction, we have potentially multiple points of infection (from feature branches), and a broad test (the whole test suite). In this case I believe we would like to investigate initially the --first-parent line with a classic bisect for the first point of failure (obviously including feature branch merges). This would identify which feature merge, or regular commit, created the first breakage.

Once the first point of failure has been identified, for the next..pu case, each of the post-fail second parents of merge commits _could_ then also be checked (which is a linear search, not a bisection), to identify any additional feature branches that need attention. This second stage search would probably be an option, but if the merging sequence onto pu is generally from good to bad, then the search is likely to be short. At least for a CI system this 2nd stage could provide useful feedback to the authors of their mistakes...

I haven't looked back at the actual patches in that thread, so they may not have followed my expectation of the --multi-bug (TM) search algorithm. --

Philip

From: "Junio C Hamano" <gitster@pobox.com>

I think you may mean https://public-inbox.org/git/7v8vyam5la.fsf@alter.siamese.dyndns.org/

I think we are thinking of opposite abstractions.

For regular bisect, the assumption (to a first order) is that there is a single point of infection of a single persistent bug with a well defined test, and that the goal is to find the point of first infection, as all other incidents of the bug are in successor commits, which are all infected. The fail-fix-break again sequence you mentioned in that thread is to my mind a red herring as it contradicts the normal bisection assumptions (but see below).

In the next..pu case the abstraction is in the other direction, we have potentially multiple points of infection (from feature branches), and a broad test (the whole test suite). In this case I believe we would like to investigate initially the --first-parent line with a classic bisect for the first point of failure (obviously including feature branch merges). This would identify which feature merge, or regular commit, created the first breakage.

Once the first point of failure has been identified, for the next..pu case, each of the post-fail second parents of merge commits _could_ then also be checked (which is a linear search, not a bisection), to identify any additional feature branches that need attention. This second stage search would probably be an option, but if the merging sequence onto pu is generally from good to bad, then the search is likely to be short. At least for a CI system this 2nd stage could provide useful feedback to the authors of their mistakes...

I haven't looked back at the actual patches in that thread, so they may not have followed my expectation of the --multi-bug (TM) search algorithm. --

Philip

Hi Philip,

On Tue, 14 Feb 2017, Philip Oakley wrote:

Thanks for that link!

However, my main aim was not to get distracted into yet another corner of Git that needs to be fixed (I am on enough of those projects already).

I was merely surprised (and not in a good way) that a plenty ordinary bisect between `next` and `pu` all of a sudden tested a *one year old* commit whether it was good or not.

And I doubt that the strategy to mark all second parents of all merge commits in pu..next as "good" would work well, as the merge bases *still* would have to be tested.

I guess what I have to resort to is this: if I know that `next` tests fine, and that `pu` fails, I shall mark all merge bases as "good". I am sure this has its own set of pitfalls, undoubtedly costing me more time on that front...

But at least my cursory analysis of this idea seems to make sense: I use `next` essentially as a catch-all to verify that the breakage has entered `pu`, but not yet `next`. This reasoning makes sense, given that we know the waterfall topology of pu/next/master/maint: patches enter from left to right, i.e. anything that entered `pu` may later enter `next`, but not vice versa.

Ciao, Dscho

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>

I was expecting that if all the second parents were marked as good, then there would be no merge bases, as there shouldn't be a forked graph, just the linear string of pearls - if bisect doesn't do that then there's an failed optimisation to be had.

I don't see anything in the `git bisect --help` page that would indicate that the merges themselves are omitted from the bisection.

It may even be worth 'splitting' the pu branch sequence into the existing pu (with merges from series that are selected as reasonable), and then a pr branch (public review?) on top of that holding the rest of the series that have been submitted, so that the CI can do a full test on the tips of them to support those devs with limited test capability.

"Philip Oakley" <philipoakley@iee.org> writes:

I won't stop you from publishing such a pr branch yourself.

For patches whose merit is not clear because the problem they try to solve is under-explained, whose solution is ill-designed, etc., IOW, with issues that makes me judge that they are not interesting enough for 'pu', it is not worth my time to deal with whitespace brekages in them to make them not even apply, to figure out what base the patches are meant to apply to, or to resolve conflicts caused by them with topics already in flight, etc.

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Thursday, February 16, 2017 12:20 AM

But would others see it?... (rhetorical, better thoughts below))

It is reasonble that that a project's integrator is able to make these decisions. For some projects they may have a layered approach of descisions which does allow the gradations for the submitted feature series.

Some of this does fall into dscho's differentiation between those patch series that should pass the CI (continuous integration) testing, and those that are there for CT (continuous testing) feedback. This could either be an extra branch marking the transition, or a named commit similar to the "pu^{/^### match next}", etc. In some ways it is similar to my 'pr' suggestion, without the inclusion of the 'all and sundry' series.

For for integrators who are willing/want to recieve any/all contributions for public view (usually those for projects of a more lenient and less critical variety), then even the CT grouping could then have those additional pr submissions. For Git, you provide that that 'voice of reason' for gatekeeping the pu branch.

If a centralised CT service was available to the project, maybe via the GitHub PR process (which isn't curently used by the project) then is may be a way of allowing the 'all and sundry' contributors to get their ideas upto a basic level before even bothering yourself (because PRs do not trouble you).

It may need an extra gatekeeper between the passing patches (PRs) and auto submission (the Heroku script thingy) which could flood the list with with inane changes - one only has to look at the http://stackoverflow.com/questions/tagged/git stream to see that.

At least if there was a break point within pu that allowed differentiation between the series that should fit a CI view, and those that are still at the CT stage, then that may help.

Thanks

Philip.

Hi Christian,

On Wed, 15 Feb 2017, Christian Couder wrote:

Indeed.

Ciao, Dscho