3 ms·

Every commit should build is a reasonable principle. However, it's in contradiction to TDD. If you have a bug (in master) the first thing you are supposed to d

Every commit should build is a reasonable principle. However, it's in contradiction to TDD. If you have a bug (in master) the first thing you are supposed to do is to create a test that fails because of the bug. This is a self-contained commit, but it will not build (assuming your test are run during the build as they should). The next commit is fixing the bug and now it builds again. So the commit with the new test should be marked as unsuitable for bisecting. How to do that easily in away that bisecting remains easy I have yet to work out.

simonw 5y ago
One of the many reasons I'm not a fan of the "pure" model of TDD. If you want to work like that though one way to do it is to work in a branch/PR - building out your commit that fails, then your iterative commits - and then squash the branch into a single commit onto main layer on. If anyone want to see your TDD tiny commits process later they can go and see it in the PR.
trevinhofmann 5y ago
I've been enforcing every commit to both build and pass tests, but now I might only require each commit to build and each merge request to pass tests. In that case, you can have a merge request with a commit to add a test and then another commit to fix it. As long as both commits build, bisect will still work. I agree with having CI enforce both passing tests and working builds, but why would you want the build step to require passing tests?
- usr1106 5y ago
  That's just terminology. When I said "does not build" I meant CI fails because a test fails. Of course the code would compile or what other steps you might have.
  - trevinhofmann 5y ago
    Change the CI to test that every commit builds and every merge request passes tests. If we don't enforce the tests to pass for every commit, that solves the problem. Right?
- WorldMaker 5y ago
  This is basically the approach that I take. I expect git log --first-parent (and git bisect --first-parent) on the main branch to pass full CI builds, but I'm increasingly more lax about individual commits the further you get out into the DAG. I think it way more useful to record the steps-in-progress than to toss them (especially when you are trying to debug the why or the how of a change months or years later from say a junior developer; it's a lot easier sometimes to see things like "oh, this was a test value meant to be replaced" when you see exactly which intermediate commit it was from deep in a PR). (ETA: It's easy to forget that the I in CI stands for "integration" and was always focused on merges of changes more than specific changes.)
omegalulw 5y ago
Isn't this taking TDD to literally? Verifying that a new test is indeed broken doesn't deserve a separate commit. The presence of that test in the codebase with all tests passing is enough. If your devs are writing new tests that are already passing your team needs a better lead, not more build controls.
- usr1106 5y ago
  In normal development flow sure. But if the bug made it already to master I'd prefer the separate commit so it can be easily verified that the problem is gone.
lloydatkinson 5y ago
I have never seen TDD state that a test should not be in the same commit as a feature.

simonw 5y ago

One of the many reasons I'm not a fan of the "pure" model of TDD. If you want to work like that though one way to do it is to work in a branch/PR - building out your commit that fails, then your iterative commits - and then squash the branch into a single commit onto main layer on. If anyone want to see your TDD tiny commits process later they can go and see it in the PR.

trevinhofmann 5y ago

I've been enforcing every commit to both build and pass tests, but now I might only require each commit to build and each merge request to pass tests. In that case, you can have a merge request with a commit to add a test and then another commit to fix it. As long as both commits build, bisect will still work. I agree with having CI enforce both passing tests and working builds, but why would you want the build step to require passing tests?

usr1106 5y ago
That's just terminology. When I said "does not build" I meant CI fails because a test fails. Of course the code would compile or what other steps you might have.
- trevinhofmann 5y ago
  Change the CI to test that every commit builds and every merge request passes tests. If we don't enforce the tests to pass for every commit, that solves the problem. Right?
WorldMaker 5y ago
This is basically the approach that I take. I expect git log --first-parent (and git bisect --first-parent) on the main branch to pass full CI builds, but I'm increasingly more lax about individual commits the further you get out into the DAG. I think it way more useful to record the steps-in-progress than to toss them (especially when you are trying to debug the why or the how of a change months or years later from say a junior developer; it's a lot easier sometimes to see things like "oh, this was a test value meant to be replaced" when you see exactly which intermediate commit it was from deep in a PR). (ETA: It's easy to forget that the I in CI stands for "integration" and was always focused on merges of changes more than specific changes.)

omegalulw 5y ago

Isn't this taking TDD to literally? Verifying that a new test is indeed broken doesn't deserve a separate commit. The presence of that test in the codebase with all tests passing is enough. If your devs are writing new tests that are already passing your team needs a better lead, not more build controls.

usr1106 5y ago
In normal development flow sure. But if the bug made it already to master I'd prefer the separate commit so it can be easily verified that the problem is gone.

lloydatkinson 5y ago

I have never seen TDD state that a test should not be in the same commit as a feature.