From c2876962075addb9edc31fcbd75fea84041c1461 Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Sun, 24 Aug 2025 15:21:10 +0200 Subject: [PATCH 1/5] Minor improvements of testing documentation - Fixed instructions for running tests. The mentioned example test had been refactored so the given commands were not able to locate a test to run. - Fixed copy/paste errors in test comments - Documented how to write server-based tests - Extended formatter setup documentation Signed-off-by: Philipp Hasper --- CONTRIBUTING.md | 42 +++++++++++++++---- README.md | 1 + .../owncloud/android/AbstractOnServerIT.java | 11 ++++- .../java/com/owncloud/android/DownloadIT.java | 2 +- 4 files changed, 46 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5fab39edd484..510fd5e94885 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ 1. [Contributing to Source Code](#contributing-to-source-code) 1. [Developing process](#developing-process) 1. [Branching model](#branching-model) - 1. [Android Studio formatter setup](#android-studio-formatter-setup) + 1. [Formatter setup](#formatter-setup) 1. [Build variants](#build-variants) 1. [Git hooks](#git-hooks) 1. [Contribution process](#contribution-process) @@ -107,12 +107,22 @@ We are all about quality while not sacrificing speed so we use a very pragmatic * Hot fixes not relevant for an upcoming feature release but the latest release can target the bug fix branch directly -### Android Studio formatter setup +### Formatter setup Our formatter setup is rather simple: * Standard Android Studio -* Line length 120 characters (Settings->Editor->Code Style->Right margin(columns): 120) +* Line length 120 characters (Settings->Editor->Code Style->Right margin(columns): 120; also set by EditorConfig * Auto optimize imports (Settings->Editor->Auto Import->Optimize imports on the fly) +You can fix Check / check (spotlessKotlinCheck) via following commands: + +```bash +./gradlew spotlessApply +./gradlew detekt +./gradlew spotlessCheck +./gradlew spotlessKotlinCheck +``` + +See section [Git hooks](#git-hooks) to have these run automatically with your commits. ### Build variants There are three build variants @@ -122,7 +132,7 @@ There are three build variants ### Git hooks We provide git hooks to make development process easier for both the developer and the reviewers. -To install them, just run: +They are stored in [/scripts/hooks](/scripts/hooks) and can be installed with: ```bash ./gradlew installGitHooks @@ -214,21 +224,37 @@ Source code of app: - small, isolated tests, with no need of Android SDK - code coverage can be directly shown via right click on test and select "Run Test with Coverage" +``` +./gradlew jacocoTestGplayDebugUnitTest +```bash + #### Instrumented tests - tests to see larger code working in correct way - tests that require parts of Android SDK -- best to avoid server communication, see https://github.com/nextcloud/android/pull/3624 - run all tests ```./gradlew createGplayDebugCoverageReport -Pcoverage=true``` - run selective test class: ```./gradlew createGplayDebugCoverageReport -Pcoverage=true - -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.datamodel.FileDataStorageManagerTest``` + -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.datamodel.FileDataStorageManagerContentProviderClientIT``` - run multiple test classes: - separate by "," - - ```./gradlew createGplayDebugCoverageReport -Pcoverage=true -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.datamodel.FileDataStorageManagerTest,com.nextcloud.client.FileDisplayActivityIT``` + - ```./gradlew createGplayDebugCoverageReport -Pcoverage=true -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.datamodel.FileDataStorageManagerContentProviderClientIT,com.nextcloud.client.FileDisplayActivityIT``` - run one test in class: ```./gradlew createGplayDebugCoverageReport -Pcoverage=true - -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.datamodel.FileDataStorageManagerTest#saveNewFile``` + -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.datamodel.FileDataStorageManagerContentProviderClientIT#saveFile``` - JaCoCo results are shown as html: firefox ./build/reports/coverage/gplay/debug/index.html +#### Instrumented tests with server communication +It is best to avoid server communication, see https://github.com/nextcloud/android/pull/3624. +But if a test requires a server, this is how it is done: +- Have a Nextcloud service reachable by your test device. This can be an existing server in the internet or a locally deployed one +as per the [server developer documentation](https://docs.nextcloud.com/server/latest/developer_manual/getting_started/devenv.html) +- Create a separate(!) test user on that server, otherwise the tests will infer with productive data. +- In `gradle.properties`, enter the URL, user name and password via the `NC_TEST_SERVER_...` attributes. + If you want to prevent an accidental commit of those, you can also store them in `~/.gradle/gradle.properties`. +- Your test class should inherit from `AbstractOnServerIT`, e.g.: `public class DownloadIT extends AbstractOnServerIT { ...` + Note that this will automatically delete all files after each test run, so you absolutely NEED a separate test user as mentioned above. +- All preconditions of your test regarding server data, e.g. existing files, need to be established by your test itself. + As a reference, see how `DownloadIT` first uploads the files it later tests the download with. +- Clean up these preconditions again, also in the failure case, using one or multiple `@After` methods in your test class. #### UI tests We use [shot](https://github.com/Karumi/Shot) for taking screenshots and compare them diff --git a/README.md b/README.md index bd99dd07fe89..1a23a124eb06 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ If you want to [contribute](https://nextcloud.com/contribute/) to the Nextcloud * reporting problems / suggesting enhancements by [opening new issues](https://github.com/nextcloud/android/issues/new/choose) * implementing proposed bug fixes and enhancement ideas by submitting PRs (associated with a corresponding issue preferably) * reviewing [pull requests](https://github.com/nextcloud/android/pulls) and providing feedback on code, implementation, and functionality +* Add [automated tests](CONTRIBUTING.md#testing) for existing functionality * installing and testing [pull request builds](https://github.com/nextcloud/android/pulls), [daily/dev builds](https://github.com/nextcloud/android#development-version-hammer), or [RCs/release candidate builds](https://github.com/nextcloud/android/releases) * enhancing Admin, User, or Developer [documentation](https://github.com/nextcloud/documentation/) * hitting hard on the latest stable release by testing fundamental features and evaluating the user experience diff --git a/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java b/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java index fd452db6f74f..b20a26ff1cbd 100644 --- a/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java +++ b/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java @@ -53,7 +53,16 @@ import static org.junit.Assert.assertTrue; /** - * Common base for all integration tests. + * Common base for all integration tests requiring a server connection. + * ATTENTION: Deletes ALL files of the test user on the server after each test run. + * So you MUST use a dedicated test user. + * + * Uses server, user and password given as `testInstrumentationRunnerArgument` + * - TEST_SERVER_URL + * - TEST_SERVER_USERNAME + * - TEST_SERVER_PASSWORD + * These are supplied via build.gradle, which takes them from gradle.properties. + * So look in the latter file to set to your own server & test user. */ public abstract class AbstractOnServerIT extends AbstractIT { @BeforeClass diff --git a/app/src/androidTest/java/com/owncloud/android/DownloadIT.java b/app/src/androidTest/java/com/owncloud/android/DownloadIT.java index 610469f4d3fa..d3099b251562 100644 --- a/app/src/androidTest/java/com/owncloud/android/DownloadIT.java +++ b/app/src/androidTest/java/com/owncloud/android/DownloadIT.java @@ -29,7 +29,7 @@ import static org.junit.Assert.assertTrue; /** - * Tests related to file uploads. + * Tests related to file downloads. */ public class DownloadIT extends AbstractOnServerIT { private static final String FOLDER = "/testUpload/"; From 4c07da74c239831574fdbcef154b88f71d159e05 Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Sun, 24 Aug 2025 17:38:57 +0200 Subject: [PATCH 2/5] Instrumentation tests now automatically grab the notifications permission Before that, when starting individual tests from the command line or from inside the IDE, they could fail because a dialog asking for the permission to post notifications was blocking the view. While we are on it, added a small explanation to the other existing rule. Without that explanation it might be unclear why this is not also done via the same GrantPermissionRule used for the notifications. Signed-off-by: Philipp Hasper --- .../java/com/nextcloud/test/GrantStoragePermissionRule.kt | 5 +++++ .../androidTest/java/com/owncloud/android/AbstractIT.java | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/src/androidTest/java/com/nextcloud/test/GrantStoragePermissionRule.kt b/app/src/androidTest/java/com/nextcloud/test/GrantStoragePermissionRule.kt index b310a897fa42..3bade62247c4 100644 --- a/app/src/androidTest/java/com/nextcloud/test/GrantStoragePermissionRule.kt +++ b/app/src/androidTest/java/com/nextcloud/test/GrantStoragePermissionRule.kt @@ -15,6 +15,10 @@ import org.junit.rules.TestRule import org.junit.runner.Description import org.junit.runners.model.Statement +/** + * Rule to automatically enable the test to write to the external storage. + * Depending on the SDK version, different approaches might be necessary to achieve the full access. + */ class GrantStoragePermissionRule private constructor() { companion object { @@ -30,6 +34,7 @@ class GrantStoragePermissionRule private constructor() { private class GrantManageExternalStoragePermissionRule : TestRule { override fun apply(base: Statement, description: Description): Statement = object : Statement() { override fun evaluate() { + // Refer to https://developer.android.com/training/data-storage/manage-all-files#enable-manage-external-storage-for-testing InstrumentationRegistry.getInstrumentation().uiAutomation.executeShellCommand( "appops set --uid ${InstrumentationRegistry.getInstrumentation().targetContext.packageName} " + "MANAGE_EXTERNAL_STORAGE allow" diff --git a/app/src/androidTest/java/com/owncloud/android/AbstractIT.java b/app/src/androidTest/java/com/owncloud/android/AbstractIT.java index 5ea27541062f..8e6c34db085c 100644 --- a/app/src/androidTest/java/com/owncloud/android/AbstractIT.java +++ b/app/src/androidTest/java/com/owncloud/android/AbstractIT.java @@ -6,6 +6,7 @@ */ package com.owncloud.android; +import android.Manifest; import android.accounts.Account; import android.accounts.AccountManager; import android.accounts.AuthenticatorException; @@ -78,6 +79,7 @@ import androidx.test.espresso.contrib.DrawerActions; import androidx.test.espresso.intent.rule.IntentsTestRule; import androidx.test.platform.app.InstrumentationRegistry; +import androidx.test.rule.GrantPermissionRule; import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry; import androidx.test.runner.lifecycle.Stage; @@ -93,7 +95,10 @@ */ public abstract class AbstractIT { @Rule - public final TestRule permissionRule = GrantStoragePermissionRule.grant(); + public final TestRule storagePermissionRule = GrantStoragePermissionRule.grant(); + + @Rule + public GrantPermissionRule notificationsPermissionRule = GrantPermissionRule.grant(Manifest.permission.POST_NOTIFICATIONS); protected static OwnCloudClient client; protected static NextcloudClient nextcloudClient; From f6a235cf7fbc2b20d7d84ce14d3e97d60712d783 Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Tue, 28 Oct 2025 18:58:45 +0100 Subject: [PATCH 3/5] DownloadIT now works for all build variants Signed-off-by: Philipp Hasper --- app/src/androidTest/java/com/owncloud/android/DownloadIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/com/owncloud/android/DownloadIT.java b/app/src/androidTest/java/com/owncloud/android/DownloadIT.java index d3099b251562..f8fd99517f1c 100644 --- a/app/src/androidTest/java/com/owncloud/android/DownloadIT.java +++ b/app/src/androidTest/java/com/owncloud/android/DownloadIT.java @@ -98,10 +98,10 @@ private void verifyDownload(OCFile file1, OCFile file2) { assertTrue(new File(file2.getStoragePath()).exists()); // test against hardcoded path to make sure that it is correct - assertEquals("/storage/emulated/0/Android/media/com.nextcloud.client/nextcloud/" + + assertEquals("/storage/emulated/0/Android/media/"+targetContext.getPackageName()+"/nextcloud/" + Uri.encode(account.name, "@") + "/testUpload/nonEmpty.txt", file1.getStoragePath()); - assertEquals("/storage/emulated/0/Android/media/com.nextcloud.client/nextcloud/" + + assertEquals("/storage/emulated/0/Android/media/"+targetContext.getPackageName()+"/nextcloud/" + Uri.encode(account.name, "@") + "/testUpload/nonEmpty2.txt", file2.getStoragePath()); } From db778afa366ec9c3005183bfb280087babd2545e Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Sun, 2 Nov 2025 18:58:43 +0100 Subject: [PATCH 4/5] Fixed AbstractIT deleting the wrong account The account type depends on the build flavor, as some of them define their own R.string.account_type. The test did respect that value when creating the account in AbstractIT.createAccount(), but not when deleting the account beforehand. Signed-off-by: Philipp Hasper --- app/src/androidTest/java/com/owncloud/android/AbstractIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/androidTest/java/com/owncloud/android/AbstractIT.java b/app/src/androidTest/java/com/owncloud/android/AbstractIT.java index 8e6c34db085c..0c213f846757 100644 --- a/app/src/androidTest/java/com/owncloud/android/AbstractIT.java +++ b/app/src/androidTest/java/com/owncloud/android/AbstractIT.java @@ -123,7 +123,7 @@ public static void beforeAll() { AccountManager platformAccountManager = AccountManager.get(targetContext); for (Account account : platformAccountManager.getAccounts()) { - if (account.type.equalsIgnoreCase("nextcloud")) { + if (account.type.equalsIgnoreCase(MainApp.getAccountType(targetContext))) { platformAccountManager.removeAccountExplicitly(account); } } From ba0751b68e0dc71c042d2e48beaf8036b79dac8f Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Sun, 2 Nov 2025 18:22:20 +0100 Subject: [PATCH 5/5] AbstractOnServerIT file deletion now ignores non-empty encrypted folders TODO: helper function to check mime type for folder should probably move to the RemoteFile class in the Nextcloud Library. Signed-off-by: Philipp Hasper --- .../com/owncloud/android/AbstractOnServerIT.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java b/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java index b20a26ff1cbd..194511b7041a 100644 --- a/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java +++ b/app/src/androidTest/java/com/owncloud/android/AbstractOnServerIT.java @@ -35,6 +35,7 @@ import com.owncloud.android.lib.resources.files.model.RemoteFile; import com.owncloud.android.operations.RefreshFolderOperation; import com.owncloud.android.operations.UploadFileOperation; +import com.owncloud.android.utils.MimeType; import org.apache.commons.httpclient.HttpStatus; import org.apache.commons.httpclient.methods.GetMethod; @@ -129,6 +130,11 @@ public void after() { super.after(); } + private static boolean isFolder(RemoteFile file) { + // TODO: should probably move to RemoteFile class + return MimeType.DIRECTORY.equals(file.getMimeType()) || MimeType.WEBDAV_FOLDER.equals(file.getMimeType()); + } + public static void deleteAllFilesOnServer() { RemoteOperationResult result = new ReadFolderRemoteOperation("/").execute(client); assertTrue(result.getLogMessage(), result.isSuccess()); @@ -146,6 +152,13 @@ public static void deleteAllFilesOnServer() { .execute(client) .isSuccess(); + if (!operationResult && isFolder(remoteFile)) { + // Deleting encrypted folder is not possible due to bug + // https://github.com/nextcloud/end_to_end_encryption/issues/421 + // Toggling encryption also fails, when the folder is not empty. So we ignore this folder + continue; + } + assertTrue(operationResult); }