Hacker News Reader

Comment by Deradon | Hacker News Reader

> 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.

View on HN

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.

Reply on news.ycombinator.com