fix: Incorrect method reference used in LazyTraceScheduledThreadPoolExecutor #1783
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1783 +/- ##
=============================================
+ Coverage 22.72% 35.26% +12.54%
- Complexity 542 814 +272
=============================================
Files 263 240 -23
Lines 6587 6567 -20
Branches 714 726 +12
=============================================
+ Hits 1497 2316 +819
+ Misses 4955 4050 -905
- Partials 135 201 +66
Continue to review full report at Codecov.
|
|
Thanks! Maybe you can write a test to ensure that we don't make this mistake again? |
Will do! |
|
@marcingrzejszczak I added pretty comprehensive tests for the class in question. Not sure how useful all of them are given that the majority of it is just straightforward delegation, but I figure it's not hurting anything 🤷♂️ |
| BDDAssertions.then(executor.isShutdown()).isFalse(); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Can we separate given, when and then sections? I can't figure out which line is responsible for calling the actual code to test. I guess it's this one assertThat((Future<?>) executor.decorateTask(runnable, value)).isEqualTo(future); ?
There was a problem hiding this comment.
Yep, I'll go ahead and do that. In the example you cite, that statement is both the when and then. It's verifying that the return value for that method matches the delegate's return value. That's not super clear, so let me rework those a bit
There was a problem hiding this comment.
Reworked the tests a bit. Lemme know what you think
3 participants
I noticed while testing some functionality that one of the methods (
decorateTaskCallable) mapped via Reflection inLazyTraceScheduledThreadPoolExecutoris incorrect. I believe this should be decorateTask, as that is the method that exists in theScheduledThreadPoolExecutordelegate.Similarly,
newTaskForCallablealso appears to be mapped incorrectly. The only argument to this method is a Callable, yet the Reflection mapper is expecting an Object as well