How to structure commits when unit test requires refactoring

Question

I'm trying to get a review for my lists of pros/cons about how to structure commits that came out of a discussion at my work.

Here's the scenario:

• I have to add feature X to a legacy code base
• The current code base has something I can't mock making unit testing feature X impossible
• I can refactor to make unit testing possible, but it results in a very large code change touching many other non-test classes that have little in common with feature X

My company has the following strictly enforced rules:

• Each and every commit must be stand alone (compiles, passes test, etc.) We have automation that makes it impossible to merge until these have proven to pass.
• Only fast-forward merges are allowed (no branches, no merge commits, our origin repository only has a a single master branch and it is a perfectly straight line)

So the question is how to structure the commits for these 3 things. (refactoring, feature X, and test for feature X) My colleague referred me to this other article but it doesn't seem to tackle the refactoring part. (I agree without the refactoring source and test should be in one commit) The article talks about "breaking git bisect" and "making sure every commit compiles/passes" but our strict rules already cover that. The main other argument they give is "logically related code kept together" which seems a bit to philosophical for me.

I see 3 ways to proceed. I'm hoping that you can either a) add to it b) comment on why one of the existing pro/cons is not important and should be removed from the list.

method 1 (one commit): includes feature X, test for feature X, and refactoring

pros:

• "Logically related code kept together" (Not sure this is actually a "reason". I would probably argue all 3 methods do this, but some may argue otherwise. However, no one can argue against it here).
• If you cherry-pick / revert without merge conflict, it will probably always compile & pass tests
• There is never code not covered by test

cons:

• Harder to code review. (Why is all this refactoring is done here despite not being related to feature X?)
• You cannot cherry-pick without the refactoring. (You have to bring along the refactoring, increasing chance of merge conflict and time spent)

method 2 (two commits): one includes feature X, then two includes refactoring and test for feature X

pros:

• Easier to code review both. (Refactoring done only for the sake of testing is kept with the test it is associated with)
• You can cherry-pick just the feature. (e.g. for experiments or adding feature to old releases)
• If you decide to revert the feature, you can keep the (hopefully) better structured code that came from the refactoring (However, revert will not be "pure". See cons below)

cons:

