Fixes #3814 : implement package-level clinit suppression - #3815

Codecov Report

❌ Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.84%. Comparing base (c9fe6a0) to head (cbe3a09).

@@             Coverage Diff              @@
##               main    #3815      +/-   ##
============================================
- Coverage     86.84%   86.84%   -0.01%     
- Complexity     3032     3039       +7     
============================================
  Files           343      343              
  Lines          9152     9167      +15     
  Branches       1131     1135       +4     
============================================
+ Hits           7948     7961      +13     
- Misses          915      917       +2     
  Partials        289      289              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hi @raphw . Any chance you could look at this? 🙏

I have been thinking, but I am not sure if this is a good idea. New classes might do useful things in their initializers, like settings fields, and this could lead to unexpected breakage. I think this should list class names for this reason, not packages. As for the principle of least surpise. @mockito/developers What do you think?

I dunno, it seems harmless to me. This property is already kind of a power-user scenario so they probably know what they are doing.

As for the principle of least surpise. @mockito/developers What do you think?

I dunno, it seems harmless to me. This property is already kind of a power-user scenario so they probably know what they are doing.

I think #3801 is already somewhat already in that area. I think having a wildcard makes sense, and barely change that feature. Also, like Joshua mentioned, it's kind of a niche feature.

However, I do not think the whole feature is not harmless either ; while the mechnism is explicit in the build descriptor via a system property, it's not guaranteed to be visible enough. So I do think Mockito should log something, there's no println in the original PR. That feature is sufficiently hidden that it should print something at least for late contributors to such a code base.

Given it happens in the premain, I'm not sure we should use MockitoLogger (or other logger frameworks, this could cause too early class initialization), so when adding the log only rely on println.

Thanks everyone for taking the time to review.

@raphw I think that those are fair concerns. However, I personally think that the risk is very low since users need to explicitly set the property with a wildcard pattern, so it's unlikely someone would accidentally incur into it. Also, in legacy codebases, listing all the individual classes that need suppression can become fragile as new classes are added, in addition to being verbose.

In general, I find it a great suggestion about adding a log line when a class initializer is suppressed to improve visibility.

For my part, we can merge this than. There is no best solution, and I expect that people mostly mock packages they own. Libraries avoid extensive logic in static initializers for a reason. If there is no other comment, I will merge this within this week.