Minor improvements for automated testing #15424
Conversation
PhilLab
changed the title
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
PhilLab
force-pushed
the
automated_testing_minor_improvements
branch
2 times, most recently
from
14b716d to
38fa3da
Compare
PhilLab
force-pushed
the
automated_testing_minor_improvements
branch
from
38fa3da to
5096dea
Compare
| 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/" + |
There was a problem hiding this comment.
Is there a reason to change that?
We test "only" our official Nextcloud client.
There was a problem hiding this comment.
@tobiasKaminsky Two reasons:
- The issue is that when you switch between test development and app debugging (to fix the found issues), the tests always deletes the data and when starting the app you need to login again. This costs me 2-3 minutes every time and is quite annoying.
A good workaround for this is to start the tests under one build variant, and debug the app under another build variant. But for this to work, it needs to be respected everywhere, otherwise assertions will fail or even the wrong data be deleted. This is the common background of the two commitsFixed AbstractIT deleting the wrong accountandDownloadIT now works for all build variants - Also conceptually it is clearer that instead of the magic string
com.nextcloud.clientit refers to what is semantically needed here - and that is the packageName. Sure, following that paradigm, the assertion should make use of even more functions (e.g. to retrieve the/media/path programmatically instead of hard-coded). But I didn't have an actual use case to justify that. Only the use case from#1.
There was a problem hiding this comment.
@tobiasKaminsky with that explanation - is this fine for you?
PhilLab
force-pushed
the
automated_testing_minor_improvements
branch
from
5096dea to
ba0751b
Compare
PhilLab
force-pushed
the
automated_testing_minor_improvements
branch
from
ba0751b to
d5aa82a
Compare
|
@PhilLab the CI does not work on forked repos. |
| * - TEST_SERVER_URL | ||
| * - TEST_SERVER_USERNAME | ||
| * - TEST_SERVER_PASSWORD | ||
| * These are supplied via build.gradle, which takes them from gradle.properties. |
There was a problem hiding this comment.
@alperozturk96 @tobiasKaminsky I just noticed that the test setup is broken on master. The values in gradle.properties are ignored now, even though there are still default values to be filled in there:
NC_TEST_SERVER_BASEURL=http://server
NC_TEST_SERVER_USERNAME=test
NC_TEST_SERVER_PASSWORD=test
I just tried to run tests but couldn't reach my server until I noticed that it is not using my custom URL and credentials.
I am not fully clear how this happened, but this is what I found so far:
- 247d085 introduced an alternative parsing of the same values, but from .gradle/config.properties.
- later, 5fd2e29 came and changed that
testInstrumentationRunnerArgumentwasn't taking these values from the gradle.properties but instead from the otherwise unused config.properties.
This is not the first weird side-effect of that Gradle rewrite PR 🤔 #15859 .
It is very confusing. We need to clean up two things
- We should only have one way of passing these values to the code. Either as test argument or as BuildConfig.
- To be honest, storing the password in the BuildConfig, even if it is only the debug config, seems very risky to me. This can easily be overlooked and cause unintended side-effects. E.g. testing .apks you are sharing with others will include these credentials.
- Hence, I would suggest to remove them from the BuildConfig again
- We should only parse these values from one file.
.gradle/config.propertiesorgradle.properties- I would suggest
gradle.properties. This is how it always has been and it seems to be common practice.
- I would suggest
We can also fix this in a new PR, as it is more urgent and independent of the documentation update here.
There was a problem hiding this comment.
In my local stash, I have replaced this line https://github.com/nextcloud/android/blob/master/app/build.gradle.kts#L78 with:
val file = rootProject.file("gradle.properties")
shall I add this to the PR here, or will you open a new one with more consequential changes?
There was a problem hiding this comment.
@tobiasKaminsky @alperozturk96 I added another commit to change this to
val file = rootProject.file("gradle.properties")
because my local testing was blocked by this. Can we merge? But I do think that it needs a clean up, as eluded to in my message above.
@tobiasKaminsky regarding this, I sent an email to the address mentioned in the Github profile (not the one from your commits). |
- 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 <vcs@hasper.info>
…sion 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 <vcs@hasper.info>
Signed-off-by: Philipp Hasper <vcs@hasper.info>
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 <vcs@hasper.info>
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 <vcs@hasper.info>
PhilLab
force-pushed
the
automated_testing_minor_improvements
branch
from
d5aa82a to
04138d5
Compare
- UserAccountManagerImpl#getAccountByName() is never null because since nextcloud#13074 it rather returns an anonymous account. To detect an account lookup failure, the type needs to be compared - The getMaterialSchemesProvider() object returned null for most functions, even though they were annotated with @nonnull. Extracted the only actually used function getMaterialSchemesForCurrentUser() Signed-off-by: Philipp Hasper <vcs@hasper.info>
Prior, tests couldn't reach the configured server, because the custom URL and credentials are simply ignored. 247d085 introduced an alternative parsing of the same values, but from .gradle/config.properties. Later, 5fd2e29 came and changed that testInstrumentationRunnerArgument wasn't taking these values from the gradle.properties but instead from the otherwise unused config.properties. Signed-off-by: Philipp Hasper <vcs@hasper.info>
3 participants
Uh oh!
There was an error while loading. Please reload this page.