[New arch] Modularization and mockk #2702
Conversation
davigonz
force-pushed
the
new_arch/modularization_and_mockk
branch
from
970397b to
bef38d0
Compare
davigonz
changed the title
davigonz
force-pushed
the
new_arch/modularization_and_mockk
branch
from
bef38d0 to
d590027
Compare
build.gradle
Outdated
|
|
||
| // Testing | ||
| junitVersion = "4.12" | ||
| mockitoVersion = "2.24.0" | ||
|
|
||
| // Extensions | ||
| ktxVersion = "1.1.0" | ||
| ktxVersion = "1.0.2" |
There was a problem hiding this comment.
It was during the rebase with master, fixing it
| @@ -25,7 +28,7 @@ dependencies { | |||
| // Dependencies for unit tests | |||
| testImplementation "junit:junit:$junitVersion" | |||
| testImplementation "androidx.arch.core:core-testing:$archLifecycleVersion" | |||
| testImplementation "org.mockito:mockito-core:$mockitoVersion" | |||
| testImplementation "io.mockk:mockk:$mockkVersion" | |||
|
|
||
| private lateinit var file: OCFile | ||
|
|
||
| private val publicShares = arrayListOf( |
There was a problem hiding this comment.
why do you use arrayList?? It's always better to interfaces, in this case List ;-)
| private lateinit var file: OCFile | ||
|
|
||
| private val publicShares = arrayListOf( | ||
| AppTestUtil.createPublicShare( |
There was a problem hiding this comment.
Why don't you create a DUMMY_PUBLIC_SHARE and create new instances with copy function??
| } | ||
|
|
||
| private fun loadCapabilitiesSuccessfully( | ||
| capability: OCCapabilityEntity = AppTestUtil.createCapability( |
|
|
||
| private fun savePublicShare(newShare: OCShareEntity, resource: DataResult<Unit> = DataResult.success()) { | ||
|
|
||
| every { |
There was a problem hiding this comment.
in this function, you are mixing mocking with performing a click action.... Please, divide it
| private val account = Account("admin", "owncloud") | ||
|
|
||
| @BeforeClass | ||
| @JvmStatic | ||
| fun init() { | ||
| addAccount() | ||
| } | ||
|
|
||
| @AfterClass | ||
| @JvmStatic | ||
| fun cleanUp() { | ||
| AccountsManager.deleteAllAccounts(targetContext) | ||
| } | ||
|
|
||
| private fun addAccount() { | ||
| // obtaining an AccountManager instance | ||
| val accountManager = AccountManager.get(targetContext) | ||
|
|
||
| accountManager.addAccountExplicitly(account, "a", null) | ||
|
|
There was a problem hiding this comment.
Why don't you create a class to be extended for instrumented tests with these functions?? I have seen this code repeated several times....
| onView(withText(R.string.unshare_link_file_error)).check(matches(isDisplayed())) | ||
| } | ||
|
|
||
| private fun getOCFileForTesting(name: String = "default"): OCFile { |
There was a problem hiding this comment.
this function is also repeated several times. Can you create a DUMMY_FILE?
| } | ||
|
|
||
| private fun loadCapabilitiesSuccessfully( | ||
| capability: OCCapabilityEntity = AppTestUtil.createCapability( |
| AppTestUtil.createPublicShare( // With permission Download/View/Upload | ||
| path = "/Photos/", | ||
| expirationDate = 0L, | ||
| permissions = 15, |
There was a problem hiding this comment.
permissions 15?? Use constants please.
Apply for all files
| every { ocShareViewModel.getPublicShares(file.remotePath) } returns sharesLiveData | ||
| every { ocShareViewModel.getPrivateShares(file.remotePath) } returns MutableLiveData() | ||
|
|
||
| stopKoin() |
There was a problem hiding this comment.
you should call stopKoin in @after method
| OCShareViewModel(androidContext(), account) | ||
| val viewModelModule = module { | ||
| viewModel { (filePath: String, account: Account) -> | ||
| OCShareViewModel( |
There was a problem hiding this comment.
It's a pretty old and strong dependency I need to create ownCloud Clients and execute network operations. I already fixed it using dependency injection in coming PRs.
| if (getContentResolver() != null) { | ||
| getContentResolver().update(ProviderTableMeta.CONTENT_URI, cv, where, whereArgs); | ||
|
|
||
| } else { | ||
| try { | ||
| getContentProviderClient().update(ProviderTableMeta.CONTENT_URI, cv, where, | ||
| whereArgs); | ||
| } catch (RemoteException e) { | ||
| Log_OC.e(TAG, "Exception in resetShareFlagsInFolder " + e.getMessage()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
these lines have been repeated several times differing with just one aegument. Can you simplify it?
There was a problem hiding this comment.
You mean resetShareFlagsInAllFiles, resetShareFlagsInFolder, resetShareFlagInAFile and cleanShares methods? They have more than one different argument when calling getContentResolver() or getContentProviderClient(). Anyway, I removed those 4 methods since are no longer used.
| } | ||
|
|
||
| fun isLoading(): Boolean = (status == Status.LOADING) | ||
| fun isSuccess(): Boolean = (status == Status.SUCCESS) |
There was a problem hiding this comment.
why don't you create a function isError??
| @@ -146,96 +143,96 @@ class UsersAndGroupsSearchProvider : ContentProvider() { | |||
| userQuery, | |||
| REQUESTED_PAGE, | |||
| RESULTS_PER_PAGE | |||
| ) | |||
| ).also { uiResult -> | |||
| if (!uiResult.isSuccess() && !uiResult.isLoading()) { | |||
| val names = uiResult.data | ||
|
|
||
| // convert the responses from the OC server to the expected format | ||
| if (names?.size!! > 0) { |
There was a problem hiding this comment.
use !names.isNullOrEmpty() instead
|
|
||
| val manager = FileDataStorageManager( | ||
| context, | ||
| account, context!!.contentResolver |
There was a problem hiding this comment.
if you are formatting file, do it well
There was a problem hiding this comment.
After this, I can not trust in the AS reformat code option anymore. The whole file was properly formatted except this line. 😄
| .isTrue | ||
|
|
||
| try { | ||
| while (namesIt.hasNext()) { |
There was a problem hiding this comment.
I think this file has been translated by Android Studio.
Why don't you use something like a for (name in names)
| } | ||
|
|
||
| public override fun onStop() { | ||
| super.onStop() |
There was a problem hiding this comment.
you can remove this function, can't you?
There was a problem hiding this comment.
Sure though AS doesn't complain about it
| when (uiResult?.status) { | ||
| Status.SUCCESS -> { | ||
| if (publicShareFragment != null) { | ||
| publicShareFragment?.updateCapabilities(uiResult.data) |
There was a problem hiding this comment.
why don't you a shared view model instead of keeping references to fragments?? That's the best way to do it
There was a problem hiding this comment.
I fix this in coming PRs
| } | ||
| } | ||
| else -> { | ||
| Log.d(TAG, "Unknown status when loading capabilities in account ${account?.name}") |
| file.isSharedWithMe -> return RemoteShare.READ_PERMISSION_FLAG // minimum permissions | ||
| isFederated -> { | ||
| val serverVersion = com.owncloud.android.authentication.AccountUtils.getServerVersion(account) | ||
| return if (serverVersion != null && serverVersion.isNotReshareableFederatedSupported) { |
There was a problem hiding this comment.
Even with two conditions? I rather use if else for those cases.
| Status.LOADING -> { | ||
| showLoadingDialog(R.string.common_loading) | ||
| } | ||
| else -> { |
There was a problem hiding this comment.
nothing to do with success case??
There was a problem hiding this comment.
No, I'm observing the database shares from observeShares() function of ShareFileFragment so if everything was fine, they are automatically updated from there.
| } | ||
|
|
||
| override fun showEditPrivateShare(share: OCShareEntity) { | ||
| val ft = supportFragmentManager.beginTransaction() |
There was a problem hiding this comment.
try with ktx ;-)
There was a problem hiding this comment.
I've managed to use it when using replace() but not show() as we have to do in this function
| @@ -101,7 +107,7 @@ class ShareFileFragment : Fragment(), ShareUserListAdapter.ShareUserAdapterListe | |||
| /** | |||
| * List of public links bound to the file | |||
| */ | |||
| private var publicLinks: ArrayList<OCShare>? = null | |||
| private var publicLinks: ArrayList<OCShareEntity>? = null | |||
There was a problem hiding this comment.
as I said before, thinking in ArrayList instead of List is not best, use interfaces instead
| } | ||
|
|
||
| init { | ||
| capabilitiesLiveData?.observeForever(capabilitiesObserver) |
There was a problem hiding this comment.
really??? observing forever??
Why don't use a _capabilities as MediatorLiveData, adding source capabilitiesLiveData ???
There was a problem hiding this comment.
MediatorLiveData is meant to merge emissions of changes from different LiveDatas in one object. Here we have just one LiveData to observe.
If _capabilities is a MediatorLiveData and we add capabilitiesLiveData as unique source, we are doing basically the same as I wrote, since MediatorLiveData uses observeForever and removeObserver under the hood, as you can see below.
There was a problem hiding this comment.
For me, it's better to use MediatorLiveData because you don't need to override onCleared to remove the observer. It's automatically done
There was a problem hiding this comment.
Ok, I changed it and is cleaner but let's see if I do not need to use removeSource() to make it work in the future. 😄
… when using it only in ShareActivity
…ding ShareFileFragment or PublicShareDialogFragment
davigonz
force-pushed
the
new_arch/modularization_and_mockk
branch
from
03850e5 to
fe18043
Compare
|
@jesmrec code approved, ready to QA. Please take into account that the tests are not going to pass here, they will be fixed in coming PRs. It was the only way to start merging the modularization without having just one and very huge PR. |
davigonz
changed the title
|
App build and correctly installed. Approved. |
davigonz
changed the title
delete-merged-branch
bot
deleted the
new_arch/modularization_and_mockk
branch
1st PR for #2607 , changes are not final, coming PRs will bring more.
Implements:
Split the project in different modules: #2696
Switch from Mockito to Mockk: #2697
Create new use cases for sharing in domain module: #2698