Test CreateFolderOperation without server #3624

tobiasKaminsky · 2019-02-14T13:28:04Z

We have CreateFolderRemoteOperation which is in library and is (or should be) tested there.
Goal of this PR is to test CreateFolderOperation without really using CreateFolderRemoteOperation or any other RemoteOperation, so that this is all Unit Testing.

It is meant to be a start, where we can discuss.

Some consideration:

I could also have mocked FileDataStorageManager and test the class in a separate unit test
I commented Log_OC.d(…) as I have no idea how to mock this: maybe same approach like with PreferencesManager? But we use this way more often…
Tests for failure, multiple folder structure, etc. are missing
I used a method "wrapper" for CreateFolderRemoteOperation and ReadFolderRemoteOperation, maybe there is something easier?

@ezaquarii maybe you have some input in this.

Signed-off-by: tobiasKaminsky [email protected]

codecov · 2019-02-14T13:38:12Z

Codecov Report

Merging #3624 into master will decrease coverage by 12.66%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master   #3624       +/-   ##
============================================
- Coverage     18.71%   6.04%   -12.67%     
+ Complexity        3       1        -2     
============================================
  Files           392     315       -77     
  Lines         34273   30780     -3493     
  Branches       5011    4419      -592     
============================================
- Hits           6414    1862     -4552     
- Misses        26863   28640     +1777     
+ Partials        996     278      -718

Impacted Files	Coverage Δ	Complexity Δ
...a/com/owncloud/android/utils/FileStorageUtils.java	`11.03% <ø> (-15.76%)`	`0 <0> (ø)`
...loud/android/datamodel/FileDataStorageManager.java	`10.52% <ø> (-38.5%)`	`0 <0> (ø)`
...cloud/android/operations/common/SyncOperation.java	`25% <0%> (-12.5%)`	`0 <0> (ø)`
...android/operations/SynchronizeFolderOperation.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...loud/android/datamodel/ThumbnailsCacheManager.java	`15.08% <100%> (-18.13%)`	`0 <0> (ø)`
...in/java/com/owncloud/android/datamodel/OCFile.java	`58.06% <100%> (-8.31%)`	`0 <0> (ø)`
...loud/android/operations/CreateFolderOperation.java	`74.41% <100%> (ø)`	`0 <0> (ø)`	⬇️
...va/com/owncloud/android/utils/OwnCloudSession.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
...ncloud/android/ui/adapter/FeaturesViewAdapter.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
...com/owncloud/android/ui/events/TokenPushEvent.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
... and 310 more

AndyScherzinger · 2019-02-14T13:55:56Z

Looks good code-wise on an cellphone.
As for the logger, well I would expect a logger to be usable in a static way, so I wouldn't go for the same approach as for the preference manager. Maybe we could have a method injecting the writer within the logger class so that for testing it writes to something like a list or simply doesn't write or store anything anywhere.

tobiasKaminsky · 2019-02-14T14:26:03Z

Maybe we could have a method injecting the writer within the logger class so that for testing it writes to something like a list or simply doesn't write or store anything anywhere.

Right, like with FileDataStorageManagerInterface, we could have Log_OCInterface…
But is this the best approach…

ezaquarii

Injecting Logger as a dependency is an elegant solution, but at a cost of convenience and there is apparently no appetite for this.

That is understandable - printf() is so nice to use.

Given the constraints, I'd extract logger implementation into a class (LogFileWriter, LogNullWriter, LogArrayWriter, etc, etc), and delegate all calls in Log_OC to specific runtime version.

You can swap runtime version with a static setter at any time, let's say in test harness setup.

Pseudocode. I skipped thread safety for the sake of illustration. Also, pls note that you can either inject a specific logger instance to anything and/or use a static, convenient Log_OC interface.

interface LoggerInterface {
   void d(...)
   etc, etc
}

class FileLogger implements LoggerInterface {
    // move existing file-based logger logic from Log_OC here
}

class NullLogger implements LoggerInterface {
   // do nothing :)
}

class MemoryLogger implements LoggerInterface {
   // write logs to ListArray
}

/**
 * BTW, Does it still need _OC suffix?
 */
public class Log_OC {
    private static LoggerInterface sLogger;

    /**
     * You can probably move this to a separate class, and use package-scope to remove
     * init() from public Log_OC API.
     */
    public static void init(LoggerInterface logger) {
         sLogger = logger;
    }

    /**
     * Another variant is package-scoped. Let's imagine that this API
     * is available via some LoggerControl or LoggerManager API that
     * is in the same pkg
     */
    static void init(LoggerInterface logger) {
         sLogger = logger;
    }

    public static void d(Object object, String message) {
         sLogger.d(object, message);
    }
    ...
}

/**
 * You can get fancy with that and make yourself
 * a nice settings panel to fiddle with logger at runtime.
 */
class LoggerManager {
    public static init(LoggerInterface ....);
    public static setLogLevel(...);
    public static setSomeOtherFancyLogOption(...)
}

ezaquarii · 2019-02-14T21:19:52Z

src/main/java/com/owncloud/android/operations/CreateFolderOperation.java

@@ -76,6 +76,14 @@ protected RemoteOperationResult run(OwnCloudClient client) {
        return result;
    }

+    public CreateFolderRemoteOperation createCreateFolderRemoteOperation(String mRemotePath, boolean mCreateFullPath) {


Those m prefixes are probably a left-overs after either refactoring or automated generation? It can be trimmed with Shift-F6 in no time.

ezaquarii · 2019-02-14T21:32:24Z

src/test/java/com/owncloud/android/FileDataStorageManagerLocal.java

+
+    @Override
+    public boolean fileExists(long id) {
+        throw new IllegalArgumentException("to implement");


How about UnsupportedOperationException?

tobiasKaminsky · 2019-02-15T10:27:01Z

As this seems to be the right way, thanks @ezaquarii, I'll do three PRs:

create FileDataStorageManagerInterface
create Log Interface
create Test PR based on both above

So this clean and easy to review.

AndyScherzinger · 2019-02-15T10:41:27Z

So @ezaquarii and @tobiasKaminsky I'd like to start the interface naming discussion :D
My vote would be to no prefix or suffix the interface names. Usually we probably always work with the interface so the code is easier to read and less verbose. As for implementing classes I would even suggest the same thing as in to choose a proper name for the class which hints to the class' intent and implementation case. In edge cases where we can't find a name for the class we then could add an Impl suffix but rather as some kind of a last resort.

AndyScherzinger · 2019-02-15T10:43:59Z

Example

FileDataStorageManager (Inferface),
FileDataStorageManagerLocal (for Test) and
FileDataStorageManagerDB (for SQLite/App)

ezaquarii · 2019-02-15T15:58:48Z

Example

* FileDataStorageManager (Inferface),

* FileDataStorageManagerLocal (for Test) and

* FileDataStorageManagerDB (for SQLite/App)

It's not a bad practice to optimize for most frequent expected use case:

there is an interface (obviously)
there is only 1 default implementation
there is at most 1 mock

It boils down to choosing between 2 industry wide conventions that happen to work:

IFoo + Foo + FooTest + some Runnable-style exceptions
Foo + FooImpl + FooImplTest/FooTest + (matches default Runnable style, but has Impl)

Each one of them has some corner aesthetic issues, philosophical foundations, fans and haters. I have my opinion of course, but it's not important as long as:

you are not intrusive
you are consistent across the stack.

I believe Android picked Foo-FooImpl variant.

And if some day you find yourself on the wrong side of the flame war: Shift+F6. ;)

AndyScherzinger · 2019-02-15T16:55:12Z

Well, in that case I vote for variant 2 proposed by @ezaquarii since that should fit the most frequent case (just my opinion, no proof) and as stated would also be the typical Android naming schema pick.

ezaquarii · 2019-03-30T21:14:42Z

Is it going to be merged? Still WIP?

tobiasKaminsky · 2019-04-02T10:14:32Z

Is it going to be merged? Still WIP?

Yes, I just need to find a bit more time… :-)

nextcloud-android-bot · 2019-05-14T11:38:40Z

Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/9180

nextcloud-android-bot · 2019-05-30T20:16:08Z

Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/9575

nextcloud-android-bot · 2019-12-19T07:50:31Z

Codacy

329

Lint

Type	master	PR
Warnings	73	73
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	72
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	107
Security Warnings	44
Dodgy code Warnings	137
Total	413

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	70
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	106
Security Warnings	44
Dodgy code Warnings	136
Total	409

SpotBugs increased!

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

nextcloud-android-bot · 2020-01-10T19:23:44Z

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

Issues
======
+ Solved 27
- Added 11
           

Complexity increasing per file
==============================
- src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java  21

See the complete overview on Codacy

nextcloud-android-bot · 2020-01-10T19:23:53Z

src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java

        for (Account account : accountManager.getAccounts()) {
-            final FileDataStorageManager storageManager = new FileDataStorageManager(account, contentResolver);
+            final FileDataStorageManager storageManager = new FileDataStorageManagerImpl(account, getContext());


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:23:54Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+    public void copyLocalFile(OCFile file, String targetPath) {
+
+        if (file != null && file.fileExists() && !OCFile.ROOT_PATH.equals(file.getFileName())) {
+            String localPath = FileStorageUtils.getDefaultSavePathFor(account.name, file);


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:23:55Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+                );
+            } catch (RemoteException e) {
+                Log_OC.e(TAG, e.getMessage(), e);
+                return ret;


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-10T19:23:56Z

src/main/java/com/owncloud/android/jobs/OfflineSyncJob.java

@@ -92,8 +93,7 @@ protected Result onRunJob(@NonNull Params params) {
            Account[] accounts = userAccountManager.getAccounts();

            for (Account account : accounts) {
-                FileDataStorageManager storageManager = new FileDataStorageManager(account,
-                        getContext().getContentResolver());
+                FileDataStorageManager storageManager = new FileDataStorageManagerImpl(account, getContext());


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:23:57Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+ *
+ * Copyright (C) 2012  Bartek Przybylski
+ * Copyright (C) 2015 ownCloud Inc.
+ *


Issue found: Avoid really long classes.

nextcloud-android-bot · 2020-01-10T19:23:58Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+        cv.put(
+            ProviderTableMeta.FILE_MODIFIED_AT_LAST_SYNC_FOR_DATA,
+            file.getModificationTimestampAtLastSyncForData()
+        );


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:23:59Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+                ContentValues cv = new ContentValues();
+                fileId[0] = String.valueOf(cursor.getLong(cursor.getColumnIndex(ProviderTableMeta._ID)));
+                String oldFileStoragePath =
+                    cursor.getString(cursor.getColumnIndex(ProviderTableMeta.FILE_STORAGE_PATH));


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-10T19:24:01Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+                    result_uri = getContentProviderClient().insert(ProviderTableMeta.CONTENT_URI_SHARE, cv);
+                } catch (RemoteException e) {
+                    Log_OC.e(TAG, FAILED_TO_INSERT_MSG + e.getMessage(), e);
+                }


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:24:02Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+    /**
+     * Retrieves an stored {@link OCShare} given its id.
+     *
+     * @param id Identifier.


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:24:03Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+
+                    }
+                    if (child.getRemotePath().equals(file.getRemotePath())) {
+                        cv.put(ProviderTableMeta.FILE_PARENT, targetParent.getFileId());


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-10T19:24:05Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManagerImpl.java

+        }
+        cursor.close();
+
+        /// 3. apply updates in batch


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-10T21:02:06Z

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

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 · 2020-01-10T21:15:37Z

Codacy

340

Lint

Type	master	PR
Warnings	76	76
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	73
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	108
Security Warnings	44
Dodgy code Warnings	140
Total	418

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	71
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	107
Security Warnings	44
Dodgy code Warnings	139
Total	414

SpotBugs increased!

tobiasKaminsky added the 2. developing label Feb 14, 2019

ezaquarii reviewed Feb 14, 2019

View reviewed changes

tobiasKaminsky force-pushed the testWithoutServer branch from 76dd4e2 to 41fe0bf Compare April 17, 2019 11:13

nextcloud deleted a comment Apr 17, 2019

tobiasKaminsky force-pushed the testWithoutServer branch from 41fe0bf to 3a7ce0d Compare May 14, 2019 11:35

nextcloud deleted a comment May 14, 2019

AndyScherzinger force-pushed the testWithoutServer branch from 3a7ce0d to 88626e6 Compare May 30, 2019 20:07

nextcloud deleted a comment May 30, 2019

tobiasKaminsky force-pushed the testWithoutServer branch from 88626e6 to 85b6b2c Compare December 12, 2019 14:20

nextcloud deleted a comment Dec 12, 2019

tobiasKaminsky force-pushed the testWithoutServer branch from b0f93b6 to d0ea6ee Compare December 19, 2019 07:28

tobiasKaminsky added 3 commits January 10, 2020 14:35

test CreateFolderOperation without server

ebf22f6

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

update tests

6da3a07

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

fix during CI

1e2cbff

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

tobiasKaminsky added 3 commits January 10, 2020 14:35

remove annotation

688a44d

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

Fix unittests

aaba0a8

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

wip

fb969cc

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

tobiasKaminsky force-pushed the testWithoutServer branch from d0ea6ee to fb969cc Compare January 10, 2020 19:20

nextcloud-android-bot reviewed Jan 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test CreateFolderOperation without server #3624

Test CreateFolderOperation without server #3624

tobiasKaminsky commented Feb 14, 2019

codecov bot commented Feb 14, 2019 •

edited

Loading

AndyScherzinger commented Feb 14, 2019

tobiasKaminsky commented Feb 14, 2019

ezaquarii left a comment •

edited

Loading

ezaquarii Feb 14, 2019

ezaquarii Feb 14, 2019

tobiasKaminsky commented Feb 15, 2019

AndyScherzinger commented Feb 15, 2019

AndyScherzinger commented Feb 15, 2019

ezaquarii commented Feb 15, 2019 •

edited

Loading

AndyScherzinger commented Feb 15, 2019 •

edited

Loading

ezaquarii commented Mar 30, 2019

tobiasKaminsky commented Apr 2, 2019

nextcloud-android-bot commented May 14, 2019

nextcloud-android-bot commented May 30, 2019

nextcloud-android-bot commented Dec 19, 2019

nextcloud-android-bot commented Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot Jan 10, 2020

nextcloud-android-bot commented Jan 10, 2020

nextcloud-android-bot commented Jan 10, 2020

Test CreateFolderOperation without server #3624

Are you sure you want to change the base?

Test CreateFolderOperation without server #3624

Conversation

tobiasKaminsky commented Feb 14, 2019

codecov bot commented Feb 14, 2019 • edited Loading

Codecov Report

AndyScherzinger commented Feb 14, 2019

tobiasKaminsky commented Feb 14, 2019

ezaquarii left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasKaminsky commented Feb 15, 2019

AndyScherzinger commented Feb 15, 2019

AndyScherzinger commented Feb 15, 2019

ezaquarii commented Feb 15, 2019 • edited Loading

AndyScherzinger commented Feb 15, 2019 • edited Loading

ezaquarii commented Mar 30, 2019

tobiasKaminsky commented Apr 2, 2019

nextcloud-android-bot commented May 14, 2019

nextcloud-android-bot commented May 30, 2019

nextcloud-android-bot commented Dec 19, 2019

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

SpotBugs increased!

nextcloud-android-bot commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-android-bot commented Jan 10, 2020

nextcloud-android-bot commented Jan 10, 2020

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

SpotBugs increased!

codecov bot commented Feb 14, 2019 •

edited

Loading

ezaquarii left a comment •

edited

Loading

ezaquarii commented Feb 15, 2019 •

edited

Loading

AndyScherzinger commented Feb 15, 2019 •

edited

Loading