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

Write Test Code In 'core.domain' package #1359

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
77677e9
Add GetRecentSearchQueriesUseCaseTest.kt ([#1327](https://github.com/…
squart300kg Apr 3, 2024
e1ee9b4
Add GetRecentSearchQueriesUseCaseTest.kt (#1327)
squart300kg Apr 3, 2024
31a4560
Revert "Add GetRecentSearchQueriesUseCaseTest.kt (#1327)"
squart300kg Apr 3, 2024
7313f6c
Merge remote-tracking branch 'origin/Issue1327/ImproveDomainCoverage'…
squart300kg Apr 3, 2024
0f711be
Fix Change the way data is created in flow ([#1327])
squart300kg Apr 3, 2024
17c937f
Add test code : whenNoParams_recentSearchQueriesAreReturnedUpTo10 ([#…
squart300kg Apr 3, 2024
db16878
FIX test code : whenNoParams_recentSearchQueriesAreReturnedUpTo10 ([#…
squart300kg Apr 3, 2024
b782291
FIX test code : whenParamIsSetTo5_recentSearchQueriesAreReturnedUpTo5…
squart300kg Apr 3, 2024
98fd9ee
FIX test code : whenParamIsSetTo5_recentSearchQueriesAreReturnedUpTo5…
squart300kg Apr 3, 2024
16d73b3
Add test code in GetFollowableTopicsUseCaseTest.kt([#1327])
squart300kg Apr 3, 2024
80c147c
Merge remote-tracking branch 'origin/Issue1327/ImproveDomainCoverage'…
squart300kg Apr 3, 2024
5b1c704
FIX test code : apply code formmat ([#1327])
squart300kg Apr 3, 2024
ba04452
Update test code by applying code conventions and reflecting feedback
squart300kg Apr 4, 2024
bf1b117
apply code convention with spotlessApply ([#1327])
squart300kg Apr 4, 2024
2a9fad4
Update test code by applying code conventions and reflecting feedback…
squart300kg Apr 4, 2024
212e957
Merge branch 'Issue1327/ImproveDomainCoverage' of https://github.com/…
squart300kg Apr 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,112 @@
/*
* Copyright 2024 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.samples.apps.nowinandroid.core.domain

import com.google.samples.apps.nowinandroid.core.testing.repository.TestRecentSearchRepository
import com.google.samples.apps.nowinandroid.core.testing.util.MainDispatcherRule
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.test.runTest
import org.junit.Rule
import org.junit.Test
import kotlin.test.assertEquals

class GetRecentSearchQueriesUseCaseTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. i will check this convention.


@get:Rule
val mainDispatcherRule = MainDispatcherRule()

private val recentSearchRepository = TestRecentSearchRepository()

private val useCase = GetRecentSearchQueriesUseCase(
recentSearchRepository,
)

@Test
fun whenNoParams_recentSearchQueriesAreReturnedUpTo10() = runTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Params should be replaced by its actual name, which is "limit"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, your opinion is more reasonable. but, test code naming convention(='whenNoParams_followableTopicsAreReturnedWithNoSorting' in 'GetFollowableTopicsUseCaseTest') is not correct.

so according to what you said, is it correct that this method name should also be modified? if yes, i will try one of them.

  1. Modifying the method name of 'whenNoParams_followableTopicsAreReturnedWithNoSorting' together and pull request.
  2. Register as another issue and proceed.
  3. Or, is there a reason to maintain the name?

// Obtain a stream of recent search queries with no param.
squart300kg marked this conversation as resolved.
Show resolved Hide resolved
val recentSearchQueries = useCase()

// insert 5 search queries.
for (index in 0 until 5) {
Copy link
Contributor

Choose a reason for hiding this comment

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

testRecentSearchQueries.take(5).forEach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that the loop type of 'forEach' is mainly used exceping for 'for (item in items)' type. i will use these type from now on :)

recentSearchRepository.insertOrReplaceRecentSearch(testRecentSearchQueries[index])
// delay for saving value
delay(10L)
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't rely on timing in unit tests. It's an in-memory cache, why do you need the delay in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the loop for saving search queries, there is an issue where the sequence of the saved values is not maintained in order. I hypothesized that the problem might be due to the timing of saving values multiple times.

Therefore, I applied a delay of 10 milliseconds. and so, i resolve for timing issue.

If there are any issues with this approach, I would appreciate your review.

}

// Check that 5 recent search queries are ordered by latest.
assertEquals(
testRecentSearchQueries.take(5).reversed(),
recentSearchQueries.first().map { it.query },
)

// insert 9 more search queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to do this again? I think this should be in a different test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after careful consideration, it is correct for deleting this code. :)

for (index in 5 until testRecentSearchQueries.size) {
recentSearchRepository.insertOrReplaceRecentSearch(testRecentSearchQueries[index])
// delay for saving value
delay(10L)
}

// Check that recent search queries are ordered by latest up to 10.
assertEquals(
testRecentSearchQueries.reversed().take(10),
recentSearchQueries.first().map { it.query },
)
}

@Test
fun whenParamIsSetTo5_recentSearchQueriesAreReturnedUpTo5() = runTest {
// Obtain a stream of recent search queries with param set 5.
val recentSearchQueries = useCase(5)

// insert 2 search queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already tested in a previous test, can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regardless of parameter injection, after thinking about it again, I think the test logic is duplicate.

for (index in 0 until 2) {
recentSearchRepository.insertOrReplaceRecentSearch(testRecentSearchQueries[index])
// delay for saving value
delay(10L)
}

// Check that 5 recent search queries are ordered by latest.
assertEquals(
testRecentSearchQueries.take(2).reversed(),
recentSearchQueries.first().map { it.query },
)

// insert 12 more search queries.
for (index in 2 until testRecentSearchQueries.size) {
recentSearchRepository.insertOrReplaceRecentSearch(testRecentSearchQueries[index])
// delay for saving value
delay(10L)
}

// Check that recent search queries are ordered by latest up to 5.
assertEquals(
testRecentSearchQueries.reversed().take(5),
recentSearchQueries.first().map { it.query },
)
}
}

private val testRecentSearchQueries = listOf(
"Compose", "Wear OS",
"Jetpack", "Headlines",
"Architecture", "UI",
"Testing", "Android Studio",
"Performance", "New API",
"Games", "Android TV",
"Camera", "Media",
)
Expand Up @@ -19,14 +19,16 @@ package com.google.samples.apps.nowinandroid.core.testing.repository
import com.google.samples.apps.nowinandroid.core.data.model.RecentSearchQuery
import com.google.samples.apps.nowinandroid.core.data.repository.RecentSearchRepository
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.flow

class TestRecentSearchRepository : RecentSearchRepository {

private val cachedRecentSearches: MutableList<RecentSearchQuery> = mutableListOf()
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using MutableSharedFlow like another TestRepository? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yongsuk44 I agree with your opinion. but because i didn't fully understand using 'MutableList' instead of "MutableSahredFlow' like other unit test code, i did not change this code.

and the primary purpose of this 'PR' is writing the test code of 'UseCase' class in domain layer. :)

i have thought that your opinion is reasonable so far 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@squart300kg
I've determined that handling test cases reactively when utilizing this repository in the future would likely lead to a smoother process. Hence, I've left a comment to that effect. :)


override fun getRecentSearchQueries(limit: Int): Flow<List<RecentSearchQuery>> =
flowOf(cachedRecentSearches.sortedByDescending { it.queriedDate }.take(limit))
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i think that this code need to be changed. because the flow producers such as 'flowOf' and 'flow { emit() }' have different in emitting updated list. and although one depth inner code is same when emitting data by 'flow { emit() }, two depth inner code is different. because of this, i realized that the way of data emitting can not be same.

[two depth inner code]
스크린샷 2024-04-04 오전 10 49 00
left : flow { emit() } / right : flowOf()
additional reference, my blog column(lang korean) : different between flowOf and flow { emit() }

because of this change, i had tested 'SearchViewModelTest' and passed. so, i think that this change is reasonable.

how about your opinion? :)

flow {
emit(cachedRecentSearches.sortedByDescending { it.queriedDate }.take(limit))
}

override suspend fun insertOrReplaceRecentSearch(searchQuery: String) {
cachedRecentSearches.add(RecentSearchQuery(searchQuery))
Expand Down