Migrated ZimHostFragment -> ZimHostRoute

[atharvyadav22]

Fixes: #4615

[atharvyadav22]

@MohitMaliFtechiz
Sorry for the delay and not able to contribute in other Prs i was prioritizing this migration it was difficult because of many classes which required understanding.
This is an initial commit ill later add test cases too for now please review the logic.

I have added comments to help other read the code, also from previous migration i have implemented your points so it saves time.

Migration covered these parts-

• Hotspot now use composable lifecycle
• Merged Di and common fragment files and updated in ViewModel itself
• Handled Playstore and custom apps logic
• Permission block kept the same
• Instead of suppressing i made small code blocks.
• for the checkbox i modified the logic cause it wasn't updating its state with toggleSelection.

[atharvyadav22]

@MohitMaliFtechiz Done with this migration waiting for your reviews.

[MohitMaliFtechiz]

@atharvyadav22 Please see the review changes. If anything is unclear, please ask me before implementing it. Since this PR has many logic changes. It takes significant time in testing/reviewing PRs like this one.

[atharvyadav22]

@atharvyadav22 Please see the review changes. If anything is unclear, please ask me before implementing it. Since this PR has many logic changes. It takes significant time in testing/reviewing PRs like this one.

Yes thanks for your valuable time and pointing out i always try to not many many possible errors and solve then push commits but migration is lengthy anyways i'll take care of in in next commit i'll solve everything you mentioned so it won't take your time.
Thanks for detailed review ❤️

[MohitMaliFtechiz]

@atharvyadav22 Mark your PR as a draft when the effort is ongoing.

[atharvyadav22]

@MohitMaliFtechiz i'll prioritize this one issue now.

[atharvyadav22]

@MohitMaliFtechiz i have checked the code and functionality it works fine for all the cases.
Coming up to test cases there are concerning issues lie.

1. flakyTest{} wrapper
2. testFlow should be replaced with test{} (Kotlin has this)
3. Turbine should be closed

@Suppress("InjectDispatcher")
  private var ioDispatcher: CoroutineDispatcher = Dispatchers.IO

this should be passed via Constructor, also for my PR of stabilization IO i didn't touched this as this is little complex and as migration was going.

similarly for this

  fun sendUiEvent(uiEvent: OnlineLibraryUiEvent) =
    viewModelScope.launch {
      _onlineLibraryEvent.emit(uiEvent)
    }

we cant control this in tests

so to properly make an robust test we need to consider this after migration properly i suppose, cause we have seen many issues with these in past also.

[MohitMaliFtechiz]

@atharvyadav22 Now, this PR code is set. You can start writing the test cases(unit test cases) for ZimHostViewModel and other things introduced in this PR. If any UI test case fails, please have a look at that as well :)

If you need anything from me, just drop a comment here.

[atharvyadav22]

@MohitMaliFtechiz okay ill do it here i see new pushes
Thanks for your help 👍

[MohitMaliFtechiz]

@atharvyadav22 For sendUiEvent method. I have added a comment on that method. This will be updated in the OnlineLibraryScreen migration.

Here we need to add/update/improve the test cases for this PR. Not for other screens. Since ZimManageViewModel is very large class and handles 2 screen datas. We need to first split them into seperate viewModels, and then improve those test cases.

[atharvyadav22]

@atharvyadav22 For sendUiEvent method. I have added a comment on that method. This will be updated in the OnlineLibraryScreen migration.

Here we need to add/update/improve the test cases for this PR. Not for other screens. Since ZimManageViewModel is very large class and handles 2 screen datas. We need to first split them into seperate viewModels, and then improve those test cases.

new tests file for newly created methods right only or any specific things?

As we are in urgent for migration for that OnlineLibraryScreen i see no progress as that PR has many issues including many CI failure, if you allow i can take that : ) since my migration related tasks are almost completed.
between 1-1.5 week i can complete that migration.
Thanks.

[MohitMaliFtechiz]

new tests file for newly created methods right only or any specific things?

The newly created files with edge cases.

As we are in urgent for migration for that OnlineLibraryScreen i see no progress as that PR has many issues including many CI failure, if you allow i can take that : ) since my migration related tasks are almost completed.
between 1-1.5 week i can complete that migration.
Thanks.

Thank you for showing interest. But OnlineScreen has many logics and scenarios. I will take that. You can take some other issues :).

[atharvyadav22]

new tests file for newly created methods right only or any specific things?

The newly created files with edge cases.

As we are in urgent for migration for that OnlineLibraryScreen i see no progress as that PR has many issues including many CI failure, if you allow i can take that : ) since my migration related tasks are almost completed.
between 1-1.5 week i can complete that migration.
Thanks.

