Hacker News Reader

Comment by wfunction | Hacker News Reader

> The thing we do differently from most is that we run the full test suite against every patch, as if it were merged to master, before committing it into the master branch, whereas most CI setups test after committing, or if they do run tests against every PR, they do so before merging, leaving open the possibility of regressions introduced during the merge.

Hm, Travis CI on GitHub runs tests on a pull request before the merge. Is what they're doing really that unusual?

View on HN

As it says in the quote you provided, the difference is that it tests what the result would be of the merge, which differs in that a merge might introduce regressions; if you've never had a bug introduced by a merge not doing what you expected, consider yourself one of the lucky ones.

I'm not sure I'm understanding things correctly. What I meant was that the test chronologically occurs before the merge takes place. But the test includes the merge itself; it's testing what the result would be after the merge. Have you used the system I'm talking about/do you know what I mean? Am I still misunderstanding something?

Here's an example scenario:

1. Your PR is tested with the merge against Master. All green!

2. Another PR is tested with the merge against Master. All green!

3. The other PR goes in first. All good!

4. Your PR goes in (merge happens, but introduces a bug). Oh no!

Thanks! Someone pointed it out in a sibling comment afterwards too. Kind of sucks, I wish they had a "do final check and merge" button that would merge PRs sequentially into master after re-testing when there's a collision like this. Any idea why they don't?

I think this is more of an issue with GitHub not firing another webhook, than Travis. Travis just does what it's told.

But that only works well if the PR is up to date with the targeted branch.

AFAIK, you have to manually merge the targeted branch into the PRs to be in an equivalent situation. Also, other commits might still be added into the targeted branch between the time the tests have been run and the PR is merged.

Oh I see, so the problem is that the merge commit is not the same as the one that the test runs for if there are any intervening commits between them. Interesting, I didn't quite realize that! (Though it seems obvious in hindsight, since a new push doesn't make all past PRs' tests re-run.) Thanks for explaining that! So it seems it's something between what they do and what they were trying to avoid. GitHub should really put a button to re-run tests on merges that are out of date...

Not unusual at all. Any organization using Gerrit with a rebase-only workflow with any popular CI systems (like Jenkins) have been doing this for ages.

Reply on news.ycombinator.com