Poll view state unit tests [PSF-1130] #6366

onurays · 2022-06-22T13:44:39Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Determining PollState and PollOptionViewState require business login. This PR aims to clean up MessageItemFactory, refactor this by creating a factory class and increase the test coverage.

Motivation and context

Screenshots / GIFs

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 · 2022-06-22T15:25:26Z

...or/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactory.kt

+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal fun List<PollAnswer>.mapToOptions(


rather than exposing the internals for testing I would take a step back and consider what we can and can't easily unit test, in this case it's the AndroidViews/Epoxy Items

which means instead of returning an epoxy PollItem, we could create a dedicated proxy ViewState data class for the PolItem

// MessageItemFactory createPollItem() { val viewState = pollItemViewStateFactory.create(params) return PollItem_() .callback() .highlight() .state(viewState) }

If Views have no/minimal business logic, then it means our ViewStates could be considered UI proxies (which means less need for slow/flaky instrumentation tests)

whichhhh would turn our class api into...

val viewState = PollItemViewStateFactory().create( pollContent: MessagePollContent, informationData: MessageInformationData, )

as the factory returns immutable data we can make simple asserts against the result

@Test fun `given a certain input, when creating state, then has specific result`() { val specific: MessagePollContent val result = factory.create(specificInput) result shouldBeEqualTo expectedOutput }

Very nice idea, done!

ouchadam · 2022-06-22T15:31:10Z

...rc/test/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactoryTest.kt

+
+    @Test
+    fun `given a sent poll then all option view states is PollReady`() = runTest {
+        with(pollItemFactory) {


For the test bodies if we follow Gherkin conventions (with blank lines between the sections) we can make the tests more consistent and quicker to understand

// given - the parameters or setup required to create the test result val payload = Payload(argumentThatCreatesResult) // when - the single action under test val result = myClassUnderTest.theFunctionUnderTest(payload) // then - assert or verify side effects result shouldBeEqualTo ExpectedResult() verify(mySideEffect).wasCalled()

Done (hopefully).

ouchadam · 2022-06-22T15:40:23Z

...rc/test/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactoryTest.kt

+    fun setup() {
+        // We are not going to test any UI related code
+        pollItemFactory = PollItemFactory(
+                stringProvider = mockk(),


these should be test fakes rather than direct mocks and as they're unused by the tests it also suggests we aren't asserting any of the data they provide

for example we have a FakeStringProvider which allows us to assert against the correct resource ids

val fake = FakeStringProvider() val result = fake.getString(R.string.ok) result shouldBeEqualTo R.string.ok.toTestString()

ouchadam · 2022-06-22T15:45:27Z

...rc/test/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactoryTest.kt

+
+    @After
+    fun tearDown() {
+        unmockkAll()


unmocking is only needed if we're statically mocking with @Before/AfterClass or at the object/singleton level

Right, removed.

ouchadam · 2022-06-22T15:49:08Z

...rc/test/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactoryTest.kt

+
+    @Test
+    fun `given an ended poll then all option view states is Ended`() = runTest {
+        with(pollItemFactory) {


I would argue that the optionViewStates can exist on a parent PollItemViewState, if we want to avoid too many conditions under a single class, we can extract out another factory which the PollItemViewStateFactory could depend on

// Our PollItemViewState test doesn't need to know how the option view state is generated // that class can have its own tests fakeOptionViewStateFactory.given(A_OPTION_VIEW_STATE) val result = factory.create() result shouldBeEqualTo Result( optionViewStates = A_OPTION_VIEW_STATE )

Refactored.

mnaturel · 2022-06-23T08:06:34Z

...in/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactoryHelper.kt

+import im.vector.app.features.home.room.detail.timeline.TimelineEventController
+import im.vector.app.features.home.room.detail.timeline.item.MessageInformationData
+
+object MessageItemFactoryHelper {


I think in general it is better to create classic injectable classes and avoid object. Objects are not scoped to only where they are needed and are not suitable for unit tests.

No need to a helper class after refactoring.

mnaturel · 2022-06-23T08:16:42Z

...rc/test/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactoryTest.kt

+    private val testDispatcher = UnconfinedTestDispatcher()
+
+    @get:Rule
+    val mvRxTestRule = MvRxTestRule(


Here this rule is not needed since the PollItemFactory does not rely on coroutines. And it is only useful in ViewModel and when switching the context Dispatcher.

mnaturel · 2022-06-23T08:18:33Z

...rc/test/java/im/vector/app/features/home/room/detail/timeline/factory/PollItemFactoryTest.kt

+    }
+
+    @Test
+    fun `given a sending poll state then PollState is Sending`() = runTest {


runTest can be removed from all tests. It is only useful when testing suspend functions.

mnaturel

LGTM!

ouchadam · 2022-06-27T14:47:11Z

...ava/im/vector/app/features/home/room/detail/timeline/factory/PollItemViewStateFactoryTest.kt

+    @Before
+    fun setup() {
+        // We are not going to test any UI related code
+        pollItemViewStateFactory = PollItemViewStateFactory(


we can inline the creation as the junit test runner will create a new instance of the test class for each test

private val stringProvider = FakeStringProvider() private val pollItemViewStateFactory = PollItemViewStateFactory(stringProvider.instance)

sonarqubecloud · 2022-06-27T16:26:11Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

73.7% Coverage
0.0% Duplication

ouchadam · 2022-06-27T16:29:10Z

...ava/im/vector/app/features/home/room/detail/timeline/factory/PollItemViewStateFactoryTest.kt

+
+        pollViewState shouldBeEqualTo PollViewState(
+                question = A_POLL_CONTENT.getBestPollCreationInfo()?.question?.getBestQuestion() ?: "",
+                totalVotes = stringProvider.instance.getString(R.string.poll_no_votes_cast),


because we're using the fakeStringProvider it means our expected values can be R.string.poll_no_votes_cast.toTestString() (rather than using the provider directly)

So the test is not passing?

(oh yes, please ignore me)

ideally our expected data creation would avoid calling any real functions or mocks as it makes them more susceptible to unrelated changes

eg if the StringProvider.getString function parameters change, this unrelated individual test will also need to be updated

In this case we know the expected value is the computed value of R.string.poll_no_votes_cast which we can create with the .toTestString() extension, fetching the value from the fake we've already setup is unnecessary

ouchadam

tiny comment about the test string resource, otherwise great PR! the tests look nice and simple 💯 (which should make them easier to maintain 🤞 )

bmarty · 2022-06-27T17:58:05Z

vector/src/test/java/im/vector/app/test/fakes/FakeStringProvider.kt

@@ -27,6 +27,10 @@ class FakeStringProvider {
        every { instance.getString(any()) } answers {
            "test-${args[0]}"
        }
+
+        every { instance.getQuantityString(any(), any(), any()) } answers {
+            "test-${args[0]}-${args[1]}"


Quick question, why don't we use args[2] here?

bmarty

Only one small question, will force push

onurays added 13 commits June 21, 2022 17:03

Refactor poll item factory to make it testable.

bd9fa48

Create initial test class.

77dfd5f

Test sending poll state.

a886e93

Test ended poll state.

8854b81

Test undisclosed poll state.

0fe4b9f

Test voted poll state.

2c5ddca

Test ready poll state.

5a94889

Test sending option view states.

2cf40cb

Test ready option view states.

0f0492d

Test poll voted option view states.

8bb421a

Test undisclosed option view states.

d0d2929

Test ended poll option view states.

aab558a

Changelog added.

a7bc2ef

onurays requested review from a team, Claire1817 and mnaturel and removed request for a team June 22, 2022 13:46

onurays added the A-Polls label Jun 22, 2022

Johennes requested review from ouchadam and removed request for Claire1817 June 22, 2022 14:09

Test isVotable function.

2be43e9

onurays changed the title ~~Poll view state unit tests~~ Poll view state unit tests [PSF-1130] Jun 22, 2022

ouchadam reviewed Jun 22, 2022

View reviewed changes

mnaturel reviewed Jun 23, 2022

View reviewed changes

onurays added 9 commits June 24, 2022 16:52

Refactor poll item view state factory.

532bc18

Move epoxy related poll functions back to MessageItemFactory.

e63fa2d

Fix sending poll unit test.

a9358e9

Fix ended poll unit test.

1a668da

Fix undisclosed poll unit test.

6f4e079

Fix voted poll unit test.

d7c2dbe

Fix ready poll unit test.

4f9b361

Remove duplicated unit tests.

8100a2e

Code review fixes.

30115fa

onurays requested review from ouchadam and mnaturel June 27, 2022 12:44

mnaturel approved these changes Jun 27, 2022

View reviewed changes

ouchadam reviewed Jun 27, 2022

View reviewed changes

Code review fix.

863cc7e

onurays requested a review from ouchadam June 27, 2022 15:37

ouchadam reviewed Jun 27, 2022

View reviewed changes

ouchadam approved these changes Jun 27, 2022

View reviewed changes

bmarty reviewed Jun 27, 2022

View reviewed changes

bmarty approved these changes Jun 27, 2022

View reviewed changes

bmarty merged commit a398391 into develop Jun 27, 2022

bmarty deleted the feature/ons/poll_view_state_unit_tests branch June 27, 2022 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Poll view state unit tests [PSF-1130] #6366

Poll view state unit tests [PSF-1130] #6366

onurays commented Jun 22, 2022 •

edited

Loading

ouchadam Jun 22, 2022 •

edited

Loading

onurays Jun 24, 2022

ouchadam Jun 22, 2022 •

edited

Loading

onurays Jun 27, 2022

ouchadam Jun 22, 2022

ouchadam Jun 22, 2022

onurays Jun 27, 2022

ouchadam Jun 22, 2022

onurays Jun 27, 2022

ouchadam Jun 22, 2022

onurays Jun 27, 2022

mnaturel Jun 23, 2022

onurays Jun 27, 2022

mnaturel Jun 23, 2022

onurays Jun 27, 2022

mnaturel Jun 23, 2022

onurays Jun 27, 2022

mnaturel left a comment

ouchadam Jun 27, 2022

sonarqubecloud bot commented Jun 27, 2022

ouchadam Jun 27, 2022

bmarty Jun 27, 2022

bmarty Jun 27, 2022

ouchadam Jun 28, 2022

ouchadam left a comment

bmarty Jun 27, 2022

bmarty left a comment

Poll view state unit tests [PSF-1130] #6366

Poll view state unit tests [PSF-1130] #6366

Conversation

onurays commented Jun 22, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

ouchadam Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

onurays commented Jun 22, 2022 •

edited

Loading

ouchadam Jun 22, 2022 •

edited

Loading

ouchadam Jun 22, 2022 •

edited

Loading