Hacker News Reader

Comment by stickfigure | Hacker News Reader

I can perhaps understand where this one is coming from:

I refactored the code from untested and untestable, to testable with 40% test coverage. The senior architect is refusing to merge because the test coverage is to low.

You have a piece of software running in production; even if it's poorly written and bereft of tests, your customers are satisfied. Customers (unwittingly) have already vetted this version! A rewrite, even with brilliant design and fantastic test coverage, is still more of a risk than fixing what isn't broken.

This isn't to say that you shouldn't rewrite code, especially if it makes other improvements possible. But don't discount how high a bar new code has to reach to measure up with "it already works". Maybe 40% is still too risky.

View on HN

  a rewrite... is still more of a risk than fixing what isn't broken.

Defining "broken" is the problem. A long-extant program may be "broken" even while it meets customer expectations, and "fixing" it could be viewed as a regression by the customer.

I had a case early in my career where I was finishing up a rewrite of a report writer program. The old version didn't really have test data, so "testing" mainly meant comparing outputs of the legacy system vs. the rewrite. After I had sussed out all the errors, there were still some differences in which the legacy output just didn't make sense. I met with the customer liaison, and he checked with his users.

It turned out that nobody had bothered even to glance at those specific reports in as long as anyone could remember.

So... did you correct the errors and release your updated (useless) report? Or did you slog through 7 levels of management approval to get permission to get rid of it?

No, I just had the customer rep send a note appeoving the new system with unwanted reports removed. (This was a mainframe shop, where excess CPU, I/O, and printing had meaningful costs.)

Just because it works doesn’t mean it isn’t broken.

And just because a developer thinks it's broken doesn't mean it doesn't work exactly the way the customer likes it.

Software is done when nobody is willing to work on it anymore.

There are some things you do for employee retention in every industry.

Typically the bits we are talking about are the bits the customer doesn't know about. Or they're caught in a cognitive dissonance loop where they want two things like features and stability but they won't agree that the two affect each other. So they ask for one until problems arise and then they pretend like we should have been honoring the other the whole time. Even when they told us it wasn't a priority.

It's basic maintenance philosophy. It's not a priority to get your car to the shop. But it is a priority that it work reliably for your Thanksgiving trip in two weeks. Many things worth doing are inconvenient. As are many critical things.

You don't want to test code. Coverage is mostly useless.

You want to test functionalities: setup end-to-end tests. It will be hard, it will take time and it often is a solo endeavor for the first months. But when tested functionalities stop breaking for no reasons even when you refactor a lot of code, when "the project for which a dev put tests" starts being "the project we are confident to push to prod without breaking things" you should get some traction.

And once you think all your functionalities are tested you can measure code coverage: what is not covered should be dead code you can remove.

Pointing people only at end-to-end testing seems poor advice. There is value in a mix of all sorts of testing. I agree that testing functionality blows counting coverage by the line out of the water. But since a lot of code that implements algorithms, string manipulation, etc. can be tested just fine with unit tests, and since end-to-end tests are expensive, easily more brittle, require a lot more dependencies, it would be better to have a healthy mix of different types of tests.

The comments was specifically about the use case of getting a large refactor merged.

If you are simply adding code or making simple changes, then unit tests can have advantages. It is when you are making a large refactor that you need end-to-end testing to verify that nothing was broken.

Coverage is very useful in determining whether your tests actually do what you think they're doing. It's not very useful as a metric to be optimized. Writing good end-to-end tests that really cover the functionality as specified is very difficult, doing so blindly without tool support is doomed to fail.

An even better tool than measuring coverage is setting up a mutation testing framework and adding tests until all mutations are caught.

I agree, and I think there is an arrogance by those with the view that 'coverage testing is useless' which blinds them to the real purpose of code coverage testing: to tell you what you haven't tested yet.

Coverage isn't supposed to tell you anything about the quality of the tests beyond what code simply has never been tested yet.

And yet, people seem to think "code coverage testing" means something else entirely. The only purpose for paying attention to coverage metrics, as a manager, is to understand how much more work there is to be done on the tests - or, to determine code paths which never get executed and are therefore dead weight on the project.

> the real purpose of code coverage testing: to tell you what you haven't tested yet.

I can get 100% code coverage with tests passing on:

    // Returns the result of A*B
    int mult(int A, int B) {
      return 4;
    }

I won't know the functionality is not tested. Code coverage will just be an illusion of security.

Code coverage is not all you should be doing.

Tests need to cover small units one at a time. I've been bitten too many times by tests that were too complex and attempted to test too many things at once. They invariably ended up doing nothing of value. Test setup becomes overly complex and prone to oversights. Writing another test for a variation of a problem that takes a slightly different code path becomes a chore. It also gets hard to spot that odd untested corner case hidden deep inside some complex logic.

The proper way to think about this is to have unit tests that cover the nooks and crannies of the business logic and integration tests that make sure that interfaces are correct.

I can't get behind this.

End to end tests are indeed important.

