Adding unit tests for live location sharing aggregation code (PSF-1063)

[mnaturel]

Type of change

• Feature
• Bugfix
• Technical
• Other : unit tests

Content

Adding unit tests to cover code of aggregation process of live location share events.

Motivation and context

Closes #6155

Screenshots / GIFs

Tests

• Run unit tests

Tested devices

• Physical
• Emulator
• OS version(s):

Checklist

• Changes has been tested on an Android device or Android emulator with API 21
• UI change has been tested on both light and dark themes
• Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
• Pull request is based on the develop branch
• Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
• Pull request includes screenshots or videos if containing UI changes
• Pull request includes a sign off
• You've made a self review of your PR
• If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

[ouchadam]

as we're actioning different inputs no related event id and empty related event id, this feels like two separate tests (similar comment for the test below)

a parameterised test could potentially cover all these validation cases (orrrrr the naive parameterised tests I've started doing like this

---

for the parameters themselves, are null/empty ids possible? would be nice if we could catch them further up the chain and avoid invalid empty ids

[mnaturel]

I agree with your point. I created a loop on a list of ignored values to group the similar tests: see 912244a. I didn't use parametrized tests since I don't like the boilerplate syntax in Junit 4. I am wondering if it is possible to switch to Junit 5 in Android: the syntax for parametrized tests is much nicer.

For you last point, I agree but I didn't change anything since it would require to change the events aggregation process in a more general way.

[ouchadam]

val previousEntities = mockPreviousLiveLocationShareAggregatedSummaryEntities()

previousEntities.forEach { entity ->
  entity.isActive shouldBeEqualTo false
}

it looks like the previousEntities aren't used?

[mnaturel]

In fact, they are. They are modified internally by the aggregation process. And here we check the result is correct. I could not find a better way to ensure the aggregation is correct. I am open to any suggestion that could improve that.

[ouchadam]

it could be helpful to name these helpers with the data they're fetching

givenLastRoomBeaconQueryReturns(roomId, eventId, beaconLocation)

val activeSummary = LiveLocationShareAggregatedSummaryEntity(
  eventId = "ignored-value",
  roomId = A_ROOM_ID,
  userId = A_SENDER_ID,
  isActive = true
)
givenLocationSummaryQueryReturns(activeSummary)

[mnaturel]

I clarified the namings here: ed13244

[ouchadam]

💯 for updating the fake!

[ouchadam]

I'm wondering if we should be explicitly expecting the tests to mock enqueueUniqueWork cancelUniqueWork rather than applying it by default, we can make the test cases more resilient by also verifying against the arguments

class FakeWorkManagerProvider(val fakeWorkManager = FakeWorkManager()) {
    val instance = mockk<WorkManagerProvider>().also {
        every { workManager } returns fakeWorkManager.instance
    }
}

class FakeWorkManager {
    val instance = mockk<WorkManager>()

    fun expectCancelUniqueWork() {
        every { instance.cancelUniqueWork(any()) } returns mockk()
    }

   fun verifyCancelUniqueWork(name: String) {
     verify { instance.cancelUniqueWork(name) }
   }

}

[mnaturel]

Yes you are right. I will make an update.

[mnaturel]

Updated here: 454209a

[ouchadam]

💯 thanks for the updates!

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication