Recent QA testing has found some regression bugs in our code. My team lead blames recent refactoring efforts for the regressions.
My team lead’s stance is “refactor, but don’t break too many things”, but wouldn’t tell me how many is “too many”. My stance is it’s QA’s job to find bugs that we can’t find, and refactoring usually introduces breaking changes. So sure, I can be careful, but I don’t knowingly release code with bugs to QA. I do it because I don’t see them.
If the refactoring was necessary, how many regression bugs should be considered too many?
9
You are right that refactoring code is important. It prevents code rot and improves code. It makes for cleaner code.
But good code is not only clean code, it’s code that is correct, and thus by definition contains as few bugs as possible (ideally none). The first goal of your code is to produce its expected result. So if your refactoring is introducing bugs you might want to consider the net effect of those refactorings.
You should refactor code that is tested. If it’s not, add tests and then refactor. This way you know you haven’t broken anything. This will help mitigate the risks of a similar situation from happening in the future.
As for refactoring introducing bugs, refactoring should not alter the behavior of a program. I will quote from Wikipedia:
disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior
Sadly nowadays, refactoring has come to mean anything from this definition to total rewrites.
I would alter what your team lead said from:
refactor but don’t break too many things
To:
refactor and don’t break anything
As for finding bugs being the job of the QA, quality isn’t someone else’s problem. The goal should be that QA finds nothing. Realistically finding as little as possible.
1
My stance is it’s QA’s job to find bugs that we can’t find,
Finding bugs is a joint responsibility.
QA’s job is to find bugs that you don’t find. There should not be such a thing as bugs that you can’t find. (And if there is, then maybe you should ask for access to the tools / tests that they are using so that you can find them for yourself.
refactoring usually introduces breaking changes.
That is not true. Refactoring done properly doesn’t break things. And you could argue that fixing the things that break should be part of the refactoring.
The problems arise in reality when:
-
the refactoring triggers latent bugs in other parts of the codebase that the tests you can run don’t reveal, or
-
you can’t do the full refactor because someone else “owns” some parts of the code that is affected by the refactor.
You shouldn’t cop the blame for latent bugs (unless they were yours in the first place). But on the other hand, if your refactoring is intentionally changing APIs and or behaviours then you need to discuss this with all parties before you refactor, and have a plan for dealing with the effects. On the third hand1, if the breaking change is unexpected or accidental … then you should have thought it through in more detail.
And finally, if the regressions were your fault (or partly your fault), then “man up” and take the blame. And aim to do better next time.
If he agrees that refactoring is necessary (which he does), then why does he have a problem when stuff breaks? Especially when it hasn’t affected our schedule so far.
That is easy to answer if you put yourself into his shoes. He thinks you did the wrong thing in the way you refactored the code. This time it didn’t impact on the schedule. But next time, it could. He is trying to prevent that eventuality.
1 – A Larry Niven allusion.
8