But let's say you have an end to end test that covers A, B, C, D, E, and F.

One day this test stops working. Well, what's the culprit? A? B? C? D? E? F? If you had more granular tests in addition to the end-to-end tests, you'd know.

Now, I agree there's a possible downside here. Maybe there comes a time when C and D are no longer being used. It might not be obvious and C and D could linger in the code base, with corresponding dead weight in the test suite. This is not ideal. However, I would rather have this than a tough-to-debug test failure.

I see this over and over also in organizations I work in and even have to fight it in my own behavior..

If you run an end to end test automation and it fails and then you are afraid or otherwise hesitant of doing the process manually you are out of touch with the system and probably missing the forest for the trees when you work on subsystems.

While I would like to have better end-to-end tests, I know that I will have to fight more problems of getting too far away from how the system works.

> Well, what's the culprit? A? B? C? D? E? F?

I honestly don't care. I know this merge is not touching production. I could have all of A, B, C, D, E and F test suites working individually if they break when put together they are all bad.

The developer have tests so they know exactly how to reproduce the bug. Time for them to earn their salary.

> Well, what's the culprit? A? B? C? D? E? F?

Probably, the one you just changed. How often is it really going to be that hard to debug?

I only changed L, which is nominally unrelated to all of those. You thought this was a clean, well-modularized codebase?

    You thought this was a clean, well-modularized codebase?

Well, even if it's nice and clean A, B, C, D, E are each going to have their own dependencies!

If you change B and the A/B/C/D/E integration test breaks.... duh, it's obvious. Yes, that's easy.

But A, B, C, D, E are each going to have their own dependencies.

You could change X, Y, and Z and now the A/B/C/D/E integration test breaks. Which one was it?

Or maybe you're super cautious and you only change Z. And now the A/B/C/D/E integration test breaks. Obviously it's Z. But how? Why? It's a four-developer team and you're only really familiar with R, S, T, U, V, W, X, Y, and Z.

There's also the fact that unit tests serve as living, de facto documentation on how to actually use the classes. You can both use and iterate upon Z a lot faster and more confidently if it has its own tests.

The overuse of overly granular services can make this pretty awful unless you've put event tracing in place.

I mean, the obvious answer is don't do that, but this is the world people are embracing.

My thoughts exactly. I'm not merging a bunch of untested code that replaces a bunch of working code, no matter how bad the old code was!

That seems like a recipe for a production nightmare.

The fact that this situation even arose in the first place would be bad news.

1. Did the other dev just refactor the code on a Red Bull bender one night on his own time? Hopefully this refactor work is something we would have discussed ahead of time. Hopefully I wouldn't have to even tell them that the expected coverage for a big refactor like this would be darn close to 100%.

2. I'd phrase this rejection in a positive way. I'd be excited about the refactor and testability and I'd let the other dev know that I appreciated the work and was looking forward to getting it to 100%, testing and merging it

