Chore/migrate to kt #1394
Chore/migrate to kt #1394
Conversation
atulgpt
force-pushed
the
chore/migrate-to-kt
branch
from
fffa501 to
43e1e80
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1394 +/- ##
==========================================
+ Coverage 63.16% 63.56% +0.39%
==========================================
Files 158 158
Lines 3125 3134 +9
Branches 324 326 +2
==========================================
+ Hits 1974 1992 +18
+ Misses 1056 1045 -11
- Partials 95 97 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| activityListenerCaptor.capture(), | ||
| ArgumentMatchers.eq(frameMetricsHandler), | ||
| ) | ||
| Assert.assertEquals( |
There was a problem hiding this comment.
We prefer the assertj style assertions, can you modify this to use assertj please?
| import org.mockito.Answers | ||
| import org.mockito.ArgumentCaptor | ||
| import org.mockito.ArgumentMatchers | ||
| import org.mockito.Captor | ||
| import org.mockito.Mock | ||
| import org.mockito.Mockito | ||
| import org.mockito.junit.MockitoJUnit | ||
| import org.mockito.junit.MockitoRule | ||
| import org.mockito.stubbing.Answer |
There was a problem hiding this comment.
The PR and the issue you referenced in the PR description both make mention of our desire to move away from mockito in favor of mockk. Would it be possible for you to do that conversion as part of this PR as well? Thanks!
| Assert.assertEquals( | ||
| span.spanContext.traceId, | ||
| currentSpan.traceId, |
There was a problem hiding this comment.
It was already structured this way, so you're not making it worse or anything, but any time I see assertions inside of callback/async/conditional code I'm leery, because there can be unexpected cases where the callback (in this case an interceptor) doesn't get invoked at all and the test still passes without the assertion being invoked.
If we're going to keep that around, then I think there also needs to be a side effect that we can also assert on to ensure that the assertion was invoked (I've frequently used countdown latches for such a thing).
| currentSpan.traceId, | ||
| ) | ||
| chain!!.proceed(chain.request()) | ||
| Assertions |
There was a problem hiding this comment.
Let's use the static import for these, because it reads nicer.
| private fun makeSomeDurations(): List<Long> = | ||
| Stream | ||
| .of( | ||
| 5L, | ||
| 11L, | ||
| 101L, // slow | ||
| 701L, // frozen | ||
| 17L, // slow | ||
| 17L, // slow | ||
| 16L, | ||
| 11L, | ||
| ).map { duration -> | ||
| TimeUnit.MILLISECONDS.toNanos( | ||
| duration, | ||
| ) | ||
| }.collect(Collectors.toList()) |
There was a problem hiding this comment.
Now that we're in kotlin, using Stream for this should be unnecessary. You can do something more like
listOf(
5L, 11L, 101L, // slow
701L, // frozen
17L, // slow
17L, // slow
16L, 11L)
.map { it.milliseconds.inWholeNanoseconds }2 participants
Migrating one TC file to kt so that mockito can be removed
This was mentioned in #1362 (comment)
This will unblock #1247