• There will be a commit without test coverage (even though it's added immediately after, philosophically bad?)
• Having a commit without test coverage makes automated coverage enforcement hard/impossible for every commit (e.g. you need y% coverage to merge)
• If you cherry-pick only the test, it will fail.
• Adds load to people wanting to do revert. (They needed to either know to revert both commits or remove the test as part of the feature revert making the revert not "pure")

method 3 (two commits): one includes refactoring, two includes feature X and test for feature X

pros:

• Easier to code review the second commit. (Refactoring done only for the sake of testing is kept out of feature commit)
• If you cherry-pick / revert either without merge conflict, it should compile & pass tests
• There is never code not covered by test (both philosophically good and also easier for automated coverage enforcement)

cons:

• Harder to code review the first commit. (If the only value of the refactoring is for test, and the test are in a future commit, you need to go back and forth between the two to understand why it was done and if it could have been done better.)
  • Arguably the worst of the 3 for "logically related code kept together" (but probably not that important???)

So based on all this, I'm leaning towards 3. Having the automated test coverage is a big win (and it what started me down this rabbit hole in the first place). But maybe one of you has pros/cons I missed? Or maybe there's a 4th options?

Answer 1

When working on existing code, it's common that you need to refactor the code before you can implement your feature.

This is the mantra from Kent Beck: "Make the change easy (warning: this may be hard), then make the easy change"

To do so, I usually recommend to do frequent little commits. Take baby steps. Refactor progressively:

Each refactoring doesn't change the way the code works, but how it's implemented. It's not "hard to review" since both implementation are equally valid. But the new implementation will make it easier for the change to be made.

Finally, write the test and make it pass. It should be relatively short and to the point. That also makes the commit easier to read.

Therefore I'd go for the 3rd option too. Maybe I'd even have multiple refactoring commits. Or I'd squash them into one before pushing that for review, so there's only one. Or maybe I'd do a first PR that's only refactoring, then a second that's only the feature. It really depends on how much refactoring is needed (keep your PRs short) and your team conventions!

If the only value of the refactoring is for test, and the test are in a future commit, you need to go back and forth between the two to understand why it was done and if it could have been done better

To solve this problem, you need to get your team comfortable in this approach: refactor first, then implement the feature.

I'd suggest you to discuss it with your colleagues and try that out. I'd also recommend you try to practice "over-committing" to get you in the habit of doing smaller commits. It's a useful skill to have when code is tricky, so it's a great exercise to do when code is not!

In any case, I think you've healthy discussion with your colleagues. No doubt you'll find what works for your team!

Answer 2

Go with 3 - but mention the reason for the refactoring clearly in the commit message. Then noone has to guess around why you did it.

Any refactoring which touches many files will always be harder to review than one which touches only a few, that makes no difference whether you do it at the start, in the middle or at the end of some development cycle. But when you mix large refactorings and new features in one commit, that becomes really hard to review, so this rules method 1 out.

Method 2 has the drawback that you will have no chance of doing any TDD for the new feature X. And after you added feature X without any unit tests, there is a certain risk of forgetting about the tests afterwards, because the extra effort of requiring a large refactoring first before being able to add unit tests may not seem worth it (which is probably a fallacy, but you may have to explain this to your superiors).

Moreover, I would recommend

• to make sure you have enough tests in place before you start (not necessarily unit tests) which give you confidence the refactoring does not break anything. If not, take the time for adding such tests beforehand.

• to make sure after you finished feature X + unit tests, you review the code by yourself and verify if the refactoring you did beforehand reached it goals, and if the code is really in a clean state now. If not, add an additional refactoring / clean up step afterwards.

Answer 3

I vote for 3. In case of large, unmaintanable legacy code, special rules should be followed. Most "common sense" rules only apply to code that is already in well-maintainable state.

Answer 4

Refactoring before the new feature should be your choice (method number 3), although I would certainly consider refactoring multiple times before the new feature. If you can, I would recommend breaking your refactoring down into individual commits.

While large commits are sometimes unavoidable (especially with older code), the smaller the better.

Answer 5

Refactor behaviors by creating the subroutines first, a few or one at a time, then changing the callers, a few or one at a time, to stop achieving that behavior by internal code and calling the subroutine. Test suites to compare the behaviors before and after can be added if the caller name is changed so they both exist at one revision for comparison testing, and then the obsolescent code and comparison test can be removed in another version, renaming the new code to the old name so its callers need not be modified.

Good test coverage is, of course, extremely important: all the odd cases, exceptions, nulls, negatives, divide by zero, NAN, errors, edge and center cases of all sorts.

Answer 6

method 3 is the most common, since

1. you develop a feature increment, and no feature increment should be possibile without its associated tests. When i pull a branch, the first thing I do is to look at tests to find usage examples of the newly developed APIs. That commit is autoconsistent and documented through test

2. Once you are able to provide working code with a new feature to your teammaetes, you are ready to refactor the code. Refactoring should be done with very little steps, without breaking any test. Breaking changes should happen in the most safe way

That part is what i call "extend, replace, delete"

let's suppose you have

public void executeBadCode(String what , String why) { ... }

and your refactoring involves a breaking change such as introduce a parameter object.

so you ADD a method

public void excuteBadCode(BadParams b) {
   executeBadCode(b.what() , b.why());
}

along with its unit test. You will commit specifing that is a refactoring. eg. [refact] introducing parameter object overload for method executebadCode()

Now i REPLACE all the usages of the previous function. I will do it one at time, doing a single commit per substitution in a class/file, along with potential changes or additions in associated tests

Finally I remove the old method moving its body in the new one and making sure there are no reference in the code.

In this way every developer could pick a working version of your code, can review it and can compare with the previous, easly detecting the single meaningful change done