8 ms·

IME, the only "test coverage % rule" that I've ever seen work was "must not decrease the overall percentage of tested code". Once you get to 100%, that becomes

IME, the only "test coverage % rule" that I've ever seen work was "must not decrease the overall percentage of tested code". Once you get to 100%, that becomes "All code must have a test". Various people objected to this, pointing out that 100% test coverage tells you nothing about whether the tests are any good. Our lead (wisely, IMO) responded that they were correct - 100% tells you nothing - but that _any other percentage_ does tell you something.

Ygg2 3y ago
>Once you get to 100% Once you get there you already fucked up. In Java covering 100% lines means in average case testing Lombok and every equals hash. If you're doing that you fucked up.
- groestl 3y ago
  I've seen broken hash implementations many times, so I'm not sure it would be a bad idea to require tests for them ;) My other favourite of trivial code that's broken: returning the same Iterator instance in Iterable.iterator()
  - Ygg2 3y ago
    Broken or just working with Hibernate? I remember hibernate recommending using static hash. In order to prevent saving entities changing hash values.
vasco 3y ago
What tells you something is how many releases get rolled back as a % and how much rework you have. If your developers need 3 attempts to put something working in production that's all you need to know about the quality of the reviews and tests. High or low coverage, what you need to look at is actual issues. All you need is a bit hyperbolic, because you also need to quarantine flaky tests and other things, but coverage as a whole I think is useful only if you have an engineering organization that doesn't see the point in tests - which is going to be its own uphill battle.
- polynomial 3y ago
  Good heavens, a team needing 3 tries to get something into production sounds like they need a QA refresher.
  - withinboredom 3y ago
    Most of the time, I see teams mixing refactoring, bug fixing, and new features into a single PR which causes this. Keeping PRs focused makes it easier to review and find issues before shipping to prod. In other words, first bug-fix PR should be hacky af to fix the bug. No refactoring, nothing controversial (other than the hacky af fix). After you verify the fix in production, then, and only then, do you open a PR to refactor the code. Finally, after that is verified in production, close the bug ticket.
    - sdiupIGPWEfh 3y ago
      I loved having small, focused PRs. So much easier to review without interrupting my day, too. Then we got handed a policy of one PR per ticket to simplify our QA process. Oh well.
- nimblegorilla 3y ago
  > Coverage as a whole I think is useful only if you have an engineering organization that doesn't see the point in tests - which is going to be its own uphill battle. I think coverage stats are always useful as they help find the edge cases that people forgot to test. A common culprit I've seen is error handling code where a bunch of tests target the happy path, but nothing tests the error logging when something breaks.
ponector 3y ago
I had a colleague who wrote unit tests without any assertions. Perfect idea! 100% coverage and always green. Another comment here mentioned mutation tests which could be a solution to increase quality of unit testing, but I've never seen anyone to actually use it in enterprise development. Same story with test driven development concept.
- srvaroa 3y ago
  I hear this point as a counter argument to “never decrease coverage helps maintain quality”. It is technically correct. But, it is only meaningful if you assume a bad actor in the team who knowingly games the system, and a team who tolerates it. At that point, your problem has nothing to to with code quality, nor is coverage meant to be a solution for it.
  - zarathustreal 3y ago
    You're missing the point entirely. This point is a counter example to the implied assertion that some percentage of code coverage indicates correctness. That alone is enough to prove that the implication is false. That being said, you don't need to assume a bad actor in the team to encounter the situation of having 100% code coverage with meaningless tests. Even the most well-meaning engineer can accidentally write tests that boil down to asserting "true == true" without proving anything about the code paths it touches. This isn't necessarily a cultural issue. I'd even assert that, due to the languages in common use, it's very common to see these sort of "touch all the code paths but assert nothing about the correctness of the code" tests. Standard OOP languages like TypeScript or Java for example have relatively limited type systems and allow mutable variables, so you end up with implementations of algorithms and data structures that don't lend themselves to property-based testing. This leads to tests which basically just duplicate the implementation and assert "I wrote what I wrote" or in other words "true == true".
    - srvaroa 3y ago
      > You don't need to assume a bad actor in the team to encounter the situation of having 100% code coverage with meaningless tests. Indeed. The thing is, I was replying to a comment that referred _explicitly_ to a bad actor. The situation @ponector describes (team's coverage indicator is rendered useless because have a bad actor games it), THEN the cause is not coverage. It says nothing about coverage beyond "a tool only works in certain conditions". You bring 2 more cases that break the indicator, and I agree with both (there are more!). We have (1): We all make mistakes and write dumb tests. (2): Coverage is not useful for _some_ practises / scopes of testing. I agree on both. But we're back on the same place. I'm not saying those problems don't exist. I'm saying that those are not problems coverage ever claimed to solve. Making a sweeping dismissal of a tool because it doesn't solve problems it never claimed to solve is throwing away the baby with the dirty water. * Coverage does not claim to be a tool to fix bad actor, (1) or (2)! There are other tools to cover those risks (e.g. managers, code reviews, pair programming, etc.). * Discussions about those tools (code reviews, etc.) tend to make the same mistake. Find problems the tool doesn't claim to solve to dismiss the tool. * This all happens because people pretend to treat tools like coverage, tests, DORA metrics, as silver bullets. They are not. They are all meant to be a toolbox that engineers evaluate and use where they yield value. And this is why yes, a lot of the "$tool is useless because $situation_where_it_doesnt_work" conversations are fundamentally about cultural issues. If your team uses a tool without knowing what problem is trying to solve, you have a cultural issue. If your tool has an actual purpose, and yet engineers are intentionally working around it, you have a cultural issue. Etc.
strken 3y ago
This works better as "All tests decreasing the overall percentage of tested code must have a good explanation signed off on by the reviewer". I've occasionally deleted dead code paths, had an almost entirely red diff, had close to 100% test coverage over the functions I touched, and yet decreased the overall percentage of tested code because the dead code path was more heavily tested than the rest of the code base.
- stavros 3y ago
  I kind of disagree, in that any rule should have an implied "you can ignore this with good justification". Otherwise you turn people into robots, which renders moot all the reasons why you have people, and not robots.
  - strken 3y ago
    I agree with this - in practice any rule can be broken - but how easily it's broken and whether developers are explicitly told to break it is a process decision. How big a roadblock is your CI putting in the path of developers? If you require e.g. sign-off from a team lead or manager or whoever has "merge anyway" permissions in GitHub, that's more difficult than sign-off from the same reviewer who's reading your code anyway. You can make it easier or harder to break rules and in this case I think it should be so easy to break that there's a codified procedure to do so.
- danielheath 3y ago
  I haven’t tried that - in that situation we did “find something that needs a test”
berkes 3y ago
I like that. What I also do, is ensure test coverage is over 100%¹ for important parts. Important is designated through churn (if a file or class os changed on every second commit, it must be important) and through domain knowledge (building a recipe app, then likely the Recipe is important). ¹ covered by unit tests, AND (partially) covered by integration AND by E2E tests.
nonethewiser 3y ago
> IME, the only "test coverage % rule" that I've ever seen work was "must not decrease the overall percentage of tested code". That’s a stupid rule and not measuring what you think it is. It fails if you just delete tested code. It fails if you remove some code thats tested and add some tested code, but not enough. These are all extremely common for any refactor. Here is a simplistic example but imagine the same principles applied to a large set of changes. Dozens of files, thousands of lines. You cant just manually account for that. 50%: 100 lines of code. 50 covered, 50 uncovered. remove some function and its 44% from 40 covered, 50 uncovered. Failed. Or remove some function and replace it with something better thats not as long. 44 covered, 50 uncovered. 47% coverage. Failed. A stack overflow post about this. https://softwareengineering.stackexchange.com/questions/400745/unexpected-code-coverage-reduction https://softwareengineering.stackexchange.com/questions/4007... This inevitably leads to worthless tests to increase coverage and avoiding optionL refactors.
angra_mainyu 3y ago
It sounds great but I beg to differ. If I had to pick one rule that is great to always follow, is when you get hit with a bug, write tests that catch it and then write code to fix the test. Aside from preventing said bugs from resurfacing, it forces coverage of code that is likely complex enough to be buggy.
Fargren 3y ago
> "must not decrease the overall percentage of tested code" This rule has been a problem for me when deleting code. On my precious check we had an automated checker that wouldn't let us merge (easily) if we broke this rule. We had mediocre coverage. Some parts of the code had lots of tests, some parts were totally uncovered. Many times I had to delete code in the tested parts, and the checker would complain that total coverage went down because I deleted a tested line without adding more tested lines. But I agree it's a good rule in general, which is why we kept it despite the occasional hiccup.
pfdietz 3y ago
I'd worry about test coverage as soon as the team commits to actually fixing every bug found.

Ygg2 3y ago

>Once you get to 100% Once you get there you already fucked up. In Java covering 100% lines means in average case testing Lombok and every equals hash. If you're doing that you fucked up.

groestl 3y ago
I've seen broken hash implementations many times, so I'm not sure it would be a bad idea to require tests for them ;) My other favourite of trivial code that's broken: returning the same Iterator instance in Iterable.iterator()
- Ygg2 3y ago
  Broken or just working with Hibernate? I remember hibernate recommending using static hash. In order to prevent saving entities changing hash values.

vasco 3y ago

What tells you something is how many releases get rolled back as a % and how much rework you have. If your developers need 3 attempts to put something working in production that's all you need to know about the quality of the reviews and tests. High or low coverage, what you need to look at is actual issues. All you need is a bit hyperbolic, because you also need to quarantine flaky tests and other things, but coverage as a whole I think is useful only if you have an engineering organization that doesn't see the point in tests - which is going to be its own uphill battle.

polynomial 3y ago
Good heavens, a team needing 3 tries to get something into production sounds like they need a QA refresher.
- withinboredom 3y ago
  Most of the time, I see teams mixing refactoring, bug fixing, and new features into a single PR which causes this. Keeping PRs focused makes it easier to review and find issues before shipping to prod. In other words, first bug-fix PR should be hacky af to fix the bug. No refactoring, nothing controversial (other than the hacky af fix). After you verify the fix in production, then, and only then, do you open a PR to refactor the code. Finally, after that is verified in production, close the bug ticket.
  - sdiupIGPWEfh 3y ago
    I loved having small, focused PRs. So much easier to review without interrupting my day, too. Then we got handed a policy of one PR per ticket to simplify our QA process. Oh well.
nimblegorilla 3y ago
> Coverage as a whole I think is useful only if you have an engineering organization that doesn't see the point in tests - which is going to be its own uphill battle. I think coverage stats are always useful as they help find the edge cases that people forgot to test. A common culprit I've seen is error handling code where a bunch of tests target the happy path, but nothing tests the error logging when something breaks.

ponector 3y ago

I had a colleague who wrote unit tests without any assertions. Perfect idea! 100% coverage and always green. Another comment here mentioned mutation tests which could be a solution to increase quality of unit testing, but I've never seen anyone to actually use it in enterprise development. Same story with test driven development concept.

srvaroa 3y ago
I hear this point as a counter argument to “never decrease coverage helps maintain quality”. It is technically correct. But, it is only meaningful if you assume a bad actor in the team who knowingly games the system, and a team who tolerates it. At that point, your problem has nothing to to with code quality, nor is coverage meant to be a solution for it.
- zarathustreal 3y ago
  You're missing the point entirely. This point is a counter example to the implied assertion that some percentage of code coverage indicates correctness. That alone is enough to prove that the implication is false. That being said, you don't need to assume a bad actor in the team to encounter the situation of having 100% code coverage with meaningless tests. Even the most well-meaning engineer can accidentally write tests that boil down to asserting "true == true" without proving anything about the code paths it touches. This isn't necessarily a cultural issue. I'd even assert that, due to the languages in common use, it's very common to see these sort of "touch all the code paths but assert nothing about the correctness of the code" tests. Standard OOP languages like TypeScript or Java for example have relatively limited type systems and allow mutable variables, so you end up with implementations of algorithms and data structures that don't lend themselves to property-based testing. This leads to tests which basically just duplicate the implementation and assert "I wrote what I wrote" or in other words "true == true".
  - srvaroa 3y ago
    > You don't need to assume a bad actor in the team to encounter the situation of having 100% code coverage with meaningless tests. Indeed. The thing is, I was replying to a comment that referred _explicitly_ to a bad actor. The situation @ponector describes (team's coverage indicator is rendered useless because have a bad actor games it), THEN the cause is not coverage. It says nothing about coverage beyond "a tool only works in certain conditions". You bring 2 more cases that break the indicator, and I agree with both (there are more!). We have (1): We all make mistakes and write dumb tests. (2): Coverage is not useful for _some_ practises / scopes of testing. I agree on both. But we're back on the same place. I'm not saying those problems don't exist. I'm saying that those are not problems coverage ever claimed to solve. Making a sweeping dismissal of a tool because it doesn't solve problems it never claimed to solve is throwing away the baby with the dirty water. * Coverage does not claim to be a tool to fix bad actor, (1) or (2)! There are other tools to cover those risks (e.g. managers, code reviews, pair programming, etc.). * Discussions about those tools (code reviews, etc.) tend to make the same mistake. Find problems the tool doesn't claim to solve to dismiss the tool. * This all happens because people pretend to treat tools like coverage, tests, DORA metrics, as silver bullets. They are not. They are all meant to be a toolbox that engineers evaluate and use where they yield value. And this is why yes, a lot of the "$tool is useless because $situation_where_it_doesnt_work" conversations are fundamentally about cultural issues. If your team uses a tool without knowing what problem is trying to solve, you have a cultural issue. If your tool has an actual purpose, and yet engineers are intentionally working around it, you have a cultural issue. Etc.

