3 ms·

Everyone talks about refactoring code. What I don't see enough discussion on is refactoring tests. It's a constant battle at work, and I tend to be firm with:

Everyone talks about refactoring code. What I don't see enough discussion on is refactoring tests. It's a constant battle at work, and I tend to be firm with: "If the test is working, no matter how horrible the (test) code, leave it be."[1] For regular code, we rely on tests to validate our refactors. With test code, we don't have that support. If you have a bunch of tests that could do with a refactor, and your motivation is that you need to write more tests, then set up whatever abstractions you need and write the new tests, but don't touch the existing tests! If you really want, you can overtest: Write new tests with your refactored code that test the same thing as the old ones. It's OK to test the same thing multiple times (when testing is cheap). About a third of the time a coworker looks at my tests and refactors them, he makes an error - the most common one being not testing what the original test meant to test. See, the reason he wanted to refactor was that the original test was hard to follow. And because of that, he failed to interpret it correctly and failed to properly reproduce the test in his refactored code. I then have to code review all his refactors. I have to spend time to figure out what my difficult-to-understand test did, and confirm he didn't introduce bugs. It's very, very tiring. And for what? And he's not a newbie. He's as good at SW as I am. Plenty of experience. If he gets it wrong, I expect most people will get it wrong. With a higher error rate. This is the one case where I say: "Feel free to overcomment." If you took a long time understanding a given test, write out what you learned so that the next time you read it, you know what it does. I'll be happy to code review that. [1] Unless you are the original author of the tests, and they are still fresh in your mind. In that case, refactor all you want.

kylereeve 2y ago
When refactoring tests, some form of mutation coverage [0] would be really nice. Verify that the tests break when the underlying code changes. [0] https://en.wikipedia.org/wiki/Mutation_testing https://en.wikipedia.org/wiki/Mutation_testing
- deleted 2y ago
  [deleted]
foo_barrio 2y ago
This is a good point! We've gotten some instances of Chesterson's Fence that some devs casually remove during a "test refactor" that later allowed a regression to make it into prod. I've caught some errors in "test refactoring" from our multiple levels of testing having large overlap with teach other. Our end-to-end tests have a lot of overlap with the integration tests which in turn have a large overlap with unit testing. The unit tests run in a matter of seconds compared to the end-to-end which can take minutes or in some cases hours for our manual testing so the levels of testing also serve as an efficiency for us.
maples37 2y ago
For tests, when in doubt, I don't touch the original tests and just add my own separately. That way I know that the old tests still validate the originally-intended behavior, and my new tests validate what I expect them to do. If the tests aren't broken, is there ever a good reason to make sweeping changes to them?
martin-t 2y ago
Sounds like the tests would benefit from comments explaining what they are testing and why. Just a few days ago there was an article from an experienced dev that after multiple decades he thinks you simply can't put too many comments in tests. I was on the fence with it but seeing your comment, I now know where it comes from. Maybe this could also be solved with better tooling - tests that allow substituting the tested system with a dummy that fails and thus verify the test is actually working.
- BeetleB 2y ago
  If the name is not descriptive enough, I put it in the test's docstring. Still, it sometimes is not enough. I like languages where the test infrastructure lets you put an arbitrary string as the test name (e.g. F#, Elisp, Racket, etc). This way, you don't spend time thinking up a weird name for the test. You simply write a string that describes what you're testing, and what is expected. It's OK if that string is a few paragraphs long.

kylereeve 2y ago

When refactoring tests, some form of mutation coverage [0] would be really nice. Verify that the tests break when the underlying code changes. [0] https://en.wikipedia.org/wiki/Mutation_testing https://en.wikipedia.org/wiki/Mutation_testing

deleted 2y ago
[deleted]

foo_barrio 2y ago

This is a good point! We've gotten some instances of Chesterson's Fence that some devs casually remove during a "test refactor" that later allowed a regression to make it into prod. I've caught some errors in "test refactoring" from our multiple levels of testing having large overlap with teach other. Our end-to-end tests have a lot of overlap with the integration tests which in turn have a large overlap with unit testing. The unit tests run in a matter of seconds compared to the end-to-end which can take minutes or in some cases hours for our manual testing so the levels of testing also serve as an efficiency for us.

maples37 2y ago

For tests, when in doubt, I don't touch the original tests and just add my own separately. That way I know that the old tests still validate the originally-intended behavior, and my new tests validate what I expect them to do. If the tests aren't broken, is there ever a good reason to make sweeping changes to them?

martin-t 2y ago

Sounds like the tests would benefit from comments explaining what they are testing and why. Just a few days ago there was an article from an experienced dev that after multiple decades he thinks you simply can't put too many comments in tests. I was on the fence with it but seeing your comment, I now know where it comes from. Maybe this could also be solved with better tooling - tests that allow substituting the tested system with a dummy that fails and thus verify the test is actually working.

BeetleB 2y ago
If the name is not descriptive enough, I put it in the test's docstring. Still, it sometimes is not enough. I like languages where the test infrastructure lets you put an arbitrary string as the test name (e.g. F#, Elisp, Racket, etc). This way, you don't spend time thinking up a weird name for the test. You simply write a string that describes what you're testing, and what is expected. It's OK if that string is a few paragraphs long.