Hacker News Reader

HNHacker News

Top New Best Ask Show Jobs

Comment by dan-robertson | Hacker News Reader

Parent Full thread

dan-robertson·

One advantage of ‘stacking’ is breaking up review into more logical units, eg

1. Introduce new test exhibiting bug

2. Introduce bug fix and update the test

If 1 and 2 are reviewed together you have less evidence that the test actually shows the bug being fixed.

View on HN

You can still have them in 2 commits, and configure your CI to build both of them, and the first 1 should fail.

We actually have a rule that there must be 1 commit just introducing a test that fails on CI for bugfix PRs.

That's interesting, explicitly to require that bugs be reproduced in CI. It makes sense in theory, but in praxis (IME) CI systems tend to be overtaxed / underprovisioned - meaning this extra burden might be questionable. /$.02

The same burden is present when splitting the two commits into two PRs.

Actually no, the burden I referred to was -- unrelated to stacked vs serial PRs -- specifically the GP's practice of requiring that tests fail in CI.

"rule that there must be 1 commit just introducing a test that fails on CI for bugfix PRs"

Given the cost a typical CI system vs the salary cost of a typical dev, the business case should be fairly easy that devs always should have CI capacity available.

Ideally your CI workflow prevents merging something that breaks the tests. Also now if the first review merges but the other doesn't for some reason you've broken everyone else which is bad.

EDIT: at least in rebase workflows which is what I'm used to. I guess in merge workflows this works.

But once you merge 1, your test suite is broken, and many/most companies wouldn't allow that.

Yeah there’s two solutions:

1. Merge #2 into #1 then merge the result, but this can obscure your history

2. Don’t make the test fail. Eg expect the incorrect behaviour and leave a comment explaining why it is incorrect and what the correct thing should be.

Reply on news.ycombinator.com