Thank you for showing interest. But OnlineScreen has many logics and scenarios. I will take that. You can take some other issues :).

Okay if there is anything which might help kiwix and save your time i can help with that if something you feel.
Thanks.

[MohitMaliFtechiz]

@atharvyadav22 Any update here?

[atharvyadav22]

@atharvyadav22 Any update here?

Yes actually i am doing coverage also side by side for this. I am currently doing that Media Save PR as the recent test files fails for new code so its taking me time to rewrite them along side i am doing that UnsupportedMimeType as you said so.

I'll do this PR by this week itself, actually i am using best practice and trying to make test robust so it saves your reviewing time to so its taking me time.
Thanks.

[atharvyadav22]

@MohitMaliFtechiz
This is only for Unit Test cases but inorder to make an robust migration i'll also make Instrument Test like i have started working on it should i go on ?

[MohitMaliFtechiz]

@atharvyadav22 Yes, we should add both instrumentation and unit test cases. However, the UI test cases are already exist. Those are the scenarios based. But you can add the compose UI test cases as well. So that we can verify the UI as well.

[atharvyadav22]

@MohitMaliFtechiz
actually the Current UI Test is not that good reason is we need to mimic actual behavior of the Screen like server starting, etc.

Now interesting thing is this can't work on CI as emulators dont have these WIFI but if we test on real device rather it should work i came up with idea like we'll check about WIFI to know whether we are testing in emulator or real devices also will be placing a fix to run for all the devices API 27+ (current has upto 32 i suppose)

I'll need time for such things as for an robust migration.

Thanks.

[MohitMaliFtechiz]

@atharvyadav22 Previously, we had the fragments, and we could not test the functionality directly within a fragment for real scenarios. But now we have migrated to compose with view models.

So now we can test layer by layer to ensure all the functionality works correctly.

• First layer should be the viewModel testing, where all the logic is written. By doing this, we can ensure how this functionality will behave on an actual device.
• The second layer is the UI layer. A pure compose UI testing, so that we can ensure which data come from the viewModel is rendered correctly and shows the correct UI. Also, when clicks are performed, they should call the correct viewModel methods(ViewModel testing is already done).
• The third one is a pure real-scenario test that is already written. Due to the unavailability of WIFI on emualtor it is currently running on the API level 30, which is okay because we have tested both UI and logic above the testing layer.

By doing this, we don't need to add additional testing and conditions based on emualtor or real device.

[atharvyadav22]

@atharvyadav22 Previously, we had the fragments, and we could not test the functionality directly within a fragment for real scenarios. But now we have migrated to compose with view models.

So now we can test layer by layer to ensure all the functionality works correctly.

• First layer should be the viewModel testing, where all the logic is written. By doing this, we can ensure how this functionality will behave on an actual device.
• The second layer is the UI layer. A pure compose UI testing, so that we can ensure which data come from the viewModel is rendered correctly and shows the correct UI. Also, when clicks are performed, they should call the correct viewModel methods(ViewModel testing is already done).
• The third one is a pure real-scenario test that is already written. Due to the unavailability of WIFI on emualtor it is currently running on the API level 30, which is okay because we have tested both UI and logic above the testing layer.

By doing this, we don't need to add additional testing and conditions based on emualtor or real device.

Okay actually what i was working was if someone tests on real devices they will see that navigation, clicks eg- Server is started, books selection toggle.
Keeping CI safe.

I also agree with your point so should i continue with that work or only fix that CI errors?

[MohitMaliFtechiz]

We should add the testing as I described in layers. That will already ensure that everthing works correctly. Devs can easily run the test on the required API level. However, if there are some changes made in this screen, then devs first test manually. So we should not add conditions here.

[atharvyadav22]

@MohitMaliFtechiz

Sorry forgot to amend the commit

Fixed Ci for Instrumentation Test and Renamed as per Layer 3 (Integration Test)
Added Test for layer 2 (UI Test)
Address review changes

Also Apologies for bad commit the reason is there was merge conflict so had to reset head .

[MohitMaliFtechiz]

@atharvyadav22 Thanks for the update.

There are still some conflicts in this PR. Also, in this commit 1c778bf there are still some unnecessary messages in the commit; remove that.

[atharvyadav22]

@MohitMaliFtechiz
Could you verify everything is fine now any improvements?
also there were many merge conflicts i have resolved them still please verify if i did correctly the rebase part for files like KiwixFilePermissionChecker and others.
I have rewritten the history as well and edited the commit message.
Thanks.