Hacker News Reader

Comment by ricardobeat | Hacker News Reader

This is a shortcut to achieve better coverage, brittle, possibly wrong, and not maintainable; and it is not really giving you any additional confidence on the code. If you're pressed for time, why waste it on something with close to zero value? Coverage by itself is an empty goal, you're better off with no tests than this.

View on HN

Suppose you're the first person to touch this code in a while. You don't really know how solid it is, but given there's no tests you would be justified in being a bit nervous.

Now you need to change it.

Any future bug discovered is probably going to come back to you, given you are now the last person to touch it and the perceived expert on this legacy monstrosity. Having proof of how the code worked before you touched it not only prevents regressions while you're working on it, but also prevents having to do this sleuthing on the fly with someone wagging a finger in your face. Not only that, but depending on dependency drift you not be able to easily go back and prove the old functionality after the fact, so having this in a CI log will be valuable for your sanity and reputation.

I don't think that scenario is realistic. If you're making a change that doesn't alter a function's output, why is the code for that function being changed at all? That would only be possible if the snapshot didn't capture all the relevant outputs to being with.

Most likely the output will change and you'll be left with a random blob of data to figure out what went wrong instead of clear specifications.

If your goal is "I have a blackbox of code, I must make a change, and I will assume existing behaviors are correct", this approach should be reasonable to ensure that the code doesn't break after your change.

You don't have an external spec to test against in this case, the existing implementation details are the spec, and so the goal is to preserve them.

I'd say going from: * Find one input * Get coverage * Mutation test to ensure you have assertion density

is probably a fine way to accomplish the goal.

Eventually you'd want to define an external specification for the code that isn't just "the service works, therefor ensure the service does what it's been doing", at which point some of these tests may be removed, or at least enhanced.

> assume existing behaviors are correct

That's part of the problem. The original test was created based on implemented behaviour and not test/functional requirements, so you don't have any confidence that what you're preserving is the right behaviour. It tells you you haven't changed the output, but once you've covered your codebase with these, and make a commit that triggers 46 snapshot failures, you have zero knowledge to decide whether that change is desirable or not.

In the absence of a specification, documentation, or access to the people who originally implemented the program, the program itself is the spec. With no way to decide what the program "should" be doing, whatever it's doing right now is the right behavior.

The point is that the existing behavior is de-facto correct by virtue of being the existing behavior.

The goal here is not "Prove this software meets a spec" it is "This software is the spec, and we must not regress". There are many more systems that act as a defacto spec than that are built to an external one.

If you choose to then move to a world where you want an external spec, as I said, at that point some of these tests will be thrown away or refactored. Most systems never make it to that stage.

This is especially common when you have legacy software where the person or team that wrote the code is no longer there. It takes a monstrous amount of effort to derive a meaningful spec, and step 0 is always preserving existing behavior.

Often I see tests that spend a lot of time mocking internal components of the application. Such tests are verifying the implementation details instead of the functionality. They often need to be thrown out if you do some refactoring, even if that refactoring didn’t change the behavior of the program at all. The kind of text described in this article should work just fine in that case, achieving the desired goal of showing whether your refactor caused any behavior changes.

In my experience, tests like this actually aren’t hard to maintain. The test can print out a diff between the expected and actual result when there is a failure, so you can check if the new behavior is expected and easily copy-paste to update the test if desired.

I like mocking internals because I can control the flow of the logic.

I will write a mocking test to ensure a business flow is followed at the test level just because I have fallen too many times for a developer updating snapshots because they made a change... but they update all the snapshots even the ones that show they introduced a new bug.

It's not meant to give you confidence in the code. It's meant to give you a quick test to make sure you don't break something while you change the code, if you have to make changes to code that lacks any tests at all.

As the author said:

> It’s fine to delete useless tests afterwards. They were useful when you had to change the code. They’re not here to replace proper tests in your codebase!

> It's not meant to give you confidence

> It's meant to [...] make sure you don't break something

It helps you see if you didn't break 'something', but 1) you have no clue what that something is by reading the test 2) you have no information to fix it. Plus the snapshot itself might have already captured the wrong implementation. It's a false sense of security.

Snapshots are perfectly acceptable for testing Legacy Apps.

If it truly is a legacy app, snapshots should rarely be updated (because you're not updating the code) and any introduced bugs will trip a snapshot. The issue is always about people blindly updating snapshots because 'I made a change and the snapshots are wrong'.

If the failed tests don't line up with what you believe you changed, you are the reason it's Legacy Code.

It adds regression protection which is highly valuable.

It allows you to refactor your code and tell whether or not you have changed behaviour. This is practically what unit tests are for full stop. It also allows you to modify behaviour and show that the behaviour has been modified as you expected without writing any additional tests. If you are finding them too brittle, then you can remove the ones that are bothering your and replace them with better tests.

It's a very good technique that I've used many times in the past.

So code that is in production should have no protection from inadvertent changes because that is worse than no tests at all? Wow. Do you code for a living?

This is not zero value. If nothing else, this test will later on, tell you if your behavior changed.

I disagree that tests of this form are worse than no tests at all. It depends on the purpose of the test suite. The article itself mentions most of these disadvantages to this approach at the end. But recall, the stated purpose of the test suite in the article is "regression", in the context of a legacy code situation. This isn't a situation we'd aspire to be in, but it is a useful tool to help locally improve a bad situation where you are required to change legacy code.

Ideally, in a healthy software project, you want to define automated tests where each test checks that an invariant is satisfied by the implementation, and links that invariant to requirements that the software is meant to implement. Then, if a test fails, you know that one of your functional requirements has been broken, in at least some scenario. If the tests and test harness are well structured, it should be very quick and easy for you to learn which requirement has been broken, in exactly what scenario. Fantastic. You can take that information and iterate, and try again.

Unfortunately, in a legacy code situation, we commonly would have no idea what the functional requirements of the software were, or what the relationship is between the functional requirements and the code. There is often no existing automated test suite. If we are under pressure to make a change to the codebase, having a dumb automated regression test suite that can check if _the behaviour of the code is the same as it was before_ is very valuable. Note that the purpose of this regression test suite is very different to the purpose of the earlier test suite that we would build in a healthy software project.

You can think about the feedback from an automated test suite as giving some information as you modify the code and move about the space of possible programs. A very high quality test suite will give you feedback that helps you steer in the right direction: towards modifications that produce possible programs that do not break any existing requirements. However, in a legacy code situation, we often don't have that luxury of knowing any requirements, or knowing internal invariants that the existing code is meant to maintain. So in this case, the feedback from our automated regression test suite will not be very informative: it does not give us a signal that we are moving in a direction that breaks requirements or not. It does however let us know if our change (from our tiny bug fix / high priority feature we're trying to jam in) causes the behaviour of the existing system to change that we did not otherwise expect. I.e. we can use it to detect if we are "moving" in the space of behaviours of the program. If a regression test breaks, this alerts us to an unplanned consequence of our change, so we can abort rolling the change out and go back the drawing board with more analysis and debugging and to try to understand what the system is doing, and if we actually want the behaviour to change or not.

Reply on news.ycombinator.com