Hacker News Reader

Comment by IneffablePigeon | Hacker News Reader

The majority of the time, no.

There are a couple of circumstances I often do, though.

The first is when fixing a bug - writing the (red) regression test first forces me to pin down the exact issue and adds confidence that my test works. Committing the red test and the fix to the test in two commits makes the bug and its fix easy to review.

The second is when I'm writing something high risk (particularly from a security standpoint). In this case I want to have a good idea of what I'm building before I start to make sure I've thought through all the cases, so there's less risk of rewriting all the tests later. There's also more benefit to having a thorough test suite, and I find doing that up front forces me to pin down all of the edge cases and think through all the implications before I get bogged down in the "how" too much.

View on HN

> Committing the red test and the fix to the test in two commits makes the bug and its fix easy to review.

I've done this in the past. Then I started to use `git bisect` and having a red test somewhere in your commit-history is a killer for bisect. So now I tend to include both, the test and the bug-fix, within one commit.

A tip I learned is to commit the failing test but mark it as an expected failure, if your test framework supports that.

That way you can commit the test, bisect works, and the test begins "failing" when the bug is really fixed, and you can commit the fix as well as a one-line change to amend the test from being failure-expected to just a normal test.

this is why i love HN. that is such an outstanding idea.

I see a test as a declaration of intended outcome. By writing a test to expect an intentional failure (say you have a bug in a divide: “int -> int -> Maybe int” function that causes it to return 0 when you divide by 0 instead of “None”) you are declaring that is actually intentional behavior. So I would never write a test like this - I think I would prefer committing the fix and the new test at once. I don’t see the value in reviewing them separately, because they are related and dependent changes.

Obviously if you view tests differently (eg. as a declaration of current behavior rather than intended behavior) then my argument dies.

Keep in mind the test is written with the correct behaviour and annotated to be failing — in a hypothetical language and framework your test would be

@failing testDivZero() { assertEquals(None, div(1, 0)) }

This expresses both the intent and the reality

I get around this by squashing the PR commits. Reviewers see the individual commits, and good CI means they can see the test-commit failed and the fix-commit passed, but post-merge it's a single passing commit.

> Committing the red test and the fix to the test in two commits makes the bug and its fix easy to review.

I don't think this is compelling argument. Normally the test and the fix are looked at together and are already sufficiently separated in the code.

It makes more sense to me to use a single commit so the fix can be described in a single commit message, keeping the history clean.

Keeping it separate allows ensuring the test works easily: just revert the fix keeping only the patch for the test and make sure the test fails.

My workflow tend to be to keep them separate, and write a big description in the merge commit rather than the individual commits (those have a small description of the local change).

There's tradeoffs to both approach though, and as you mention, it's easier to keep track of the provenance of a fix in the git blame when it's unified in a single commit message.

I totally understand why you would do that and it makes sense, but I personally like to keep them in one commit so that I won't get any problems with an automated git bisect later.

Reply on news.ycombinator.com