Rename tests to new format

[Rawa]

This PR migrates all non-android tests to the kotlin convention way of naming test methods

---

This change is 

[linear]

DROID-697 Rework test names

[albin-mullvad]

Reviewed 12 of 36 files at r1, all commit messages.
Reviewable status: 12 of 36 files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/PlayPaymentUseCaseTest.kt line 17 at r1 (raw file):

import org.junit.jupiter.api.Test

class PlayPaymentUseCaseTest {

There's a mix of test names starting with given and some starting with the the function name itself. Can that be unified? Also, in my experience "given" is usually used to reflect the starting state of the test rather than the action itself. If we don't see a need include a clear explanation of the starting state as part of the test name, I believe it's better to skip it and therefore changing names such as given invocation of myFunction ... to something like myFunction invocation ... or myFunction call .... "call" might be preferred since it helps shorten the test name.

For example, the following test:
given invocation of queryPaymentAvailability paymentAvailability should emit updated paymentAvailability

Can instead be written as:
queryPaymentAvailability call should result in updated paymentAvailablity
or
queryPaymentAvailability invocation should result in updated paymentAvailablity
or
queryPaymentAvailability invocation should result in paymentAvailablity emitting updated paymentAvailability

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/PlayPaymentUseCaseTest.kt line 59 at r1 (raw file):

    @Test
    fun `purchaseProduct should invoke purchaseProduct on repository`() = runTest {

nit: I suggest clarifying in the test name that this is a function call rather than some type of data structure.

I suggest the following:
purchaseProduct call should invoke purchaseProduct on repository
or
purchaseProduct invocation should invoke purchaseProduct on repository

[Pururun]

Reviewed 35 of 36 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Rawa)

---

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

