google / ground-android

Ground mobile data collection app for Android
http://groundplatform.org
Apache License 2.0
244 stars 115 forks source link

[Test Coverage] Add Unit Test for CaptureLocationTaskViewModel #2519

Open anandwana001 opened 3 months ago

anandwana001 commented 3 months ago

Add Unit Test for CaptureLocationTaskViewModel

Few Observations:

  1. Currently, In the constructor of CaptureLocationTaskViewModel we are passing Resources, which is an android component.
  2. The idea of ViewModel is to keep it independent from the Android-specific items, like Resources.

What is the Current Issue? The reason the resources is there in the CaptureLocationTaskViewModel is because it needs to pass to AbstractTaskViewModel. In AbstractTaskViewModel it is only used to access the string from the resources.

How we can fix this? The validate function currently passes the String (resources), rather we can pass a boolean, saying isValid or notValid. The calling site will be responsible for accessing the Android resources. This way we can make our ViewModel, Android independent

@shobhitagarwal1612 WDYT?

anandwana001 commented 3 months ago

Also, as the current usage of AbstractTaskViewModel is a bit big, I think we should create a separate issue for fixing the AbstractTaskViewModel.

WDYT?

shobhitagarwal1612 commented 3 months ago

For the other tasks, we added tests for the task fragment which takes care of hilt dependencies automatically including the view model. This also provides coverage for what the user sees rather than testing the VM separately. Can you please check if we can do the same for CaptureLocationTask as well?

anandwana001 commented 3 months ago

Let me rephrase,

Screenshot 2024-06-26 at 15 08 07

Not sure I get it, but what I meant earlier is that we are passing the Resources object in the ViewModel, which is not a good practice and making our ViewModel dependent on the Android library.

So, we should separate the resource out of ViewModel and then go for writing correct test cases.

shobhitagarwal1612 commented 3 months ago

Sounds good to refactor the code first. I thought you were referring to adding tests for the view model explicitly, which we shouldn't do if possible.

anandwana001 commented 3 months ago

Sounds good to refactor the code first. I thought you were referring to adding tests for the view model explicitly, which we shouldn't do if possible.

ok, great, for refactoring, I will create a separate issue.

Regarding testing viewModel, aren't we testing it separately? So, we are doing only UI tests for fragments and based on that we are confirming the viewModel is also tested, right? or did I missed anything?

shobhitagarwal1612 commented 3 months ago

Regarding testing viewModel, aren't we testing it separately? So, we are doing only UI tests for fragments and based on that we are confirming the viewModel is also tested, right? or did I missed anything?

That's correct. The rationale is that this ensures that we have enough coverage for the fragment as well instead of having duplicate tests for UI and ViewModel