- stryker-mutator (C#, Typescript)
- pitest (Java)
- mutatest (Python)
Interesting examples:
> San Francisco Declaration on Research Assessment – 2012 manifesto against using the journal impact factor to assess a scientist's work. The statement denounces several problems in science and as Goodhart's law explains, one of them is that measurement has become a target. The correlation between h-index and scientific awards is decreasing since widespread usage of h-index.
> International Union for Conservation of Nature's measure of extinction can be used to remove environmental protections, which resulted in IUCN becoming more conservative in labeling something as extinct
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.
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 - but that's going to be an uphill battle.
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.
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.
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.
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.
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".
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.
> 100% tells you nothing - but that _any other percentage_ does tell you something.
100% doesn't tell you it is meaningful coverage, but less than 100% tells you for sure that uncovered part doesn't have meaningful coverage.
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.
My other favourite of trivial code that's broken: returning the same Iterator instance in Iterable.iterator()
I remember hibernate recommending using static hash. In order to prevent saving entities changing hash values.
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.
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/4007...
This inevitably leads to worthless tests to increase coverage and avoiding optionL refactors.
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.
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.
I agree that unit testing for the sake of KPI's is the wrong approach and unit testing functionality (as a means of documenting it, proving it still works as intended) is far better.
Memory.
> Nothing is actually happening unless you depend on constructors/destructors when you use reflection to gen a mock type or real value type to inject.
Just get an object from IoC container, then test that getFoo() == getFoo(setFoo(getFoo())) for every Foo with get and set methods, so you will have 80% of coverage for those getters and setters. For read-only properties, just get value and throw it away.
> I agree that unit testing for the sake of KPI's is the wrong approach and unit testing functionality (as a means of documenting it, proving it still works as intended) is far better.
Unit tests are for proving correctness of logic in a unit of code. Data structures are not logic. IMHO, such trivial parts of program should be ignored by coverage tool, but topic starter said that they cannot change rules. :-/
> Memory.
In unit testing, the "memory" is not the system under test though.
It doesn't make sense to test something you have no control over, if the OS fails to allocate memory there's nothing you can change in your code to fix that.
Likewise, if you're improperly instructing the OS to allocate memory (via some constructor or factory or whatever) there is no test you can write to test your own intentions that won't be subject to exactly the same level of incorrectness as the implementation code itself. If you've written "do the thing" writing a "test" that says "hey make sure I wrote 'do the thing'" is ridiculous.
I've often found this to be a matter of semantics. Someone will say they're writing a "unit test" and load all the assertions and logic associated with the meaning of "unit test" into their brain and then try to apply it in a way that subtly invalidates those assertions when they start writing.
To make things worse, there's a cultural disinclination toward "semantic arguments" so you end up arguing with people that have basically no chance of understanding why what they're doing doesn't provide the value it should
private int value1;
private int value2;
public void setValue2(int value2) {
this.value2 = value2;
}
public void setValue1(int value1) {
if (this.value2 > 0) {
this.value1 = value1 + this.value2;
} else {
this.value1 = value1;
}
}
Parse, don't validate.
I didn't like that I had tobdo it, but it was easier than getting several approvals to get Sonar rules changed.
When you create a List you want to enable the possibility of setting a Capacity - how large can this List be.
And you also want to enable reading how large the List is at a given point.
Pretty valid case for a setter and a getter in my book.
Do you want to return a copy/clone of the inner object or just let people mutate it, destroying all of your data structure's invariants? Yes, if you change the implementation of the structure, user code will also break and it is certainly easier to do that behind a method, this is indeed rare in practice but the nuisance of migrating direct access to indirect access is bigger than the nuisance of (today's) unnecessary indirection.
Since in some cases safety around invariants and future proofing will require the level of indirection, it is easier to just expect the convention. Moreover, codegen can just produce them for you, they can be excluded from test coverage. Then there are languages like python that allow you in the future to pretend that indirect access is direct access and cannot protect invariants in any case, so just go direct from day one.
All the hoops for this... Codegen, excluding from coverage, Lombok, and all that magic to hide you from a simple obj.field that is all you're doing in 99.9% of cases. It's just so rare in practice. I think the language itself should allow for readonly fields and property setters for the extremely rare occasions where you need this.
As a general rule most fields should be initialized in the c-tor, and they should be final.
Only goes to confirm my view of them that the language is deficient
That is very squarely a people problem.
It’s not as bad today - many Java juniors don’t have the “bean” affliction burned into their brains so they don’t object to public fields on data carriers (today you’d just use a record) but even the bean generation (mostly people my age) can usually be won over these days by negotiating with them on cases where getters/setters can be eliminated (e.g. start with value objects, then suggest maybe DTOs then you can go for the kill - why do we need the stupid Java bean convention?)
The problem on this topic is that they cargo cult getters/setters by inertia, usually because more experienced engs pass it on as good practice.
It’s not an inherent problem to the language.
They knew that people would write coverage tests for getters and setters, and calculated that eventuality into their minimums.
The result was a header with:
static const unsigned ONE = 1;
static const unsigned TWO = 2;
static const unsigned THREE = 3;
...
That some numbers are not magic would be obvious to a human reviewer, but the tool probably just treats any number in an expression as a magic number or something like that, and the workaround is to define constants for raw numbers. Which entirely defeats the purpose since now, people will just use these constants for actual magic numbers and the tool will see nothing.
But this is just 3 values in an expression and using a constant could actually be bad. Let's be a bit more practical.
int lightness(int r, int g, int b) { return (r+g+b)/3; }
int lightness(int r, int g, int b) {
const int NUMBER_ELEMENTS_AVERAGED = 3;
return (r+g+b)/NUMBER_ELEMENTS_AVERAGED;
}
const int NUMBER_OF_COLOR_COMPONENTS = 3;
int lightness(int r, int g, int b) {
return (r+g+b)/NUMBER_OF_COLOR_COMPONENTS;
}
Side note: I know it is the wrong formula for lightness, that's just an example.
You either have them on a list and calculate it dynamically based on the size, or have it as a magic number.
const MILLIS_PER_SECOND = 1000;
...
...
...
...
...
const durationMs = MILLIS_PER_SECOND * duration
really clearer than
const durationMs = 1000 * duration
?
long delay = (5 * 60 * 1000); // 5 minutes, in millseconds
sleep(delay) always looks ok, but sleep(delayHours) is probably going to catch your eye as suspicious.
const duration = 1000 * duration
const duration_ms = 1000 * duration_s
And _us for microsec.
const duration_us = 1000 * duration_ms
But then the tool would probably reject my code for not following the naming conventions which ”disallows using underscores in variable names”.
Guess what I wanted to say is that there are always exceptions to the rule and there should always be some way to turn off the automatic checker for certain sections of the code.
Say in some UI code called FooBarItem you suddenly have some call to set padding to 12. People will take this number, put it in constant and name it FOO_BAR_ITEM_PADDING = 12.
This is not better. It’s just jumping more through the code, and whatever is in the name of the constant is easily deductible from the usage pattern.
I learnt that pattern from others and nowadays I see it as useless. If you can add interesting info in the name or comments of the number, don’t bother extracting a constant.
Is it really difficult to see that these are pointless? If you actually think about what you are writing, that is.
To me that reads like your team fucked up at a very fundamental level, as they both failed to take into account the whole point of automated tests and also everyone failed to flag those nonsense tests as a critical blocker for the PR.
Unless your getters aand setters are dead code, they are already exercised by any test covering the happy path. Also, a 80% coverage target leaves out plenty of headroom to leave out stupid getter/setter tests.
A team that pulls this sort of stunt is a team that has opted to develop defensive tricks to preserve their incompetence instead of working on having in place something that actually benefits them.
https://en.wikipedia.org/wiki/Perverse_incentive#The_origina...
It comes up a bit on hacker news https://hn.algolia.com/?dateRange=all&page=0&prefix=true&que...
I have never seen such poorly written application in my 6 years of experience (and I am not only talking about style, stuff was absolutely utterly broken, while they had no clue what's wrong).
I hate Sonar with passion. It only ever should be used to report vulnerabilities, not telling me to rename variables or that i "should refactor this code duplication!" I already have a fucking backlog with Jira tickets, don't tell what i am supposed or not to do, and when i am supposed to do it.
But oh boy mangers love this stupid power burner
Then management sweeps in and is trying to add sonar, and it's a nightmare. Besides tripling our total build time to run this horrible tool, they want us to waste time rewriting our codebase to follow these insane rules like "cognitive complexity", and editor integration that takes several seconds to update after every file change.
When it's used thoughtlessly, as it often is, it's terrible.
Classic
- Categorizing Variants of Goodhart's Law [1]
- Building less-flawed metrics: Understanding and creating better measurement and incentive systems [2]
[1] https://arxiv.org/abs/1803.04585
[2] https://www.sciencedirect.com/science/article/pii/S266638992...
I attended many TOC conferences in the 90s and early 2000s. Eli Goldratt was famous for saying "Tell me how you'll measure me, and I'll tell you how I will behave."
The problem is, the ugly, careless code extremely very well tested: 95% code coverage. My replacement had 100% code coverage... but by being far shorter, my PR couldn't pass tests, as total coverage went down, not up. The remaining code in the repo? A bunch of Swing UI code, the kind that is hard to test, and where the test don't mean anything. So facing the prospect of spending a week or two writing swing test, the dev lead decided that it was best to keep the old code somewhere in the repo, with tests pointing at it, just never called in production.
Thousands of lines of completely dead, but very well covered code were kept in the repo to keep Sonar happy.
While there are people trying with great effort to make computer as intelligent as humans, there are countless organization trying to make humans as dumb as computers by making them adhere to arbitrary numbers without giving them any agency on the evaluation of the usefulness of the metric…
Sometimes it is attached to incentives, like monetary rewards (bonuses and promotions). I've seen someone promoted on the back of a "high-impact" project that was bug-riddled and constituted more than half the support calls for the rest of the team for at least a year, not to mention financial penalties for the organization. It wasn't laziness or apathy, just rational actors optimizing benefits from the current rules with inadequate oversight and/or penalties for adverse outcomes.
Was the high impact project rushed or was it staffed with crappy devs? Or both? Usually that is the reason for bad outcomes.
A big problem is making it mandatory with huge bureaucracy to avoid its stupidness. Just last week I was battling yet another code quality tool they made mandatory: it was complaining that my res.status(200).json() wasn't setting up HSTS headers. And then I tried setting it up manually, it kept complaining, app.use(helmet()), same thing. Apparently it wanted me to write the whole backend code into a single file for it to stop complaining. And of course, HSTS is much more elegantly and automatically handled by the ingress or load balancer itself.
I could have spent a week or two flagging it as a false positive and explaining what is HSTS for upper management to approve it. I ended up just adding a res.sendJson(data, status = 200) to the prototype of the response object. Which is obviously stupid, but working in a bureaucracy-heavy sector has made me realize how much of bad software is composed of many such bad implementations combined.
Something I've learned along the way as well. A few times in my career I will end up working under a manager that insists only work "that can be explicitly measured" be performed. That means they forbade library upgrades, refactors, things like that because you couldn't really prove an improvement in customer metrics or immediate changes in eng productivity.
I've also been at companies that follow that mantra more broadly and apply it to eng performance reviews. The entire culture turns into engineers focusing on either short term gains without regard for long term impact, or gaming metrics for meaningless changes and making them look important and impactful.
Important but thankless work gets left behind because, again, it's work that is not "immediately measurable." End result is a bunch of features customers hate (eg dark patterns), and a rickety codebase that everyone is disincentivised fix or improve.
My personal favorite was tests that a team member introduced for an object that had all the default runtime parameters in it. Think like how long should a timeout be by default, before getting overridden by instance specific settings. This team member introduced a test that checked that each value was the same as it was configured to.
So if I wanted to update or add a default, I had to write it in two places, the actual default value, and the unit test that checked if the defaults were the same as the test required.
Career QA here. E2E are the absolute highest pevel of test, that take the most time to implement, have the most dependencies, and tell you the least about what is actually wrong.
If you think finding what broke is painful with full suites of unit/integration tests under the E2E suite is bad, throw those out or stop maintaining them at all. Let me know how it goes.
Ideally, the code coverage tool would have heuristics to detect trivial getter/setter methods, and filter them out, so adding tests for them won't improve code coverage. Non-trivial getters/setters (where there is some actual non-trivial logic involved) shouldn't be filtered, since they should be tested.
Although, there is room for debate about what counts as trivial. Obviously this is trivial:
public void setUser(User user) {
this.user = user;
}
public void setUser(User user) {
this.user = Objects.requireNonNull(user);
}
public void setOwners(List<User> owners) {
this.owners = List.copyOf(owners);
}
However we might think about this differently if we flipped it to "our standard is 20% completely untested".
Uncoverage communicates the value much better.
With Java at least, it seems to drive people to do things like not catch exceptions. It's hard to inject errors so all the catch blocks are hit.
Also makes people not want to switch to use record classes, as that feature removes class boilerplate that is easy to cover.
As a young dev, it taught me that focusing only on KPIs can sometimes drive behaviors that don't align with the intended goals. A few well-thought out E2E test scenarios would probably have had a better impact on the software quality.