strken 3y ago

This works better as "All tests decreasing the overall percentage of tested code must have a good explanation signed off on by the reviewer". I've occasionally deleted dead code paths, had an almost entirely red diff, had close to 100% test coverage over the functions I touched, and yet decreased the overall percentage of tested code because the dead code path was more heavily tested than the rest of the code base.

stavros 3y ago
I kind of disagree, in that any rule should have an implied "you can ignore this with good justification". Otherwise you turn people into robots, which renders moot all the reasons why you have people, and not robots.
- strken 3y ago
  I agree with this - in practice any rule can be broken - but how easily it's broken and whether developers are explicitly told to break it is a process decision. How big a roadblock is your CI putting in the path of developers? If you require e.g. sign-off from a team lead or manager or whoever has "merge anyway" permissions in GitHub, that's more difficult than sign-off from the same reviewer who's reading your code anyway. You can make it easier or harder to break rules and in this case I think it should be so easy to break that there's a codified procedure to do so.
danielheath 3y ago
I haven’t tried that - in that situation we did “find something that needs a test”

berkes 3y ago

I like that. What I also do, is ensure test coverage is over 100%¹ for important parts. Important is designated through churn (if a file or class os changed on every second commit, it must be important) and through domain knowledge (building a recipe app, then likely the Recipe is important). ¹ covered by unit tests, AND (partially) covered by integration AND by E2E tests.

nonethewiser 3y ago

> IME, the only "test coverage % rule" that I've ever seen work was "must not decrease the overall percentage of tested code". That’s a stupid rule and not measuring what you think it is. It fails if you just delete tested code. It fails if you remove some code thats tested and add some tested code, but not enough. These are all extremely common for any refactor. Here is a simplistic example but imagine the same principles applied to a large set of changes. Dozens of files, thousands of lines. You cant just manually account for that. 50%: 100 lines of code. 50 covered, 50 uncovered. remove some function and its 44% from 40 covered, 50 uncovered. Failed. Or remove some function and replace it with something better thats not as long. 44 covered, 50 uncovered. 47% coverage. Failed. A stack overflow post about this. https://softwareengineering.stackexchange.com/questions/400745/unexpected-code-coverage-reduction https://softwareengineering.stackexchange.com/questions/4007... This inevitably leads to worthless tests to increase coverage and avoiding optionL refactors.

angra_mainyu 3y ago

It sounds great but I beg to differ. If I had to pick one rule that is great to always follow, is when you get hit with a bug, write tests that catch it and then write code to fix the test. Aside from preventing said bugs from resurfacing, it forces coverage of code that is likely complex enough to be buggy.

Fargren 3y ago

> "must not decrease the overall percentage of tested code" This rule has been a problem for me when deleting code. On my precious check we had an automated checker that wouldn't let us merge (easily) if we broke this rule. We had mediocre coverage. Some parts of the code had lots of tests, some parts were totally uncovered. Many times I had to delete code in the tested parts, and the checker would complain that total coverage went down because I deleted a tested line without adding more tested lines. But I agree it's a good rule in general, which is why we kept it despite the occasional hiccup.

pfdietz 3y ago

I'd worry about test coverage as soon as the team commits to actually fixing every bug found.