Add "virtual" merge tests

[mantognini]

As discussed here:

[...] our CI would, in addition the existing tests, test the code after a local merge of the branch into master and, if any of the tests fail, the CI bot could emit a message on GitHub with the error details.

The first part of the task consists in implementing those additional tests.

[LaurentGomila]

What kind of tests? Are we talking about writing unit tests for SFML?

[mantognini]

No, this is orthogonal to unit tests. I meant adding an additional phase to the buildsalves that: 1. merge the change into the working copy's master branch. 2. run the tests (or a subset for quicker feedback) again.

…

On Thu, 5 Apr 2018, 08:08 Laurent Gomila, ***@***.***> wrote: What kind of tests? Are we talking about writing unit tests for SFML? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAqU6_awjhe7G20qpJowX--Vy4d3_mhpks5tlbTigaJpZM4TH5LO> .

[eXpl0it3r]

I don't really get what is meant with this.

At most a PR will be off by just a few commits from master and in most cases the changes are unrelated, as such having CI run on the master and on the PR is good enough, we really don't need "what if we merge this right now"-CI runs.

If you're concerned about your PRs state in relation to the master branch, you can simply rebase.

I can only see potential issues when you're change fundamental things in the build system, which is a very rare case. Or when to people work on the same piece of code, which usually leads to merge conflicts and people just have to sync with each other anyways.

Ah yeah, we don't have any tests right now, so I'm not sure what tests you want to run. 🤔

[LaurentGomila]

No, this is orthogonal to unit tests.

run the tests

So I still don't know what "tests" you're talking about.

[binary1248]

Just guessing: @mantognini could mean "test the buildability after the merge". At the moment, GitHub already provides information at the bottom of a PR whether it can be merged cleanly without any conflicts. This still doesn't tell you if the merged state will be able to be built by CI. In most cases, where there are no merge conflicts anyway, this should never be a problem. It has only gained a bit of short-term attention recently because large changes to the CMake configuration were made that "broke" other branches in the sense that we knew it was OK but until it was merged CI would always fail builds.

Like @eXpl0it3r said, I don't see what we are supposed to gain long-term by implementing something like this. If a PR author intends their patch to be based on some other open PR or knows it might be affected by something that was recently merged, it is their responsibility as PR author to check for themselves if everything is still good. Sure... CI can notify people of potential problems, but it won't solve the problem that the PR author will have to fix it themselves anyway. Authors have to stay active, with or without CI support.

[Ceylo]

https://en.wikipedia.org/wiki/Gated_commit

The interest is that PR authors NEVER have to rebase on master unless there is a conflict or unless an issue is reported by the CI that is run on gated commit. Whatever the change that happened in the meantime on master. Without any human support.

This is useful only if you require PRs to be rebased on master before considering the CI results to be valid though. Isn't it currently the case?

NB: I'm surprised this has to be "implemented", doesn't buildbot support this already?

Edit: as for how it looks because it didn't look clear what kind of "test" it is, here is an example: Yalir/sfeMovie#105 (click on "View details" button after the commit list). You can see the "branch" result and the "pr" result.

[mantognini]

@binary1248 yes, the author has to be active. But with this we don't have to manually check every PR and report issue to the author. It would be done automatically.

It seems to come at little to no cost and only provides benefits. Why be so negative about this?

[binary1248]

@Ceylo This is already what is happening now (see last paragraph in that article). The fact that we rebase and fast-forward merge instead of simply merge via merge commit is really just a personal preference.

GitHub provides "special" merge refs that CI can build against to pre-test the buildability of PRs before they are merged into master, and this is the special commit sfeMovie CI makes use of. This is only true for the "merge commit workflow". At the current time GitHub does not provide a rebase ref that CI can use, which would mean we would have to manually create it when building on CI and discard it when CI is done. How are we going to report the CI status of this locally created commit? GitHub will not recognize it because it is not tracked by the system, so we won't get any nice summaries any where on the PR page.

If you really want statuses to be reported for the rebase flow we currently use, you will have to request this extension to GitHub first, there is no other way. Like I said, the only flow treated as a first class citizen by GitHub is the "merge commit flow", anything else is basically DIY and unsupported.

Also... there is the issue that CI hooks are only triggered on certain events, currently on pushes to any branch of the SFML main repository (we can't hook pushes to external repositories for obvious reasons). There is no hook that we can use to trigger rebuilds of all active PRs every time master HEAD changes. This will also probably never get implemented because it would mean repositories would end up building possibly hundreds of branches because of a small change that was merged at some point. Nobody will support this.

I've changed the SFML repository setting to only allow rebase merges, so the button at the bottom will only become green when a PR is up-to-date with master HEAD. This still doesn't change the fact that @eXpl0it3r can still rebase locally, push, close the PR as merged and delete the branch manually, which is basically how he works now. In effect, my changing of this setting only means that we have to annoy the PR author more often to keep their branch up-to-date with master in order to see the green button at the bottom of the PR.

[Ceylo]

The fact that we rebase and fast-forward merge instead of simply merge via merge commit is really just a personal preference.

What are the reasons for restricting to this workflow? If it's just for personal preference and it prevents you from using some (subjectively) useful features then just give up on forcing rebase + fast forward.

There is no hook that we can use to trigger rebuilds of all active PRs every time master HEAD changes. This will also probably never get implemented because it would mean repositories would end up building possibly hundreds of branches because of a small change that was merged at some point. Nobody will support this.

Ignore the branches that have conflict, and those whose CI build (without gated commit) already fails, and suddenly you get a quite reasonable amount of branches to build on each master update.

In effect, my changing of this setting only means that we have to annoy the PR author more often to keep their branch up-to-date with master in order to see the green button at the bottom of the PR.

Which is precisely what sucks at the moment.

[mantognini]

How are we going to report the CI status of this locally created commit? GitHub will not recognize it because it is not tracked by the system, so we won't get any nice summaries any where on the PR page.

Couldn't we report that on the same commit (HEAD of origin/, I guess) we already report things when a PR build is triggered?

[Ceylo]

I guess you'll find answers about the "how" in GitHub API doc:
https://developer.github.com/v3/repos/statuses/
https://blog.github.com/2014-12-08-see-results-from-all-pull-request-status-checks/
https://blog.github.com/2012-09-04-commit-status-api/

[binary1248]

Couldn't we report that on the same commit (HEAD of origin/, I guess) we already report things when a PR build is triggered?

So... if I understand you correctly, you intend to report a build status on a ref that is not what was actually built? How is this supposed to be a good idea? Sure... you could argue that this is a "workaround" for the fact that GitHub doesn't have a way to report build statuses for rebased PRs now, but misusing their system just so that we can have it our way also isn't right.

[eXpl0it3r]

Can someone tell me what the real benefit here is? What do we actually gain? For both the maintainers and the contributors?

[Ceylo]

Ok if nobody reads other people's answers we won’t go very far.

@binary1248 that’s why I suggest we check GitHub API spec to know what is the recommended way of doing this, because it seems like we want to do it ourselves and it seems like nobody checked if buildbot already can handle this.

@eXpl0it3r Please read again #15 (comment)

[eXpl0it3r]

This is useful only if you require PRs to be rebased on master before considering the CI results to be valid though. Isn't it currently the case?

No.

Ok if nobody reads other people's answers we won’t go very far.

See my first comment, there's no issue from my point of view, but seems my opinion is not popular. 😄

[Ceylo]

This is useful only if you require PRs to be rebased on master before considering the CI results to be valid though. Isn't it currently the case?

No.

In effect, my changing of this setting only means that we have to annoy the PR author more often to keep their branch up-to-date with master in order to see the green button at the bottom of the PR.

So I’m lost now 😄 Either you and binary1248 say the opposite or I totally misunderstood something.

Question is: I make a PR, CI is green for my branch, in the meantime many stuff is merged into master but not git conflict appears. Does my PR need any update before it can be merged?

[binary1248]

Question is: I make a PR, CI is green for my branch, in the meantime many stuff is merged into master but not git conflict appears. Does my PR need any update before it can be merged?

Once your branch can no longer fast-forward on top of master, GitHub will complain and block merging as it is configured now. CI will still stay green until another build is triggered. Even though CI is green, GitHub won't allow a merge until you rebase. Once you rebase CI will no longer be green. There is no scenario where you can merge just because CI stayed green, it is only one of 3 requirements for a merge. And I think it is no secret that the second requirement, manual human code review and testing is the part that takes the longest. Thus this whole discussion is not really achieving what you might hope to achieve.

[mantognini]

CI will still stay green until another build is triggered.

That is the original point of the requested feature: to trigger a build and turn the light to red if it is no longer passing our tests, automatically. Fewer manual interventions means a lower risk of mistakes. :)

(Although we are still addressing the why, apparently, according to Github doc on status this should be feasible.)

[binary1248]

That is the original point of the requested feature: to trigger a build and turn the light to red if it is no longer passing our tests, automatically. Fewer manual interventions means a lower risk of mistakes. :)

But exactly this is already happening. CI will always rebuild if anything about the PR code changes, whether the author amends it or rebases it. CI will not trigger because another branch (such as master) is updated. GitHub doesn't support this, and like I already said somewhere above, we don't want all 46 PRs to be rebuilt just because something was merged into master.

It's really simple:

1. Someone opens a PR, CI is triggered for it, it becomes green.
2. Some other PR gets merged in the mean time.
3. The PR in question's CI status stays green because nothing about the code changed.
4. Merging is however blocked because it isn't up-to-date with master any more.
5. In order to unblock merging, the author needs to rebase and push.
6. When they push CI will rebuild becoming green or red.
7. Repeat from step 1.

[Ceylo]

Once your branch can no longer fast-forward on top of master, GitHub will complain and block merging as it is configured now.

So only because of an artificial limitation you chose. Ok. Then

And I think it is no secret that the second requirement, manual human code review and testing is the part that takes the longest.

Which doesn't mean it's not worth avoiding wasted human time on other parts when possible.

CI will not trigger because another branch (such as master) is updated.

Right, I admit it doesn't look to be possible to go up to where I would have hoped.

we don't want all 46 PRs to be rebuilt

Those 46 PRs come from nowhere.

So fix me if I'm wrong: we could have PR (1) opened, have the CI build show that the branch is ok, have other PRs merged in the meantime, and then merge PR (1). And for no cost at all but changing a repo setting. But we don't, ONLY because of someone's personal preference? Instead we prefer to have the PR author waste time rebasing and asking SFML Core team to relaunch builds, which adds significant delays, am I correct?

Please tell me there are GOOD reasons to this wasted time.

[binary1248]

So only because of an artificial limitation you chose. Ok. Then

You might call it a limitation, but I would prefer the term development process. By the same token, you can call all forms of quality assurance artificial limitations as well, and yet they also have their purpose.

Which doesn't mean it's not worth avoiding wasted human time on other parts when possible.

I still don't understand where you see time as wasted. Time is never wasted as long as progress is made. The fact that you don't agree with how progress is made doesn't imply that time must be wasted somewhere.

Those 46 PRs come from nowhere.

I assume we are still talking about https://github.com/SFML/SFML? Because at the moment there are 46 open pull requests, and the GitHub API doesn't differentiate between them.

Instead we prefer to have the PR author waste time rebasing and asking SFML Core team to relaunch builds, which adds significant delays, am I correct?

The fact that externally sourced PRs have to be vetted is a security measure because our CI workers don't run in completely sandboxed environments. This is a technical limitation and not something I would call an "artificial limitation".

Also, you seem to think that the CI status actually has an influence on how fast or even if something gets reviewed. I can tell you with relative certainty that this is not the case. Whether we chose to somehow implement what you propose or completely get rid of CI, people are not going to go home from work or cancel their holidays just to review stuff because a button becomes green. I needn't remind you that up to a few years ago, SFML development took place without CI at all. Laurent would manually review all pull requests that were made and chose to merge them or not at his discretion. The situation has improved now since we have multiple people who can review and vet changes, and we have CI to provide information about buildability. Nothing changed the fact that a human has to find time to review, with or without CI support.

Please tell me there are GOOD reasons to this wasted time.

Like I said, if you consider this "wasted time" then there is probably little I can do to convince you otherwise. If you don't want to acknowledge the advantages that such a cautious workflow brings along, I also can't help it.

1. We have significantly fewer regressions per commit than other comparable libraries.
2. Our master branch is stable enough that many people don't feel uncomfortable with using it in production environments.
3. Because master is so stable, we aren't under constant pressure to officially release new versions of the library that often.
4. The preconception that a new feature must be buggy when it is still fresh doesn't apply here as much as it does in other projects that opt for a less cautious workflow. It might seem like luck at first glance, but new features are used by people right from the get-go without any noteworthy limitations.
5. Being a C++ library, SFML has always valued code that not only functions correctly but is also more or less aesthetically appealing to the human eye. Only a human can judge the appeal of how code is written, and reading through comfortable code is significantly less stressful than reading through code for which "reading pleasure" was not highly valued. This also contributes to the fact that bugs can be narrowed down fairly quickly once somebody comes around to investing their time.

If you want to throw these and other advantages out the window just so that you don't have to wait forever for your PR to get merged, then that is your opinion. In order for you not to have to wait, we would have to do without formal code review by a human and merely hope that everything goes good after merging a PR, based among other things solely on CI feedback. I don't know about you, but I do actually value the code itself as much as what the code does at the end of the day. As such, I have an interest in keeping the codebase as clean as possible, and to that end formal code review is indispensable.

If you really can't stand having to wait for your PR to get merged here are a few tips:

1. Fairly obvious: Don't work on something nobody asked for. This is why we encourage forum discussions before investing time in something.
2. The more people actively complain about something, the more people there will also be who will divert their effort to actually testing your solution for said problem.
3. If something has been triaged on the issue tracker and has reproducible test cases available, you can assume that people will be more likely to test your PR as well because it means less effort for them.
4. Because SFML is also used in commercial projects, anything that indirectly adds value to the final product might encourage commercial developers to divert some of their precious time into getting said patch merged because they benefit from it in the end as well.

Maybe there is this misconception that the SFML Team is among other things responsible for sparking interest in PRs when applicable. We can only support the process by providing things like forums, threads, etc. At the end of the day you as the PR author carry the responsibility for getting people interested in your change. Either there was already interest in the issue before the PR was opened or you create interest after opening it. If there is no interest, then you can be fairly certain that progress will only be made at a very slow rate causing this "irritation". The same rules that apply to you apply to any member of the team as well. We don't get any sort of bonus. I have at times waited over a year for feedback on certain issues. Do I get frustrated? No. I have other things to do in life as well. If I want the throughput to stay high, I just open up multiple PRs in parallel and hope that progress can be made on one of them at any given time. This is what git and the branching model is designed for, parallel work in the same codebase. I manage to keep track of 6-8 PRs that I have open at any given time. If you can't manage to do that, then I am very sorry, but that is just life. Not every system is going to bend itself to fit the needs of everyone, especially since the majority of users are more or less satisfied with the status quo.

[mantognini]

Let's be honest and agree we are losing more time and energy with this discussion than we will possibly win in the short term. 'Agree to disagree' is the right expression, I guess. ;)