Improve error message if initializer fails. #2098
Conversation
raphw
force-pushed
the
initializer-errors
branch
from
f1c1a1c
to
cf70fd9
Compare
|
Indeed, just updated the PR. |
src/main/java/org/mockito/internal/creation/bytebuddy/InlineBytecodeGenerator.java
Outdated
Show resolved
Hide resolved
| join( | ||
| "Cannot instrument class that could not be initialized.", | ||
| "", | ||
| "A class that is not initializable would always fail during instrumentation.", |
There was a problem hiding this comment.
Could this be omitted because it is slightly reundant? The first sentence already mentions that a class which cannot be initialized cannot be instrumented.
| "Cannot instrument class that could not be initialized.", | ||
| "", | ||
| "A class that is not initializable would always fail during instrumentation.", | ||
| "Static initializers are never mocked by Mockito to avoid permanent disintegration of classes."), |
There was a problem hiding this comment.
Would it maybe good to prefix this with "Note: " / "Hint: " to indicate that this is only a hint, but most likely not something the user has to consider?
Or maybe omit it? Because this is a general hint and not directly related to the exception, unless a user tries to use Mockito to prevent an initialization error (which is rather unlikely).
Also "permanent disintegration of classes" might be too technical / unclear for the user?
| "A class that is not initializable would always fail during instrumentation.", | ||
| "Static initializers are never mocked by Mockito to avoid permanent disintegration of classes."), | ||
| e.getException()); | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
Is this actually needed? Because type already exists so a ClassNotFoundException cannot be thrown (except for JDK 15 hidden classes?). Therefore the only possible error is LinkageError, and should that actually be caught?
There was a problem hiding this comment.
Well, one never knows what JVMs do, especially if not HotSpot. Retaining the original exception is always a good idea.
There was a problem hiding this comment.
Ah sorry, I think you have misunderstood me. I meant that it might be good to remove this catch block which silently discards any Throwable, so the caller has to deal with them instead. Or only discard the relevant exceptions, i.e. ClassNotFoundException for hidden classes?
This catch block was likely also the reason why the previous error (which this pull request improves) was so cryptic / did not contain the relevant information.
And sorry in case my previous review comments were too intrusive.
Codecov Report
@@ Coverage Diff @@
## release/3.x #2098 +/- ##
=================================================
- Coverage 84.80% 84.73% -0.08%
- Complexity 2716 2717 +1
=================================================
Files 325 325
Lines 8266 8278 +12
Branches 988 989 +1
=================================================
+ Hits 7010 7014 +4
- Misses 982 993 +11
+ Partials 274 271 -3
Continue to review full report at Codecov.
|
raphw
force-pushed
the
initializer-errors
branch
from
f219efb
to
de81334
Compare
…tecodeGenerator.java Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>
raphw
force-pushed
the
initializer-errors
branch
from
de81334
to
3a263de
Compare
Non-initializable classes can never be mocked, an error message should help guide on these issues.