Skip to content

Konsist #1526

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

Merged
merged 11 commits into from
Oct 11, 2023
Merged

Konsist #1526

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .idea/dictionaries/shared.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* [knit](#knit)
* [lint](#lint)
* [Unit tests](#unit-tests)
* [konsist](#konsist)
* [Tests](#tests)
* [Accessibility](#accessibility)
* [Jetpack Compose](#jetpack-compose)
Expand Down Expand Up @@ -156,6 +157,10 @@ Make sure the following commands execute without any error:
./gradlew test
</pre>

#### konsist

[konsist](https://github.com/LemonAppDev/konsist) is setup in the project to check that the architecture and the naming rules are followed. Konsist tests are classical Unit tests.

### Tests

Element X is currently supported on Android Marshmallow (API 23+): please test your change on an Android device (or Android emulator) running with API 23. Many issues can happen (including crashes) on older devices.
Expand Down
1 change: 1 addition & 0 deletions app/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ dependencies {
testImplementation(libs.test.truth)
testImplementation(libs.test.turbine)
testImplementation(projects.libraries.matrix.test)
testImplementation(libs.test.konsist)

ksp(libs.showkase.processor)
}
2 changes: 1 addition & 1 deletion app/src/main/kotlin/io/element/android/x/MainActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import com.bumble.appyx.core.integrationpoint.NodeComponentActivity
import com.bumble.appyx.core.plugin.NodeReadyObserver
import io.element.android.libraries.architecture.bindings
import io.element.android.libraries.core.log.logger.LoggerTag
import io.element.android.libraries.designsystem.utils.LocalSnackbarDispatcher
import io.element.android.libraries.designsystem.utils.snackbar.LocalSnackbarDispatcher
import io.element.android.libraries.theme.ElementTheme
import io.element.android.x.di.AppBindings
import io.element.android.x.intent.SafeUriHandler
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package io.element.android.x.di

import com.squareup.anvil.annotations.ContributesTo
import io.element.android.features.rageshake.api.reporter.BugReporter
import io.element.android.libraries.designsystem.utils.SnackbarDispatcher
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.matrix.api.tracing.TracingService

Expand Down
2 changes: 1 addition & 1 deletion app/src/main/kotlin/io/element/android/x/di/AppModule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import io.element.android.features.messages.impl.timeline.components.customreact
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.core.meta.BuildMeta
import io.element.android.libraries.core.meta.BuildType
import io.element.android.libraries.designsystem.utils.SnackbarDispatcher
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.di.ApplicationContext
import io.element.android.libraries.di.DefaultPreferences
Expand Down
139 changes: 139 additions & 0 deletions app/src/test/kotlin/io/element/android/app/KonsistTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/*
* Copyright (c) 2023 New Vector Ltd
*
* 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
*
* http://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 io.element.android.app

import androidx.compose.runtime.Composable
import com.lemonappdev.konsist.api.KoModifier
import com.lemonappdev.konsist.api.Konsist
import com.lemonappdev.konsist.api.ext.list.constructors
import com.lemonappdev.konsist.api.ext.list.modifierprovider.withoutModifier
import com.lemonappdev.konsist.api.ext.list.modifierprovider.withoutOverrideModifier
import com.lemonappdev.konsist.api.ext.list.parameters
import com.lemonappdev.konsist.api.ext.list.properties
import com.lemonappdev.konsist.api.ext.list.withAllAnnotationsOf
import com.lemonappdev.konsist.api.ext.list.withAllParentsOf
import com.lemonappdev.konsist.api.ext.list.withNameEndingWith
import com.lemonappdev.konsist.api.ext.list.withReturnType
import com.lemonappdev.konsist.api.ext.list.withTopLevel
import com.lemonappdev.konsist.api.ext.list.withoutName
import com.lemonappdev.konsist.api.ext.list.withoutNameEndingWith
import com.lemonappdev.konsist.api.verify.assertFalse
import com.lemonappdev.konsist.api.verify.assertTrue
import io.element.android.libraries.architecture.Presenter
import io.element.android.libraries.designsystem.preview.PreviewsDayNight
import org.junit.Test

class KonsistTest {

@Test
fun `Classes extending 'Presenter' should have 'Presenter' suffix`() {
Konsist.scopeFromProject()
.classes()
.withAllParentsOf(Presenter::class)
.assertTrue {
it.name.endsWith("Presenter")
}
}

@Test
fun `Functions with '@PreviewsDayNight' annotation should have 'Preview' suffix`() {
Konsist
.scopeFromProject()
.functions()
.withAllAnnotationsOf(PreviewsDayNight::class)
.assertTrue {
it.hasNameEndingWith("Preview") &&
it.hasNameEndingWith("LightPreview").not() &&
it.hasNameEndingWith("DarkPreview").not()
}
}

@Test
fun `Top level function with '@Composable' annotation starting with a upper case should be placed in a file with the same name`() {

Choose a reason for hiding this comment

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

Isn't detekt already checking this (or maybe it checks something similar?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, Detekt is checking class name / filename, but not Composable function name / filename

Konsist
.scopeFromProject()
.functions()
.withTopLevel()
.withoutModifier(KoModifier.PRIVATE)
.withoutNameEndingWith("Preview")
.withAllAnnotationsOf(Composable::class)
.withoutName(
// Add some exceptions...
"OutlinedButton",
"TextButton",
"SimpleAlertDialogContent",
)
.assertTrue(
additionalMessage =
"""
Please check the filename. It should match the top level Composable function. If the filename is correct:
- consider making the Composable private or moving it to its own file
- at last resort, you can add an exception in the Konsist test
""".trimIndent()
) {
if (it.name.first().isLowerCase()) {
true
} else {
val fileName = it.containingFile.name.removeSuffix(".kt")
fileName == it.name
}
}
}

@Test
fun `Data class state MUST not have default value`() {

Choose a reason for hiding this comment

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

Should we at least allow a default {} for the event sink?

Copy link
Member Author

Choose a reason for hiding this comment

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

This idea is to help the developer remember (by not compiling the code otherwise) that all the fields in the state should be provided when created by the presenter. So I think it's safer to enforce this for all the params.

Konsist
.scopeFromProject()
.classes()
.withNameEndingWith("State")
.withoutName(
"CameraPositionState",
)
.constructors
.parameters
.assertTrue { parameterDeclaration ->
parameterDeclaration.defaultValue == null &&
// Using parameterDeclaration.defaultValue == null is not enough apparently,
// Also check that the text does not contain an equal sign
parameterDeclaration.text.contains("=").not()
}
}

@Test
fun `Function which creates Presenter in test MUST be named 'createPresenterName'`() {
Konsist
.scopeFromTest()
.functions()
.withReturnType { it.name.endsWith("Presenter") }
.withoutOverrideModifier()
.assertTrue { functionDeclaration ->
functionDeclaration.name == "create${functionDeclaration.returnType?.name}"
}
}

@Test
fun `no field should have 'm' prefix`() {

Choose a reason for hiding this comment

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

:D

Konsist
.scopeFromProject()
.classes()
.properties()
.assertFalse {
val secondCharacterIsUppercase = it.name.getOrNull(1)?.isUpperCase() ?: false
it.name.startsWith('m') && secondCharacterIsUppercase
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package io.element.android.appnav

import io.element.android.libraries.designsystem.utils.SnackbarDispatcher
import io.element.android.libraries.designsystem.utils.SnackbarMessage
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarMessage
import io.element.android.libraries.matrix.api.room.RoomMembershipObserver
import io.element.android.libraries.matrix.api.verification.SessionVerificationService
import io.element.android.libraries.matrix.api.verification.VerificationFlowState
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ import io.element.android.libraries.architecture.animation.rememberDefaultTransi
import io.element.android.libraries.architecture.createNode
import io.element.android.libraries.architecture.waitForChildAttached
import io.element.android.libraries.deeplink.DeeplinkData
import io.element.android.libraries.designsystem.utils.SnackbarDispatcher
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
import io.element.android.libraries.di.SessionScope
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.matrix.api.core.MAIN_SPACE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class RootPresenterTest {

@Test
fun `present - initial state`() = runTest {
val presenter = createPresenter()
val presenter = createRootPresenter()
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
Expand All @@ -54,7 +54,7 @@ class RootPresenterTest {

@Test
fun `present - passes app error state`() = runTest {
val presenter = createPresenter(
val presenter = createRootPresenter(
appErrorService = DefaultAppErrorStateService().apply {
showError("Bad news", "Something bad happened")
}
Expand All @@ -75,7 +75,7 @@ class RootPresenterTest {
}
}

private fun createPresenter(
private fun createRootPresenter(
appErrorService: AppErrorStateService = DefaultAppErrorStateService()
): RootPresenter {
val crashDataStore = FakeCrashDataStore()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class LoggedInPresenterTest {

@Test
fun `present - initial state`() = runTest {
val presenter = createPresenter()
val presenter = createLoggedInPresenter()
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
Expand All @@ -54,7 +54,7 @@ class LoggedInPresenterTest {
@Test
fun `present - show sync spinner`() = runTest {
val roomListService = FakeRoomListService()
val presenter = createPresenter(roomListService, NetworkStatus.Online)
val presenter = createLoggedInPresenter(roomListService, NetworkStatus.Online)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
Expand All @@ -66,7 +66,7 @@ class LoggedInPresenterTest {
}
}

private fun createPresenter(
private fun createLoggedInPresenter(
roomListService: RoomListService = FakeRoomListService(),
networkStatus: NetworkStatus = NetworkStatus.Offline
): LoggedInPresenter {
Expand Down
1 change: 1 addition & 0 deletions changelog.d/1526.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add some Konsist tests.
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ fun AddPeopleView(

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun AddPeopleViewTopBar(
private fun AddPeopleViewTopBar(
hasSelectedUsers: Boolean,
modifier: Modifier = Modifier,
onBackPressed: () -> Unit = {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ fun ConfigureRoomView(

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun ConfigureRoomToolbar(
private fun ConfigureRoomToolbar(
isNextActionEnabled: Boolean,
modifier: Modifier = Modifier,
onBackPressed: () -> Unit = {},
Expand All @@ -207,7 +207,7 @@ fun ConfigureRoomToolbar(
}

@Composable
fun RoomNameWithAvatar(
private fun RoomNameWithAvatar(
avatarUri: Uri?,
roomName: String,
modifier: Modifier = Modifier,
Expand Down Expand Up @@ -235,7 +235,7 @@ fun RoomNameWithAvatar(
}

@Composable
fun RoomTopic(
private fun RoomTopic(
topic: String,
modifier: Modifier = Modifier,
onTopicChanged: (String) -> Unit = {},
Expand All @@ -254,7 +254,7 @@ fun RoomTopic(
}

@Composable
fun RoomPrivacyOptions(
private fun RoomPrivacyOptions(
selected: RoomPrivacy?,
modifier: Modifier = Modifier,
onOptionSelected: (RoomPrivacyItem) -> Unit = {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ fun CreateRoomRootView(

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun CreateRoomRootViewTopBar(
private fun CreateRoomRootViewTopBar(
modifier: Modifier = Modifier,
onClosePressed: () -> Unit = {},
) {
Expand All @@ -148,7 +148,7 @@ fun CreateRoomRootViewTopBar(
}

@Composable
fun CreateRoomActionButtonsList(
private fun CreateRoomActionButtonsList(
state: CreateRoomRootState,
modifier: Modifier = Modifier,
onNewRoomClicked: () -> Unit = {},
Expand All @@ -169,7 +169,7 @@ fun CreateRoomActionButtonsList(
}

@Composable
fun CreateRoomActionButton(
private fun CreateRoomActionButton(
@DrawableRes iconRes: Int,
text: String,
modifier: Modifier = Modifier,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ import kotlinx.collections.immutable.ImmutableList
@Immutable
data class InviteListState(
val inviteList: ImmutableList<InviteListInviteSummary>,
val declineConfirmationDialog: InviteDeclineConfirmationDialog = InviteDeclineConfirmationDialog.Hidden,
val acceptedAction: Async<RoomId> = Async.Uninitialized,
val declinedAction: Async<Unit> = Async.Uninitialized,
val eventSink: (InviteListEvents) -> Unit = {}
val declineConfirmationDialog: InviteDeclineConfirmationDialog,
val acceptedAction: Async<RoomId>,
val declinedAction: Async<Unit>,
val eventSink: (InviteListEvents) -> Unit
)

sealed interface InviteDeclineConfirmationDialog {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ open class InviteListStateProvider : PreviewParameterProvider<InviteListState> {

internal fun aInviteListState() = InviteListState(
inviteList = aInviteListInviteSummaryList(),
declineConfirmationDialog = InviteDeclineConfirmationDialog.Hidden,
acceptedAction = Async.Uninitialized,
declinedAction = Async.Uninitialized,
eventSink = {},
)

internal fun aInviteListInviteSummaryList(): ImmutableList<InviteListInviteSummary> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ fun InviteListView(

@OptIn(ExperimentalMaterial3Api::class, ExperimentalLayoutApi::class)
@Composable
fun InviteListContent(
private fun InviteListContent(
state: InviteListState,
modifier: Modifier = Modifier,
onBackClicked: () -> Unit = {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ internal fun InviteSummaryRow(
}

@Composable
internal fun DefaultInviteSummaryRow(
private fun DefaultInviteSummaryRow(
invite: InviteListInviteSummary,
onAcceptClicked: () -> Unit = {},
onDeclineClicked: () -> Unit = {},
Expand Down
Loading