wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.92k stars 1.3k forks source link

[Unit Tests] Remove mockito-core dependency #20532

Closed thomashorta closed 1 month ago

thomashorta commented 1 month ago

Issue

We are using both mockito-core and mockito-kotlin dependencies in our codebase, but the latter transitively depends on the former. We have a issue though that mockito-kotlin relies on a different mockito-core version than what's declared in the project (which was recently updated to 5.10.0 https://github.com/wordpress-mobile/WordPress-Android/pull/20246).

After that version bump mentioned above, gradle and Android Studio started complaining, on all @Mock annotations, that it "Cannot access class 'org.mockit.Answers'." due to missing and conflicting dependencies. Full error message:

image

Solution

Because of that conflict, we should be using only mockito-kotlin, which introduces the Kotlin-specific functionality while still transitively bringing in the mockito-core lib, so we can use it in Java as well.

This PR addresses that by removing mockito-core from our dependencies. There are 2 important points to be noted though, see below.

1. Mockito 5.X min Java version

Mockito 5.0.0 changed the minimum JVM target support to Java 11 so, in theory (and as long as I'm not missing anything) it shouldn't really work for us, since our jvmTarget is Java 8, including test runs.

Our project has been "using" mockito-core 5.X for some time now though without running into issues, so my guess is that since most of our tests run in Kotlin, we were actually using stuff from 4.X this whole time, since mockito-kotlin is on version 4.1.0.

I tried updating mockito-kotlin to the latest available version (5.2.1) and then I started getting compilation errors regarding inline bytecode using a different version (11) than our project (8), which matches the notes from Mockito 5.0.0, so I'm keeping 4.1.0 there, which is the latest mockito-kotlin 4 version available.

2. mockito-android

I kept mockito-android in our instrumented tests dependencies because otherwise those tests don't run properly. But to keep things consistent I downgraded it to 4.5.1, so it also depends on mockito-core:4.5.1, which is the version used by mockito-kotlin:4.1.0 as well.


To Test:

Make sure unit tests run successfully for WordPress Vanilla Release and instrumented tests still pass successfully for both Jetpack Vanilla Debug and Wordpress Vanilla Debug, which are the suites that run on our CI currentlly.


Regression Notes

  1. Potential unintended areas of impact

    • Tests breakage
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • Ran unit and instrumented tests and check CI passes
  3. What automated tests I added (or what prevented me from doing so)

    • Updated any tests that required updating to properly use mockito-kotlin
dangermattic commented 1 month ago
1 Warning
:warning: PR is not assigned to a milestone.

Generated by :no_entry_sign: Danger

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

wpmobilebot commented 1 month ago
Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20532-ae745ce
Commitae745ce5b7769d5234285f24b45929c85c2120fd
Direct Downloadjetpack-prototype-build-pr20532-ae745ce.apk
Note: Google Login is not supported on these builds.
wpmobilebot commented 1 month ago
WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20532-ae745ce
Commitae745ce5b7769d5234285f24b45929c85c2120fd
Direct Downloadwordpress-prototype-build-pr20532-ae745ce.apk
Note: Google Login is not supported on these builds.
wzieba commented 1 month ago

WDYT about increasing the target Java version to 11, or even 17 and using Mockito 5.x?

thomashorta commented 1 month ago

WDYT about increasing the target Java version to 11, or even 17 and using Mockito 5.x?

TBH I don't know if that could cause any issues and seems like a bigger change that can affect production code and not only test code.

It looks like we already have desugaring set up and we're using a version of the desugaring lib that supports Java 11 so maybe it would work, but based on the comments on pctCYC-1iC-p2 it might be worth it spending time actually replacing mockito-kotlin with MockK and only keeping Mockito around for legacy Java code.

Either way, I think it would be better to explore MockK or increasing our JVM target on a different PR, wdyt?

thomashorta commented 1 month ago

@wzieba per my comment above I will proceed with merging this PR and we can explore any of the options on separate PRs.