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

Activity mock #6301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Activity mock #6301

wants to merge 1 commit into from

Conversation

tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Jun 16, 2020

Testing

Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.

unit tests
instrumented tests
UI tests

  • Tests written, or not not needed

ActivityIT first one
separate annotations to keep track and easier create screenshots

Signed-off-by: tobiasKaminsky <[email protected]>
Copy link
Collaborator

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
- Added 2
           

Complexity increasing per file
==============================
- src/main/java/com/nextcloud/client/di/ActivitiesModule.java  1
         

See the complete overview on Codacy

((retryCount++))
done

sed -i s'#<bool name="is_beta">true</bool>#<bool name="is_beta">false</bool>#'g src/main/res/values/setup.xml
Copy link
Collaborator

Choose a reason for hiding this comment

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


scripts/wait_for_emulator.sh

sed -i s'#<bool name="is_beta">false</bool>#<bool name="is_beta">true</bool>#'g src/main/res/values/setup.xml
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14494.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

stable-Unit test failed, but no output was generated. Maybe a preliminary stage failed.

@nextcloud-android-bot
Copy link
Collaborator

master-Unit test failed, but no output was generated. Maybe a preliminary stage failed.

@nextcloud-android-bot
Copy link
Collaborator

Codacy

397

Lint

TypemasterPR
Warnings9595
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings63
Internationalization Warnings9
Malicious code vulnerability Warnings4
Multithreaded correctness Warnings9
Performance Warnings75
Security Warnings44
Dodgy code Warnings142
Total373

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings63
Internationalization Warnings9
Malicious code vulnerability Warnings4
Multithreaded correctness Warnings9
Performance Warnings75
Security Warnings44
Dodgy code Warnings142
Total373

Copy link
Collaborator

@ezaquarii ezaquarii left a comment

Choose a reason for hiding this comment

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

If I understand it correctly, we have a specialized app to conduct screenshot tests, right?
And we want to swap network I/O layer to eliminate reliance on live server?

Swapping big compinents for tests is the right approach to do it. Ideally, we'd like to swap 1 component to achieve it, but with network ops spread between independent *Operation classes, doing it at HTTP client level seems to be a reasonable choice.

However, this will be really tedious and fragile to mock, as mock responses would have to track server changes. Of course this could be worked out as well with suitable refactoring, but the question about cost to benefit ratio must be answered first.

import static com.owncloud.android.ui.activity.ContactsPreferenceActivity.TAG;

public class LocalActivitiesRepository implements ActivitiesRepository {
String json = "{\n" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kotlin allows to use """ to denote raw strings that do not need any escaping. It's really handy for stuff like that and regexps.

@@ -119,12 +119,16 @@ public ActivityListAdapter(
this.storageManager = storageManager;
this.capability = capability;
this.clientFactory = clientFactory;
try {
this.client = clientFactory.createNextcloudClient(currentAccountProvider.getUser());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't that cause NPE somewhere down the line?


@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface ScreenshotWithServerTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it called with server?

@@ -240,6 +238,14 @@ protected void attachBaseContext(Context base) {
}
}

@VisibleForTesting
public void setupDagger() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make it protected.

@tobiasKaminsky
Copy link
Member Author

If I understand it correctly, we have a specialized app to conduct screenshot tests, right?
And we want to swap network I/O layer to eliminate reliance on live server?

Yes.

Swapping big compinents for tests is the right approach to do it. Ideally, we'd like to swap 1 component to achieve it, but with network ops spread between independent *Operation classes, doing it at HTTP client level seems to be a reasonable choice.

I have two approaches…

  • This one uses dagger as mocks one specific activitiesRepository.
  • Then there is Use interceptor to mock any network request #6299 which tries to use one generic interceptor to mock any server request.
  • another approach would be to have a lightweight (and probably stateless) http server which serves static responses.

My intention was to have a reproducable state, and ideally no external components (e.g. docker) needed (this is true for option 1/2).
Another benefit is that it is currently all handled in code, so there is no context switching for testing.

@tobiasKaminsky
Copy link
Member Author

However, this will be really tedious and fragile to mock, as mock responses would have to track server changes. Of course this could be worked out as well with suitable refactoring, but the question about cost to benefit ratio must be answered first.

True.
My "ideal" plan would look like this:

  • in library repo we test against stable/master
  • in files app we test against mocked server for most of the operations

So if there are api changes on server they would first be captured by library.

But as you said, I am really uncertain about time effort with this interceptor approach…
Maybe it is first a good start to have "only" #6301 (mocking some parts of the app for testing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants