-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve test coverage of core unit tests #2294
Conversation
07b8460
to
810e356
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2294 +/- ##
===========================================
+ Coverage 69.97% 70.26% +0.29%
===========================================
Files 733 730 -3
Lines 27236 27259 +23
Branches 4576 4583 +7
===========================================
+ Hits 19058 19152 +94
+ Misses 6894 6832 -62
+ Partials 1284 1275 -9
|
return false | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
such a tiny typo, but such a big functional change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly
gradle/libs.versions.toml
Outdated
@@ -7,7 +7,7 @@ okHttp = "4.12.0" | |||
kronosNTP = "0.0.1-alpha11" | |||
|
|||
# Android | |||
androidToolsPlugin = "8.6.1" | |||
androidToolsPlugin = "8.5.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we doing downgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this shouldn't have been part of the commit, it's because I use IJ instead of AS ans IJ doesn't support the latest AGP.
@@ -23,7 +24,7 @@ import org.mockito.junit.jupiter.MockitoExtension | |||
@ForgeConfiguration(Configurator::class) | |||
internal class ThreadExtTest { | |||
|
|||
@Test | |||
@RepeatedTest(16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change on purpose or some debugging leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on purpose to go over all the possible states
// Given | ||
val fakeDelay = TimeUnit.SECONDS.toNanos(forge.aLong(min = 0, max = 2)) | ||
val mockedBlock: () -> Boolean = mock() | ||
whenever(mockedBlock.invoke()).thenAnswer { throw exception }.thenReturn(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably can be shorter a bit
whenever(mockedBlock.invoke()).thenAnswer { throw exception }.thenReturn(true) | |
whenever(mockedBlock.invoke()).thenThrow(exception).thenReturn(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed 👍
810e356
to
c1f2e34
Compare
What does this PR do?
Add missing UT on some core module classes, and improve some UT for maintainability