Skip to content
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

Migrate tests to JUnit5 #5601

Merged
merged 8 commits into from
Jan 11, 2024
Merged

Migrate tests to JUnit5 #5601

merged 8 commits into from
Jan 11, 2024

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Dec 19, 2023


This change is Reviewable

Copy link

linear bot commented Dec 19, 2023

@Pururun Pururun added the Android Issues related to Android label Dec 19, 2023
@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch 4 times, most recently from 0adca9c to a882da3 Compare December 22, 2023 12:46
@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch 3 times, most recently from 0ff341e to 11e8570 Compare January 8, 2024 08:40
@Pururun Pururun marked this pull request as ready for review January 8, 2024 08:40
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 16 of 21 files at r1, 15 of 67 files at r2, 62 of 67 files at r4.
Reviewable status: 78 of 83 files reviewed, 6 unresolved discussions (waiting on @Pururun)


android/app/src/test/kotlin/net/mullvad/mullvadvpn/relaylist/RelayNameComparatorTest.kt line 38 at r4 (raw file):

            Relay(name = "se9-wireguard", location = mockk(), locationName = "mock", active = false)

        Assertions.assertTrue(RelayNameComparator.compare(relay9a, relay9b) == 0)

nit: would be nice to skip. applies to the rest of this file as well.

Code quote:

Assertions.

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 82 at r4 (raw file):

            // Assert
            Assertions.assertEquals(

nit: skip?

Code quote:

Assertions.

android/test/arch/src/test/resources/junit-platform.properties line 4 at r2 (raw file):

junit.jupiter.execution.parallel.mode.default=concurrent
junit.jupiter.execution.parallel.config.strategy=dynamic
junit.jupiter.execution.parallel.config.dynamic.factor=0.95

What's the default values and is there a reason we use these specific values? Specifically the dynamic factor 0.95. A link or comment would be nice.

Code quote:

junit.jupiter.execution.parallel.enabled=true
junit.jupiter.execution.parallel.mode.default=concurrent
junit.jupiter.execution.parallel.config.strategy=dynamic
junit.jupiter.execution.parallel.config.dynamic.factor=0.95

android/test/e2e/build.gradle.kts line 105 at r4 (raw file):

    implementation(Dependencies.junitApi)
    implementation(Dependencies.junitAndroidTestCore)
    implementation(Dependencies.junitAndroidTestRunner)

Move these above implementation(Dependencies.Kotlin.stdlib) to keep the order alphabetical

Code quote:

    implementation(Dependencies.junitAndroidTestExtensions)
    implementation(Dependencies.junitApi)
    implementation(Dependencies.junitAndroidTestCore)
    implementation(Dependencies.junitAndroidTestRunner)

android/test/mockapi/build.gradle.kts line 81 at r4 (raw file):

    implementation(Dependencies.junitApi)
    implementation(Dependencies.junitAndroidTestCore)
    implementation(Dependencies.junitAndroidTestRunner)

Move these to keep the order alphabetical

Code quote:

    implementation(Dependencies.junitAndroidTestExtensions)
    implementation(Dependencies.junitApi)
    implementation(Dependencies.junitAndroidTestCore)
    implementation(Dependencies.junitAndroidTestRunner)

android/test/mockapi/src/main/kotlin/net/mullvad/mullvadvpn/test/mockapi/LogoutMockApiTest.kt line 35 at r4 (raw file):

        // Assert
        Assertions.assertNotNull(device.findObjectWithTimeout(By.text("Login")))

nit: would be nice to skip this

Code quote:

Assertions.

@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch from 11e8570 to e9110c8 Compare January 8, 2024 11:05
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 78 of 83 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)


android/app/src/test/kotlin/net/mullvad/mullvadvpn/relaylist/RelayNameComparatorTest.kt line 38 at r4 (raw file):

Previously, albin-mullvad wrote…

nit: would be nice to skip. applies to the rest of this file as well.

Done


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 82 at r4 (raw file):

Previously, albin-mullvad wrote…

nit: skip?

Done


android/test/arch/src/test/resources/junit-platform.properties line 4 at r2 (raw file):

Previously, albin-mullvad wrote…

What's the default values and is there a reason we use these specific values? Specifically the dynamic factor 0.95. A link or comment would be nice.

It was recommended by konsist, I will add a link.


android/test/e2e/build.gradle.kts line 105 at r4 (raw file):

Previously, albin-mullvad wrote…

Move these above implementation(Dependencies.Kotlin.stdlib) to keep the order alphabetical

Done


android/test/mockapi/build.gradle.kts line 81 at r4 (raw file):

Previously, albin-mullvad wrote…

Move these to keep the order alphabetical

Done


android/test/mockapi/src/main/kotlin/net/mullvad/mullvadvpn/test/mockapi/LogoutMockApiTest.kt line 35 at r4 (raw file):

Previously, albin-mullvad wrote…

nit: would be nice to skip this

Done

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 70 of 70 files at r6, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Pururun)


-- commits line 1 at r6:
Is the goal to merge these commits as-is or to cleanup/squash something?


-- commits line 11 at r6:
Can this be clarified? Which tests?

Code quote:

Migrate more unit tests to junit 5

-- commits line 13 at r6:
I assume this is referring to mockapi and e2e? In that case it should be clarified by the commit message

Code quote:

tests outside of app

-- commits line 23 at r6:
"again"?

Code quote:

again

android/buildSrc/src/main/kotlin/Dependencies.kt line 68 at r6 (raw file):

        const val uiToolingAndroidPreview =
            "androidx.compose.ui:ui-tooling-preview-android:${Versions.Compose.base}"
        const val uiTestManifest =

nit: order alphabetically


android/buildSrc/src/main/kotlin/Dependencies.kt line 137 at r6 (raw file):

        const val ktfmtId = "com.ncorti.ktfmt.gradle"
        const val ksp = "com.google.devtools.ksp"
        const val junit5 = "de.mannodermaus.android-junit5"

nit: order alphabetically


android/buildSrc/src/main/kotlin/Versions.kt line 21 at r6 (raw file):

        const val targetSdkVersion = 34
        const val volley = "1.2.1"
        const val junit = "1.4.0"

nit: order alphabetically


android/buildSrc/src/main/kotlin/Versions.kt line 60 at r6 (raw file):

        // https://github.com/google/ksp/releases
        const val ksp = "${kotlin}-1.0.14"
        const val junit5 = "1.10.0.0"

nit: order alphabetically


android/lib/billing/build.gradle.kts line 4 at r6 (raw file):

    id(Dependencies.Plugin.androidLibraryId)
    id(Dependencies.Plugin.kotlinAndroidId)
    id(Dependencies.Plugin.junit5) version Versions.Plugin.junit5

Move up to keep alphabetical sorting

Code quote:

id(Dependencies.Plugin.junit5) version Versions.Plugin.junit5

android/lib/billing/build.gradle.kts line 64 at r6 (raw file):

    testImplementation(Dependencies.MockK.core)
    testImplementation(Dependencies.junitApi)
    testRuntimeOnly(Dependencies.junitEngine)

nit: I suggest moving this one above the testImplementation calls

Code quote:

testRuntimeOnly(Dependencies.junitEngine)

@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch from e9110c8 to 582c1d3 Compare January 9, 2024 10:00
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @albin-mullvad)


-- commits line 1 at r6:

Previously, albin-mullvad wrote…

Is the goal to merge these commits as-is or to cleanup/squash something?

Yeah goal is to cleanup


-- commits line 11 at r6:

Previously, albin-mullvad wrote…

Can this be clarified? Which tests?

See above, goal is to cleanup commits when everything else have been solved.


-- commits line 13 at r6:

Previously, albin-mullvad wrote…

I assume this is referring to mockapi and e2e? In that case it should be clarified by the commit message

See above, goal is to cleanup commits when everything else have been solved.


-- commits line 23 at r6:

Previously, albin-mullvad wrote…

"again"?

See above, goal is to cleanup commits when everything else have been solved.


android/buildSrc/src/main/kotlin/Dependencies.kt line 68 at r6 (raw file):

Previously, albin-mullvad wrote…

nit: order alphabetically

Done


android/buildSrc/src/main/kotlin/Dependencies.kt line 137 at r6 (raw file):

Previously, albin-mullvad wrote…

nit: order alphabetically

Done


android/buildSrc/src/main/kotlin/Versions.kt line 21 at r6 (raw file):

Previously, albin-mullvad wrote…

nit: order alphabetically

Done


android/buildSrc/src/main/kotlin/Versions.kt line 60 at r6 (raw file):

Previously, albin-mullvad wrote…

nit: order alphabetically

Done


android/lib/billing/build.gradle.kts line 4 at r6 (raw file):

Previously, albin-mullvad wrote…

Move up to keep alphabetical sorting

Done


android/lib/billing/build.gradle.kts line 64 at r6 (raw file):

Previously, albin-mullvad wrote…

nit: I suggest moving this one above the testImplementation calls

Done


android/test/arch/src/test/resources/junit-platform.properties line 4 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It was recommended by konsist, I will add a link.

https://docs.konsist.lemonappdev.com/advanced/additional-junit5-setup
I added it as a comment in the file as well.

@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch 2 times, most recently from 46b3b28 to 045f776 Compare January 9, 2024 10:29
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 70 files at r8, 67 of 67 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions


android/test/arch/src/test/resources/junit-platform.properties line 4 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

https://docs.konsist.lemonappdev.com/advanced/additional-junit5-setup
I added it as a comment in the file as well.

Nice! 👍

@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch 2 times, most recently from 84dabf2 to 3ee4259 Compare January 10, 2024 14:17
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 31 of 31 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the update-to-junit5-droid-569 branch from 3ee4259 to b416746 Compare January 11, 2024 08:52
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 31 of 31 files at r14, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun merged commit 6c754c3 into main Jan 11, 2024
22 checks passed
@Pururun Pururun deleted the update-to-junit5-droid-569 branch January 11, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants