Add unit tests in some push classes #2899

bmarty · 2024-05-22T22:32:03Z

Add unit tests in some push classes

I had to introduce some interface to be able to properly unit test more classes.

Code coverage is doing a nice bump:

This does not fix any bug, except one related to log, so very minor.

We may properly add it again later if necessary.

…tPusherSubscriber

… it's acting as a key.

codecov · 2024-05-22T23:01:06Z

Codecov Report

Attention: Patch coverage is 82.78689% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 75.26%. Comparing base (7bda5fd) to head (788d9e0).
Report is 7 commits behind head on develop.

Files	Patch %	Lines
...nstorage/impl/memory/InMemoryMultiSessionsStore.kt	40.00%	6 Missing ⚠️
...ies/push/impl/pushgateway/PushGatewayApiFactory.kt	0.00%	4 Missing ⚠️
...pushproviders/unifiedpush/UnifiedPushApiFactory.kt	0.00%	4 Missing ⚠️
...libraries/push/impl/push/IncrementPushDataStore.kt	0.00%	3 Missing ⚠️
...raries/push/impl/push/OnNotifiableEventReceived.kt	0.00%	3 Missing ⚠️
...ries/pushproviders/unifiedpush/UnifiedPushStore.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2899      +/-   ##
===========================================
+ Coverage    74.21%   75.26%   +1.04%     
===========================================
  Files         1540     1550      +10     
  Lines        36892    36917      +25     
  Branches      7149     7148       -1     
===========================================
+ Hits         27381    27784     +403     
+ Misses        5779     5398     -381     
- Partials      3732     3735       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-23T01:24:30Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/wbYzzA

…ambda is invoked and it should not.

sonarqubecloud · 2024-05-23T08:34:41Z

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

jmartinesp

Thanks for the work on this and the big coverage increase! I added lots of small comments (especially about docs), but none of them are really blockers, so feel free to merge it as is.

jmartinesp · 2024-05-23T08:51:56Z

...t/src/main/kotlin/io/element/android/libraries/matrix/test/auth/FakeAuthenticationService.kt

@@ -48,6 +50,9 @@ class FakeAuthenticationService : MatrixAuthenticationService {
    override suspend fun getLatestSessionId(): SessionId? = getLatestSessionIdLambda()

    override suspend fun restoreSession(sessionId: SessionId): Result<MatrixClient> {
+        if (matrixClientResult != null) {


Ideally it would be nice to use matrixClientResult here to the get MatrixClient instead of having 2 sources of truth, but changing it may not be worth it.

jmartinesp · 2024-05-23T08:59:25Z

...otlin/io/element/android/libraries/push/impl/notifications/DefaultNotifiableEventResolver.kt

+}
+
+@ContributesBinding(AppScope::class)
+class DefaultNotifiableEventResolver @Inject constructor(


Maybe move this to a separate file in the api module?

jmartinesp · 2024-05-23T09:05:11Z

...mpl/src/main/kotlin/io/element/android/libraries/push/impl/push/OnNotifiableEventReceived.kt

+}
+
+@ContributesBinding(AppScope::class)
+class DefaultOnNotifiableEventReceived @Inject constructor(


We could move this to its own file, maybe.

jmartinesp · 2024-05-23T09:05:16Z

...mpl/src/main/kotlin/io/element/android/libraries/push/impl/push/OnNotifiableEventReceived.kt

+import io.element.android.libraries.push.impl.notifications.model.NotifiableEvent
+import javax.inject.Inject
+
+interface OnNotifiableEventReceived {


jmartinesp · 2024-05-23T09:05:24Z

...h/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/IncrementPushDataStore.kt

+import io.element.android.libraries.push.impl.store.DefaultPushDataStore
+import javax.inject.Inject
+
+interface IncrementPushDataStore {


jmartinesp · 2024-05-23T09:12:41Z

.../kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushGatewayResolver.kt

+            val port = if (url.port != -1) ":${url.port}" else ""
+            val customBase = "${url.protocol}://${url.host}$port"
+            val customUrl = "$customBase/_matrix/push/v1/notify"
+            Timber.i("Testing $customUrl")


Should we re-throw CancellationExceptions?

jmartinesp · 2024-05-23T09:12:57Z

...otlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushNewGatewayHandler.kt


 /**
 * Handle new endpoint received from UnifiedPush. Will update the session matching the client secret.
 */
-class UnifiedPushNewGatewayHandler @Inject constructor(
+interface UnifiedPushNewGatewayHandler {


Split this into 2 files?

jmartinesp · 2024-05-23T09:13:09Z

...h/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushStore.kt

 import io.element.android.libraries.di.ApplicationContext
 import io.element.android.libraries.di.DefaultPreferences
 import io.element.android.libraries.matrix.api.core.UserId
 import javax.inject.Inject

-class UnifiedPushStore @Inject constructor(
+interface UnifiedPushStore {


Docs + splitting?

jmartinesp · 2024-05-23T09:13:21Z

...otlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt

+    suspend fun execute(matrixClient: MatrixClient, clientSecret: String): Result<Unit>
+}
+
+@ContributesBinding(AppScope::class)


Docs + splitting?

jmartinesp · 2024-05-23T09:16:24Z

...pl/src/main/kotlin/io/element/android/libraries/textcomposer/mentions/MentionSpanProvider.kt

@@ -164,7 +164,7 @@ internal fun MentionSpanPreview() {
                        eventId = null,
                        viaParameters = persistentListOf(),
                    )
-                    else -> TODO()
+                    else -> throw AssertionError("Unexpected value $uriString")


bmarty · 2024-05-23T12:35:05Z

Thanks for the review. About splitting interface and default implementation, I think it does not worth it as the interface in only to ease the unit tests. so it's actually fine to me if the interface (which does not need to be expose in a api module) and the default impl is in the same file.
Let's merge the PR like this since as you said there is no blocker.
Thanks again for the review!

bmarty and others added 30 commits May 22, 2024 09:23

Add test on UnifiedPushGatewayResolver

50004e3

UnifiedPushGatewayResolver.getGateway cannot return null.

86eceb3

Add test on UnifiedPushProvider

1bf38e9

Create FakeIsPlayServiceAvailable

538c2b0

Add test for FirebasePushProvider

0087972

Remove unused code notificationStyleChanged()

b70c591

We may properly add it again later if necessary.

Extract testPush to its own class and rename PushersManager to Defaul…

20880b0

…tPusherSubscriber

Add test on PushGatewayNotifyRequest

dc6e62a

Add test on DefaultPushService

505f6d4

Move some classes to the test module.

08f70b9

Fix wrong package name.

707a530

Add test on DefaultPusherSubscriber

97530e7

Add test on DefaultTestPush

3bde744

isCalledExactly(1) -> isCalledOnce()

fe771a3

Cleanup

980a80b

Create interface for NotifiableEventResolver

5b074dc

Add test on DefaultPushHandler

b2a3b96

Add test on VectorFirebaseMessagingService

eafa713

Change to lambda

90a14ce

Add test on DefaultFirebaseNewTokenHandler

b38c144

Fix test on VectorFirebaseMessagingService

5c68956

Create interface UnifiedPushGatewayResolver

738bb83

Create interface UnifiedPushNewGatewayHandler

09b2b2c

Add test on VectorUnifiedPushMessagingReceiver

50b7f2c

Add test on DefaultRegisterUnifiedPushUseCase

f304c1f

Add test on DefaultUnifiedPushNewGatewayHandler

f974cdd

Add test on UnifiedPushTest.isRelevant

02fa405

Add test on DefaultUnregisterUnifiedPushUseCase

0a2d87f

Refactor: change signature to always have clientSecret first, since…

4e7f5c4

… it's acting as a key.

Add test on isRelevant

cf3b2bb

Clean up

c71c491

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 23, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 23, 2024

Introduce lambdaError instead of using TODO, to handle error when a l…

f042532

…ambda is invoked and it should not.

bmarty marked this pull request as ready for review May 23, 2024 07:16

bmarty requested a review from a team as a code owner May 23, 2024 07:16

bmarty requested review from jmartinesp and removed request for a team May 23, 2024 07:16

bmarty and others added 3 commits May 23, 2024 09:33

Use lambdaError() instead of throw NotImplementedError()

de032fa

Add missing import

b8b66b3

Simplify RegisterUnifiedPushUseCase, fix test

0639bc6

jmartinesp force-pushed the feature/bma/unitTestPush branch from 174dea5 to 0639bc6 Compare May 23, 2024 07:57

bmarty added 2 commits May 23, 2024 10:19

No need to use advanceTimeBy

b716482

No need to use StandardTestDispatcher

788d9e0

jmartinesp approved these changes May 23, 2024

View reviewed changes

bmarty merged commit 22bde9e into develop May 23, 2024
21 checks passed

bmarty deleted the feature/bma/unitTestPush branch May 23, 2024 12:35

Add unit tests in some push classes #2899

Add unit tests in some push classes #2899

Uh oh!

Conversation

bmarty commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented May 23, 2024

Quality Gate passed

Uh oh!

jmartinesp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty commented May 23, 2024

Uh oh!

Uh oh!

Uh oh!

bmarty commented May 22, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading