Fixes unchecked warnings when calling Mockito.mock(Class) #4413
Conversation
|
Thank you for this PR, but the problem with this fix is that once the Mockito project has taken this problem into account, we'll still have a method that's of no use. Rather than bypassing the problem in JP, it would be better to insist that the Mockito project take this problem into account. |
|
I agree on the principle that "it would be better to insist that the Mockito project take this problem into account". But at the current state of things, it seems to be a rather speculative argument:
In short, it looks like neither the Mockito team is willing to do it, nor any one in the open source community able to provide a satisfying solution for it after 6 years. We could have argued that it was a niche issue having low attention, but it is not: any Mockito user faces it from the start, since mock() is a core method, used massively, and the warning comes as soon as we mock a generic class, which is rather frequent. So it is a systematic issue that still has no solution after 6 years, which is prone to support that it is a design flow that cannot be fixed easily. I am a newbie in JavaParser, so it might be not a good comparison, but it makes me think about the LexicalPreservingPrinter and its limits in keeping the formatting also for inserted code. Even if someone "insisted" that the JavaParser project should take this problem into account, it does not look like it would solve itself. If we discard the idea of getting anything from Mockito itself, then it comes down to what JavaParser would do. Here we speak about 100 warnings in GenericVisitorAdapterTest and 100 warnings in GenericListVisitorAdapterTest. I don't know how you managed to keep them at exactly 100 each, but good job! {^o^} 200 warnings is a lot, though. Enough to say that one or two more warnings won't change anything, which breaks the incentive to fix them at all. So it looks to me that if you don't do anything here, there is barely any point to fix any other warning, since we will remain with a lot of them anyway. Given how trivial is your use of Mockito, I think that the proposed solution gives a rather compelling warning fix:
There is other ways to make these warnings disappear, but this compromise seems the most reasonable to me. |
This is an issue well known by the Mockito community, but prone to not be fixed: mockito/mockito#1531 Generics allow to ensure type-safety at compile time instead of runtime. However, here it is used mainly to auto-cast the created object, thus avoiding this burden on the developer. Indeed, it is in a context where type safety is usually not a requirement, since the use of this method is often the most trivial we can have: provide a class and expect a very instance of this class in return. This commit thus creates a less constrained mock method which builds on Mockito's one, but without the constraint inducing this warning.
matthieu-vergne
force-pushed
the
fix_warnings_mock
branch
from
8c6d25f
to
0309e91
Compare
|
By the way, if Mockito happens to fix it, it means that the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4413 +/- ##
=========================================
Coverage 51.967% 51.967%
=========================================
Files 505 505
Lines 28456 28456
Branches 4930 4930
=========================================
Hits 14788 14788
Misses 11616 11616
Partials 2052 2052
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
This is an issue well known by the Mockito community, but prone to not be fixed:
mockito/mockito#1531
Generics allow to ensure type-safety at compile time instead of runtime. However, here it is used mainly to auto-cast the created object, thus avoiding this burden on the developer. Indeed, it is in a context where type safety is usually not a requirement, since the use of this method is often the most trivial we can have: provide a class and expect a very instance of this class in return.
This commit thus creates a less constrained mock method which builds on Mockito's one, but without the constraint inducing this warning.