11 ms·

At a large insurance company I worked for, management set a target of 80% test coverage across our entire codebase. So people started writing stupid unit test

At a large insurance company I worked for, management set a target of 80% test coverage across our entire codebase. So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal. Of course devs also weren't allowed to change the coverage measuring rules in Sonar. 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.

awwaiid 3y ago
True. However we might think about this differently if we flipped it to "our standard is 20% completely untested". Uncoverage communicates the value much better.
farhanhubble 3y ago
True of any metric when it becomes the goal in itself.
oneshtein 3y ago
Is it hard to write ONE test case for ALL getters and setters using reflection?
- reactordev 3y ago
  at that point what are you testing? allocation/deallocation? 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. Or do you mean just assert true that all getters are not null? 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.
  - oneshtein 3y ago
    > at that point what are you testing? 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. :-/
    - growse 3y ago
      > > at that point what are you testing? > Memory. In unit testing, the "memory" is not the system under test though.
    - zarathustreal 3y ago
      >> at that point what are you testing? > Memory. 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
    - Mawr 3y ago
      You shouldn't have setters in the first place. What's the point of encapsulation if anyone can just set properties at will?
- ildjarn 3y ago
  Simplest solution? Don’t have any setters.
  - thomasahle 3y ago
    Removing them would reduce your coverage percentage.
    - srvaroa 3y ago
      Good thing. It shows that a % of your tests were actually not targeted at the most important areas of the code base
- tauwauwau 3y ago
  I had to do it in one project. It's not trivial, but also not too difficult. Getters were easy, setters required different types of values. After handling various date type values for setters it worked fine. Occasionally I would see an exception for fields generated by APIs like Lombok, which needed to be excluded from my setter list. I didn't like that I had tobdo it, but it was easier than getting several approvals to get Sonar rules changed.
- jghn 3y ago
  Or stop using the broken pattern of getters and setters
  - geraldwhen 3y ago
    I ask interview candidates to explain why they’re using getters or setters. They never have a reason.
    - drowsspa 3y ago
      Would "because it's conventional and most tools and developers assume them" be an acceptable answer? The reason I read is "because you might wanna change the implementation" but honestly it's seems very rare to need to do that
      - BlackFly 3y ago
        Because your data structure and your API are separate things and generally speaking, the data structure should be opaque to users of that structure unless it's sole purpose is as a record for a bunch of request arguments. For those fat request structures, it ends up being a lot of noise indeed, but in any case most of those structures for most people are dealt with by deserialization and you can often skip generation of setters and getters or autogenerate them. 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.
        drowsspa 3y ago
- xxs 3y ago
  The correct solution is not having either. Getter/setters are effectively an anti pattern as they support needless mutability but worse they add nothing of value. As a general rule most fields should be initialized in the c-tor, and they should be final.
supriyo-biswas 3y ago
https://en.wikipedia.org/wiki/Goodhart%27s_law https://en.wikipedia.org/wiki/Goodhart%27s_law
- hubraumhugo 3y ago
  Thanks, I was looking for that name. 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
raverbashing 3y ago
Java is the only place where (non-automatic/non-syntax-sugared) getters and setters are though as important and valuable Only goes to confirm my view of them that the language is deficient
- hubraumhugo 3y ago
  Record types solved this in JDK 14
  - groestl 3y ago
    I mean public final was there before as well.
  - drowsspa 3y ago
    Cries in Java 8
- CraigJPerry 3y ago
  I don’t think it’s accurate to label that as a language problem. 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?)
- Slartie 3y ago
  Modern Java projects either use records, org.immutables or Lombok. Manual getter/setter creation is the exception nowadays.
- srvaroa 3y ago
  I don’t find they are valued like that. IME most devs who use them perceive them as annoying boilerplate. Proof being how Lombok or Records are generally well received among the younger gen. 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.
dclowd9901 3y ago
What if… They knew that people would write coverage tests for getters and setters, and calculated that eventuality into their minimums.
- mrich 3y ago
  So you're saying they knew engineers would be wasting their time doing useless things, but still went ahead? (instead of mandating 75% and spending 1/100 of the wasted time to adjust the metric to filter out getter/setter)
  - dev_tty01 3y ago
    I just assumed they wrote a script to automatically generate all the setter/getter tests and then took a long lunch.
danielheath 3y ago
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.
dexwiz 3y ago
I’ve started getting pinged for this at my current job. I think it’s time to move on.
wilgertvelinga 3y ago
The solution to that is mutation tests. They force your tests to actually verify the implementation instead of just running the code to fake coverage. https://en.m.wikipedia.org/wiki/Mutation_testing https://en.m.wikipedia.org/wiki/Mutation_testing Tools and frameworks exist for almost all languages. Some examples: - stryker-mutator (C#, Typescript) - pitest (Java) - mutatest (Python)
- DylanSp 3y ago
  The paper in the OP mentions mutation testing at the end when talking about future work and open problems (section 7). They think it'd be very useful, but it's "challenging to deploy such computationally demanding techniques at the scale we would require".
- misja111 3y ago
  I have tried those out for a large Java project. The problem is that it is just too slow to use with a large unit test suite. What mutation testing does, is running all the tests in your suite multiple times while changing some of your code base ("mutation points") to see if it affects the test outcome (it should). So say that your test suite normally takes 10 minutes, then a full mutation suite can easily be a factor 50 slower, which means 8 hours.
  - jononor 3y ago
    Perfect for a daily CI job?
    - polynomial 3y ago
      "daily CI"?
      - rafaelmn 3y ago
        People tend to use CI as a synonym for automated build pipeline.
Galanwe 3y ago
I can relate. At my first internship, there was a code quality tool forced on the team by management as well. It had a rule to "disable magic numbers". The result was a header with: static const unsigned ONE = 1; static const unsigned TWO = 2; static const unsigned THREE = 3; ... Up to some thousands.
- isoprophlex 3y ago
  Heavens forbid you want to raise some number to the power of 2, err I mean TWO
- awestroke 3y ago
  Really? You could not come up with a way to avoid magic numbers?
  - Retric 3y ago
    Many uses of numbers aren’t magic numbers. If you want to count something then setting the counter to 0 and incrementing by 1 isn’t unexplained or subject to change.
  - GuB-42 3y ago
    Sometimes, a number is just a number. The average of a, b, c is (a+b+c)/3, an positive interger is single digit when less then 10, the formula for the volume of a sphere is 4/3PIr^3, etc... And that's excluding 0, 1 and 2 which are naturally everywhere. 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.
    - jader201 3y ago
      Maybe in rare cases, but in my experience, literals (numbers or strings) almost always have _some_ meaning where giving them a name helps readability. In my experience, it’s rare that “a number is just a number”. And sure, in those cases, naming them is silly.
xLaszlo 3y ago
This is called "Goodhart's Law"
foofie 3y ago
> So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal. 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.
- drowsspa 3y ago
  The incentives are obviously pointing towards this. And that it is so common should make you rethink your stance of "they're just incompetent".
dataviz1000 3y ago
At least they didn’t do what IBM did, write tests and pay coding farms to write the code to satisfy the unit tests.
piersj225 3y ago
Yup, I believe this is the Cobra Effect https://en.wikipedia.org/wiki/Perverse_incentive#The_original_cobra_effect 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&query=The%20Cobra%20Effect%28https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FCobra_effect%29&sort=byPopularity&type=story https://hn.algolia.com/?dateRange=all&page=0&prefix=true&que...
dgan 3y ago
We have a mandatory Sonar scan, and when I was hired, my tech lead proudly shown me the "A" grade they have been attributed, and said something like "we have a high standard to maintain" 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
- Klathmon 3y ago
  I'm fighting against this now. We already use linters built and tuned by engineering, we have custom rules that solve real paint points and disabled everything that the team doesn't like or doesn't think provides value. 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.
  - stavros 3y ago
    I don't understand this. Why is management telling you what to do? In our company, the managers ask/push the teams to improve some metric, but the teams are responsible for the how. The metric is usually high-level, like "reduce the number of bugs reported", not "increase code coverage".
  - therealdrag0 3y ago
    I think having cognitive complexity indicators are great. Many devs don’t think about that so having the visibility reminder is good on average. However it should not be blocking in any manner and it’s silly to drive a rewrite based on it. Engineering leaders should understand the architecture at a visceral level, know good patterns/design, and suggest refactors without static analysis tools.
carlossouza 3y ago
The Cobra Effect https://en.wikipedia.org/wiki/Perverse_incentive https://en.wikipedia.org/wiki/Perverse_incentive Classic
- divan 3y ago
  Must reads: - 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 https://arxiv.org/abs/1803.04585 [2] https://www.sciencedirect.com/science/article/pii/S2666389923002210 https://www.sciencedirect.com/science/article/pii/S266638992...
jes 3y ago
> At a large insurance company I worked for, management set a target of 80% test coverage across our entire codebase. So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal. 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."
hibikir 3y ago
My favorite anecdote on the topic involved a codebase like this, handled by inexperienced programmers. I came into the team, and realized that a whole lot of careless logic could be massively simplified, so I sent a PR cutting the codebase by 20%, and still passing all the tests and meeting user requirements. 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.
- littlestymaar 3y ago
  My goodness… 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…
  - osigurdson 3y ago
    This is mostly just laziness and apathy. Don't try to learn about the problems that the business actually has, instead focus on a simple metric and the enforcement of it.
    - sangnoir 3y ago
      > This is mostly just laziness and apathy. 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.
drowsspa 3y ago
"When a metric becomes a target, it ceases to be a good metric". 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.
- pfdietz 3y ago
  Goodhart's Law
seattle_spring 3y ago
> As a young dev, it taught me that focusing only on KPIs can sometimes drive behaviors that don't align with the intended goals 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.
kevin_nisbet 3y ago
Yea, I've seen this get carried away even by just individual team members. 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.
salawat 3y ago
And you'd be dead wrong. 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.
skissane 3y ago
> So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal 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; } But should this count as trivial too? public void setUser(User user) { this.user = Objects.requireNonNull(user); } Probably. What about this? public void setOwners(List<User> owners) { this.owners = List.copyOf(owners); } Probably that too. Which suggests, maybe, there ought to be a configurable list of methods, whose presence is ignored when determining whether a getter/setter is trivial or not.
nitwit005 3y ago
I'm convinced high coverage targets cause people to write worse code. 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.
justapassenger 3y ago
[dead]

awwaiid 3y ago

True. However we might think about this differently if we flipped it to "our standard is 20% completely untested". Uncoverage communicates the value much better.

farhanhubble 3y ago

True of any metric when it becomes the goal in itself.

oneshtein 3y ago

Is it hard to write ONE test case for ALL getters and setters using reflection?

reactordev 3y ago
at that point what are you testing? allocation/deallocation? 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. Or do you mean just assert true that all getters are not null? 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.
- oneshtein 3y ago
  > at that point what are you testing? 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. :-/
  - growse 3y ago
    > > at that point what are you testing? > Memory. In unit testing, the "memory" is not the system under test though.
  - zarathustreal 3y ago
    >> at that point what are you testing? > Memory. 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
  - Mawr 3y ago
    You shouldn't have setters in the first place. What's the point of encapsulation if anyone can just set properties at will?
ildjarn 3y ago
Simplest solution? Don’t have any setters.
- thomasahle 3y ago
  Removing them would reduce your coverage percentage.
  - srvaroa 3y ago
    Good thing. It shows that a % of your tests were actually not targeted at the most important areas of the code base
tauwauwau 3y ago
I had to do it in one project. It's not trivial, but also not too difficult. Getters were easy, setters required different types of values. After handling various date type values for setters it worked fine. Occasionally I would see an exception for fields generated by APIs like Lombok, which needed to be excluded from my setter list. I didn't like that I had tobdo it, but it was easier than getting several approvals to get Sonar rules changed.
jghn 3y ago
Or stop using the broken pattern of getters and setters
- geraldwhen 3y ago
  I ask interview candidates to explain why they’re using getters or setters. They never have a reason.
  - drowsspa 3y ago
    Would "because it's conventional and most tools and developers assume them" be an acceptable answer? The reason I read is "because you might wanna change the implementation" but honestly it's seems very rare to need to do that
    - BlackFly 3y ago
      Because your data structure and your API are separate things and generally speaking, the data structure should be opaque to users of that structure unless it's sole purpose is as a record for a bunch of request arguments. For those fat request structures, it ends up being a lot of noise indeed, but in any case most of those structures for most people are dealt with by deserialization and you can often skip generation of setters and getters or autogenerate them. 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.
      - drowsspa 3y ago
xxs 3y ago
The correct solution is not having either. Getter/setters are effectively an anti pattern as they support needless mutability but worse they add nothing of value. As a general rule most fields should be initialized in the c-tor, and they should be final.

supriyo-biswas 3y ago

https://en.wikipedia.org/wiki/Goodhart%27s_law https://en.wikipedia.org/wiki/Goodhart%27s_law

hubraumhugo 3y ago
Thanks, I was looking for that name. 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

raverbashing 3y ago

Java is the only place where (non-automatic/non-syntax-sugared) getters and setters are though as important and valuable Only goes to confirm my view of them that the language is deficient

hubraumhugo 3y ago
Record types solved this in JDK 14
- groestl 3y ago
  I mean public final was there before as well.
- drowsspa 3y ago
  Cries in Java 8
CraigJPerry 3y ago
I don’t think it’s accurate to label that as a language problem. 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?)
Slartie 3y ago
Modern Java projects either use records, org.immutables or Lombok. Manual getter/setter creation is the exception nowadays.
srvaroa 3y ago
I don’t find they are valued like that. IME most devs who use them perceive them as annoying boilerplate. Proof being how Lombok or Records are generally well received among the younger gen. 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.

dclowd9901 3y ago

What if… They knew that people would write coverage tests for getters and setters, and calculated that eventuality into their minimums.

mrich 3y ago
So you're saying they knew engineers would be wasting their time doing useless things, but still went ahead? (instead of mandating 75% and spending 1/100 of the wasted time to adjust the metric to filter out getter/setter)
- dev_tty01 3y ago
  I just assumed they wrote a script to automatically generate all the setter/getter tests and then took a long lunch.

danielheath 3y ago

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.

dexwiz 3y ago

I’ve started getting pinged for this at my current job. I think it’s time to move on.

wilgertvelinga 3y ago

The solution to that is mutation tests. They force your tests to actually verify the implementation instead of just running the code to fake coverage. https://en.m.wikipedia.org/wiki/Mutation_testing https://en.m.wikipedia.org/wiki/Mutation_testing Tools and frameworks exist for almost all languages. Some examples: - stryker-mutator (C#, Typescript) - pitest (Java) - mutatest (Python)

DylanSp 3y ago
The paper in the OP mentions mutation testing at the end when talking about future work and open problems (section 7). They think it'd be very useful, but it's "challenging to deploy such computationally demanding techniques at the scale we would require".
misja111 3y ago
I have tried those out for a large Java project. The problem is that it is just too slow to use with a large unit test suite. What mutation testing does, is running all the tests in your suite multiple times while changing some of your code base ("mutation points") to see if it affects the test outcome (it should). So say that your test suite normally takes 10 minutes, then a full mutation suite can easily be a factor 50 slower, which means 8 hours.
- jononor 3y ago
  Perfect for a daily CI job?
  - polynomial 3y ago
    "daily CI"?
    - rafaelmn 3y ago
      People tend to use CI as a synonym for automated build pipeline.

Galanwe 3y ago

I can relate. At my first internship, there was a code quality tool forced on the team by management as well. It had a rule to "disable magic numbers". The result was a header with: static const unsigned ONE = 1; static const unsigned TWO = 2; static const unsigned THREE = 3; ... Up to some thousands.

isoprophlex 3y ago
Heavens forbid you want to raise some number to the power of 2, err I mean TWO
awestroke 3y ago
Really? You could not come up with a way to avoid magic numbers?
- Retric 3y ago
  Many uses of numbers aren’t magic numbers. If you want to count something then setting the counter to 0 and incrementing by 1 isn’t unexplained or subject to change.
- GuB-42 3y ago
  Sometimes, a number is just a number. The average of a, b, c is (a+b+c)/3, an positive interger is single digit when less then 10, the formula for the volume of a sphere is 4/3PIr^3, etc... And that's excluding 0, 1 and 2 which are naturally everywhere. 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.
  - jader201 3y ago
    Maybe in rare cases, but in my experience, literals (numbers or strings) almost always have _some_ meaning where giving them a name helps readability. In my experience, it’s rare that “a number is just a number”. And sure, in those cases, naming them is silly.

xLaszlo 3y ago

This is called "Goodhart's Law"

foofie 3y ago

> So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal. 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.

drowsspa 3y ago
The incentives are obviously pointing towards this. And that it is so common should make you rethink your stance of "they're just incompetent".

dataviz1000 3y ago

At least they didn’t do what IBM did, write tests and pay coding farms to write the code to satisfy the unit tests.

piersj225 3y ago

Yup, I believe this is the Cobra Effect https://en.wikipedia.org/wiki/Perverse_incentive#The_original_cobra_effect 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&query=The%20Cobra%20Effect%28https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FCobra_effect%29&sort=byPopularity&type=story https://hn.algolia.com/?dateRange=all&page=0&prefix=true&que...

dgan 3y ago

We have a mandatory Sonar scan, and when I was hired, my tech lead proudly shown me the "A" grade they have been attributed, and said something like "we have a high standard to maintain" 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

Klathmon 3y ago
I'm fighting against this now. We already use linters built and tuned by engineering, we have custom rules that solve real paint points and disabled everything that the team doesn't like or doesn't think provides value. 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.
- stavros 3y ago
  I don't understand this. Why is management telling you what to do? In our company, the managers ask/push the teams to improve some metric, but the teams are responsible for the how. The metric is usually high-level, like "reduce the number of bugs reported", not "increase code coverage".
- therealdrag0 3y ago
  I think having cognitive complexity indicators are great. Many devs don’t think about that so having the visibility reminder is good on average. However it should not be blocking in any manner and it’s silly to drive a rewrite based on it. Engineering leaders should understand the architecture at a visceral level, know good patterns/design, and suggest refactors without static analysis tools.

carlossouza 3y ago

The Cobra Effect https://en.wikipedia.org/wiki/Perverse_incentive https://en.wikipedia.org/wiki/Perverse_incentive Classic

divan 3y ago
Must reads: - 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 https://arxiv.org/abs/1803.04585 [2] https://www.sciencedirect.com/science/article/pii/S2666389923002210 https://www.sciencedirect.com/science/article/pii/S266638992...

jes 3y ago

> At a large insurance company I worked for, management set a target of 80% test coverage across our entire codebase. So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal. 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."

hibikir 3y ago

My favorite anecdote on the topic involved a codebase like this, handled by inexperienced programmers. I came into the team, and realized that a whole lot of careless logic could be massively simplified, so I sent a PR cutting the codebase by 20%, and still passing all the tests and meeting user requirements. 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.

littlestymaar 3y ago
My goodness… 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…
- osigurdson 3y ago
  This is mostly just laziness and apathy. Don't try to learn about the problems that the business actually has, instead focus on a simple metric and the enforcement of it.
  - sangnoir 3y ago
    > This is mostly just laziness and apathy. 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.

drowsspa 3y ago

"When a metric becomes a target, it ceases to be a good metric". 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.

pfdietz 3y ago
Goodhart's Law

seattle_spring 3y ago

> As a young dev, it taught me that focusing only on KPIs can sometimes drive behaviors that don't align with the intended goals 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.

kevin_nisbet 3y ago

Yea, I've seen this get carried away even by just individual team members. 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.

salawat 3y ago

And you'd be dead wrong. 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.

skissane 3y ago

> So people started writing stupid unit tests for getters and setters in Java DTOs to reach the goal 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; } But should this count as trivial too? public void setUser(User user) { this.user = Objects.requireNonNull(user); } Probably. What about this? public void setOwners(List<User> owners) { this.owners = List.copyOf(owners); } Probably that too. Which suggests, maybe, there ought to be a configurable list of methods, whose presence is ignored when determining whether a getter/setter is trivial or not.

nitwit005 3y ago

I'm convinced high coverage targets cause people to write worse code. 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.

justapassenger 3y ago

[dead]