(I realize the example in the story was purely hypothetical and this didn't actually happen. :P)

It can be defended when the system is in a rather passive form of maintenance. E.g., every month a small bugfix. If it is more than that it becomes way to easy to fix one small thing while breaking three. Especially if the functionality is a little or a lot complex.

What it does sound like, though, is that everything was refactored/covered in tests in one fell swoop. I find that questionable. I think it is much better to start out small in an untested code base. Just cover the small piece you are working on now in tests and gradually extend.

Some code is so mangled together that trying to start small is more of an investment than overhauling the whole thing altogether.

It's always a lie, about running in production. I've personally seen untested code allegedly running but it isn't actually working. Defect numbers through the roof and test harness literally not being merged exactly because of this issue.

You will get visibility, either by customer complaints, slowing down development speed and ultimately projects being shut down. It's not sabotage and it's not exactly drowning. It's willful attempt to ignore reality and cheat on metrics, prolonging the inevitable and defrauding stakeholders.

It's always a lie? You're basically arguing that code with bad test coverage never works.

Production code is copping constant manual end-to-end tests. Those tests are more valuable than most unit testing, and some automated black box testing. I'll take stable production system over the unit tested new code any day of the week.

Here's the an important piece of real-world statistics for you, which I've read about in research papers and personally verified in large-scale systems:

For every 1 complaint or help desk call you receive, there are between 100 and 1,000 users that experience the same issue but didn't bother to complain or call.

Production testing is literally 0.1% to 1% as effective as actual testing.

Depending on your user numbers, this works out pretty well. If you've got 100k users, you're likely to get reports on anything that hits at least 1% of them.

In my experience, you can write much simpler code that only needs to work in production than code that also needs to be unit testable. Adding hooks and separating bits and pieces so they can be tested can often make the code much less concise and harder to follow.

I would rather spend engineering time making sure problem reports from users get read and escalated and addressed than on testing things that are better left untested. Unit tests, and even integration tests are unlikely to let you know when the feature works as designed, but not as users expect.

My favorite example of "things that are better left untested" so far is a method that returned a hard-coded array and a test which checked whether the method really did return that hard-coded array. So, if you wanted to add something to that array, you now had to add it in two places. Sure, that's an easy way to increase test coverage, but maybe you should focus on areas where unit testing makes more sense first?

I prefer to fix bugs before they hit production. It is almost impossible to do that without tests. Also testable code is usually much more readable than untestable god objects, tightly coupled modules and spagetti code.

without tests doesn't mean bad architecture.

I can go the other way and say that code that ends up being 'modified' to be testable, with 43 mocks to just be able to setup a class is as much crap as code without tests.

I would say I never achieved more than 20% or so of test coverage in my products and production code. It doesn't mean it is a mess of tightly coupled modules in any way.

But TDD and any other dogmas in development are like this. Solution A prevents (maybe) a problem, thus if you aren't using it all other code has that problem and defendants will just keep trying to make it so.

If you need 43 mocks to unit test a class, your TDD has already found a bad smell. Why are there so many classes tied to a single component?

Video games frequently succeed despite colossal bugginess. Pokémon Go’s launch is a great example. Engineers really undermine their authority when they talk about testing and quality in abstract and isolation to the user’s experience.

> Unit tests, and even integration tests are unlikely to let you know when the feature works as designed, but not as users expect.

This is really missing the point of automated tests. Unit tests are for developers and ensure that low-level functionality doesn't break when changes happen. Integration tests verify that components fit together and that it stays that way. End-to-end-tests ensure that user functionality doesn't break.

Together, the above turn refactoring and rewriting of components from a business risk into an activity that can happen simultaneously when adding new features. This can ensure that there would never be such a thing as legacy code.

Legacy code is just code that has proven itself to be useful to the business. If you're telling me spending all day fiddling with testing is going to mean I have nothing to show for it, I guess I agree.

I think there is room for testing, but you need to really restrict the scope to those parts of the code where the requirements don't change often, and where the natural interface is amiable to testing, and often, you need someone whose job is testing and who didn't write the code to actually do it. Incidentally, those parts of the code where the requirements don't change often get to a point where the code doesn't change either, which should mean the tests won't fail, because the inputs stayed the same.

  between 100 and 1,000 users that experience the same issue but didn't bother to complain or call

Which is often because users have no (established, straightforward) process for reporting, or recognizing, a defect.

I found an ugly 2FA bug in a major ride-sharing app's onboarding and was shocked to find that they have no informal mechanism for reporting any defect. (I thought it important enough that I went through the hassle of reporting it through HackerOne, not seeking compensation but simply attention to the problem. It was closed within minutes without a glance or meaningful comment.[0]

I'm not shocked anymore, as this seems to be increasingly common.

Another growing trend is meaningless and even juvenile error messages. "Oops!" (with no context that could aid diagnosis). "Something went wrong." (ditto, and I see this out of the leading ticket site 300 times a year). Etc.

[0] it's possible that it was closed because that particular onboarding was later obsoleted, but still, that's just a bad look.

> It's always a lie? You're basically arguing that code with bad test coverage never works.

It may work, or it may not work. Without the tests you won’t ever actually know.

Maybe your customers are bouncing on bugs, and while you are earning enough, you could have been earning 3 times as much without those bounces.

That's the kind of thinking that leads to situations like this:

https://www.digitaltrends.com/mobile/samsung-galaxy-s10-ultr...

A lot of the time it's just as important to make sure certain things can't work as making sure that certain other things will.

And then you have tested code with useless unit tests that cheat on metrics and defects and customer complaints very high too, because nobody is listening to customers either way. I regularly run untested code (when appropriate, it’s a trade off for simpler design with less abstractions sometimes) with happy customers.

We may have diluted the meaning of refactor if we are arguing about pull requests.

Refactoring is an activity of small steps of a particular kind that are generally sound transformations. The biggest risk is transcription errors.

Of course one of the downsides of PRs is that the they muddle a sequence of individual commits and only show the cumulative result.

When dealing with individuals with these sorts of fears, you have to be patient and strategic. Pick the most powerful refactor out of a set of many useful ones and do them one at a time until you achieve the result. Essentially you have to boil the frog.

>Refactoring is an activity of small steps of a particular kind that are generally sound transformations. The biggest risk is transcription errors.

I think that is far from an established sole meaning of the term.

And then I will repeat

> We may have diluted the meaning of refactor

And I will quote the summary for Refactoring by Martin Fowler:

> Refactoring is a controlled technique for improving the design of an existing code base. Its essence is applying a series of small behavior-preserving transformations, each of which "too small to be worth doing"...

If words don't mean anything it's really hard to have conversations and it's insanity to try to create strategy around them.

40% code coverage is meaningless

It could be 40% weak unit tests that just go through the code but doesn't test much

I would be happier if it were 40% coverage running integration tests with actual data and use cases

That security hole that is leaking customer data hasn't been noticed for the 9 months since it was implemented. Except by that one guy who has captured it all. Customers have only vetted what they see.

Reply on news.ycombinator.com