    @Test
    fun `comparator should be able to name of only handle numbers`() {

Maybe better with "comparator should be able to handle name of only handle numbers"

---

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

    @Test
    fun `given successful createAccount side effect of NavigateToWelcome`() = runTest {

Is probably more readable like this:
"given successful **.account creation ** should result in side effect of NavigateToWelcome"

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/SplitTunnelingViewModelTest.kt line 89 at r1 (raw file):

    @Test
    fun `given includedApps and excludedApps returns sets uiState should include the lists`() =

Maybe change to "should include both"?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModelTest.kt line 118 at r1 (raw file):

    @Test
    fun `given SettingsRepository emits Constraint Only 99 uiState should custom and selectedWireguardPort 99`() =

Should we have numbers in tests? Maybe be a bit more generic, such as "emits Port" and "selectedWireguardPort with the same value" ?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModelTest.kt line 142 at r1 (raw file):

    @Test
    fun `onWireguardPortSelected with 99 should invoke updateSelectedWireguardConstraint with Constraint Only 99`() =

See above

[albin-mullvad]

Reviewed 5 of 36 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Rawa)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/PlayPaymentUseCaseTest.kt line 17 at r1 (raw file):

Previously, albin-mullvad wrote…

There's a mix of test names starting with given and some starting with the the function name itself. Can that be unified? Also, in my experience "given" is usually used to reflect the starting state of the test rather than the action itself. If we don't see a need include a clear explanation of the starting state as part of the test name, I believe it's better to skip it and therefore changing names such as given invocation of myFunction ... to something like myFunction invocation ... or myFunction call .... "call" might be preferred since it helps shorten the test name.

For example, the following test:
given invocation of queryPaymentAvailability paymentAvailability should emit updated paymentAvailability

Can instead be written as:
queryPaymentAvailability call should result in updated paymentAvailablity
or
queryPaymentAvailability invocation should result in updated paymentAvailablity
or
queryPaymentAvailability invocation should result in paymentAvailablity emitting updated paymentAvailability

To continue on this one, adding "call" or "invocation" is probably only necessary in cases where it's not clear that the initial "word"/"phrase" is a function name, so the above can probably be similified to: queryPaymentAvailability should result in updated paymentAvailablity

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/PaymentViewModelTest.kt line 60 at r1 (raw file):

    @Test
    fun `given purchaseResult emits Success uiState should return success dialog data`() = runTest {

I believe this test can be clarified/simplified to something like: purchaseResult emitting Success should result in success dialog state

[Rawa]

Reviewable status: 18 of 39 files reviewed, 7 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe better with "comparator should be able to handle name of only handle numbers"

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/PlayPaymentUseCaseTest.kt line 17 at r1 (raw file):

Previously, albin-mullvad wrote…

To continue on this one, adding "call" or "invocation" is probably only necessary in cases where it's not clear that the initial "word"/"phrase" is a function name, so the above can probably be similified to: queryPaymentAvailability should result in updated paymentAvailablity

We'd a internal discussion offline, I've now updated most of the test to include this simplification.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/PlayPaymentUseCaseTest.kt line 59 at r1 (raw file):

Previously, albin-mullvad wrote…

nit: I suggest clarifying in the test name that this is a function call rather than some type of data structure.

I suggest the following:
purchaseProduct call should invoke purchaseProduct on repository
or
purchaseProduct invocation should invoke purchaseProduct on repository

Fixed

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Is probably more readable like this:
"given successful **.account creation ** should result in side effect of NavigateToWelcome"

Simplified it a bit, I believe it is more clear now :)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/PaymentViewModelTest.kt line 60 at r1 (raw file):

Previously, albin-mullvad wrote…

I believe this test can be clarified/simplified to something like: purchaseResult emitting Success should result in success dialog state

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/SplitTunnelingViewModelTest.kt line 89 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe change to "should include both"?

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModelTest.kt line 118 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should we have numbers in tests? Maybe be a bit more generic, such as "emits Port" and "selectedWireguardPort with the same value" ?

I think you are correct, I've removed it

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModelTest.kt line 142 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

See above

Done.

[Pururun]

Reviewed 21 of 21 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 15 at r2 (raw file):

import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton
import androidx.compose.material3.ListItem

Is this needed?

---

android/test/arch/src/test/kotlin/net/mullvad/mullvadvpn/test/arch/JUnitTest.kt line 58 at r2 (raw file):

    companion object {
        private val ignoredTestPackages =

Maybe there should be some kind of explanation why this is ignored?

[Rawa]

Reviewable status: 37 of 39 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

android/test/arch/src/test/kotlin/net/mullvad/mullvadvpn/test/arch/JUnitTest.kt line 58 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe there should be some kind of explanation why this is ignored?

Good idea, I've clarified it

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 15 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is this needed?

Woops, fixed!

[albin-mullvad]

Reviewed 2 of 36 files at r1, 15 of 21 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/PlayPaymentUseCaseTest.kt line 79 at r3 (raw file):

    @Test
    fun `on resetPurchaseResult call purchaseResult should emit null`() = runTest {

nit: is there a reason for this function to start with on ... whereas the other functions doensn't?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/TunnelStateNotificationUseCaseTest.kt line 64 at r3 (raw file):

    @Test
    fun `given TunnelState is error use case should emit TunnelStateError notification`() =

given -> when ?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModelTest.kt line 64 at r3 (raw file):

    @Test
    fun `uiState should return UNKNOWN when service connection state is Disconnected`() = runTest {

This naming is a bit different from many other vm tests. I'm not saying one is better than the other, but would be good to keep it consistent imo.

For example this one could be written as:
when service connection state is Disconnected then uiState should emit UNKNOWN

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModelTest.kt line 64 at r3 (raw file):

    @Test
    fun `uiState should return UNKNOWN when service connection state is Disconnected`() = runTest {

nit: return -> emit?

---

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

    @Test
    fun `login call should result in uiState with NoInternetConnection when no internet`() =

In this one the when no internet part is in the arrange section and a prerequisite for the test, so I believe it's a typical thing to include in the beginning of the test name as a "given" thing.

For example theses test names would be more readable and better match the test imo:

given no internet when login is attempted then uiState should emit NoInternetConnection
or
given no internet when login is called then uiState should emit NoInternetConnection

---

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

    @Test
    fun `given login call returns Ok uiSideEffect should emit NavigateToConnect`() = runTest {

Is this referring to the vm or repo? 🤔 Might be good to clarify that somehow. Same applies to other tests in this class.

Code quote:

login call

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModelTest.kt line 113 at r3 (raw file):

    @Test
    fun `on onSitePaymentClick call uiSideEffect should emit OpenAccountView`() = runTest {

nit: here the function names starts with on onSitePaymentClick ... whereas further down it's just onDisconnectClick .... Would be nice to keep it consistent.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemViewModelTest.kt line 48 at r3 (raw file):

    @Test
    fun `when sendReport returns CollectLog error uiState should emit sendingState with CollectLog error`() =

Tbh I find it a bit hard to read something like below without something separating the sections of the statement (between error and uiState):

... returns CollectLog error uiState should ...

Two suggestions on how the readability can be a bit improved:

... returns CollectLog error, uiState should ...
or
... returns CollectLog error then uiState should ...

This comment also applies to more functions than this specific one.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModelTest.kt line 81 at r3 (raw file):

    @Test
    fun `uiState should by default emit Loading state`() = runTest {

Other similar tests end with ... by default. For example: uiState should emit Loading state by default

Code quote:

uiState should by default emit Loading state

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 31 at r3 (raw file):

@ExtendWith(TestCoroutineRule::class)
class VoucherDialogViewModelTest {

nit: the tests in this class are a bit inconsistent:
onRedeem should
on onRedeem call with
on onRedeem with

I've seen this in a some other places as well, so would be probably be a good idea to go through and make sure its consistent throughout this pr

[Pururun]

Reviewed all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Rawa)

[Rawa]

Reviewable status: 24 of 39 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, albin-mullvad wrote…

In this one the when no internet part is in the arrange section and a prerequisite for the test, so I believe it's a typical thing to include in the beginning of the test name as a "given" thing.

For example theses test names would be more readable and better match the test imo:

given no internet when login is attempted then uiState should emit NoInternetConnection
or
given no internet when login is called then uiState should emit NoInternetConnection

Done.

---

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

Previously, albin-mullvad wrote…

Is this referring to the vm or repo? 🤔 Might be good to clarify that somehow. Same applies to other tests in this class.

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemViewModelTest.kt line 48 at r3 (raw file):

Previously, albin-mullvad wrote…

Tbh I find it a bit hard to read something like below without something separating the sections of the statement (between error and uiState):

... returns CollectLog error uiState should ...

Two suggestions on how the readability can be a bit improved:

... returns CollectLog error, uiState should ...
or
... returns CollectLog error then uiState should ...

This comment also applies to more functions than this specific one.

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModelTest.kt line 81 at r3 (raw file):

Previously, albin-mullvad wrote…

Other similar tests end with ... by default. For example: uiState should emit Loading state by default

Done.

[albin-mullvad]

Reviewed 2 of 21 files at r2, 15 of 15 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[albin-mullvad]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved