On Writing Mock Test for openmrs

aastorg2 · April 26, 2016, 2:08am

Hello Developers,

I along with another developer are interested in writing mock test in openmrs. I have browsed through the ticket history past git commits, mock test documentation and noticed that there was an initial effort to mock the environment interactions of the unit test in openmrs. However, I my impression is that this has been abandoned.

I believe the idea of focusing on the unit test that currently take the longest time would be a reasonable place to start. It seems to me, the interesting logic within openmrs makes us of in-memory database and the spring framework (service context calls).

would you be able to provide some guidance on how to proceed?

raff · April 26, 2016, 10:00am

Hi Angello,

Thanks for your initiative!

You could start from reading https://wiki.openmrs.org/display/docs/Mock+Doc. It’s where I laid out a possible approach for introducing mocks in OpenMRS. It’s not written in stone and we welcome any improvements.

It’s a good idea to start off from identifying the slowest tests in our code base and then creating issues for converting them to mocks.

Sometimes it does make sense to keep an old test around to run it less often and exclusively on our CI as opposed to run it on dev’s machine every build. For that purpose we can rename the old test from e.g. EncounterTest to EncounterIT. We have a junit config in place, which will run tests ending wtih “IT” only when the integration-test profile is enabled in maven.

It would be best if you shared a list of the slowest tests here so that we could have a look at them and decide which ones make sense to be converted.

aastorg2 · April 26, 2016, 8:48pm

Hello Raff,

I have familiarized with the Moc+Doc documentation and mockito. I have tried to write my own mock test for some of the openmrs unit test but It is a little complicated since I still don’t have a complete grasp of the openmrs logic.

What you propose sounds reasonable. I’ll share a list here soon.

aastorg2 · April 30, 2016, 9:37pm

Hello Raff,

I looked into the top classes that take the most time and selected the longest unit test in terms of execution time (sure-fire reports).

org.openmrs.api.PatientServiceTest

 shouldGetPatientsByNameShouldLimitSize	0.523
 getPatientIdentifierTypeByUuid_shouldFetchPatientIdentifierTypeWithGivenUuid	0.261
 mergePatients_shouldAuditMovedIndependentObservations	0.236
 voidPatientIdentifier_shouldThrowAnAPIExceptionIfTheReasonIsNull	0.224
 shouldFetchNamesForPersonsThatWereFirstFetchedAsPatients	0.207
 voidPatient_shouldVoidGivenPatientWithGivenReason	0.163
 mergePatients_shouldChangeUserRecordsOfNonPreferredPersonToPreferredPerson	0.162
 getPatients_shouldFetchAllPatientsThatPartiallyMatchGivenName	0.153
 shouldGetPatientsByIdentifierAndIdentifierType	0.135
 mergePatients_shouldMergeAllNonPreferredPatientsInTheTheNotPreferredListToPreferredPatient	0.13
 getPatients_shouldNotReturnVoidedPatients	0.124
 getPatients_shouldAllowSearchStringToBeOneAccordingToMinsearchcharactersGlobalProperty	0.112
 mergePatients_shouldNotCreateDuplicateRelationships	0.104
 mergePatients_shouldOnlyMarkAddressesOfPreferredPatientAsPreferred	0.096

 org.openmrs.api.UserServiceTest

  purgePrivilege_shouldDeleteGivenPrivilegeFromTheDatabase	0.625
  saveUser_shouldGrantNewRolesInRolesListToUser	0.176
  getAllUsers_shouldFetchAllUsersInTheSystem	0.149
  purgeRole_shouldThrowErrorWhenRoleIsACoreRole	0.149
  isSecretAnswer_shouldReturnFalseWhenGivenAnswerDoesNotMatchTheStoredSecretAnswer	0.147
  saveUser_shouldShouldCreateUserWhoIsPatientAlready	0.082


org.openmrs.api.ConceptServiceTest

getConcepts_shouldReturnASearchResultWhoseConceptNameContainsAllWordTokensAsFirst	0.655
getDrugs_shouldGetDrugsLinkedToConceptsWithNamesThatMatchThePhraseAndRelatedLocales	0.42
saveConcept_shouldSaveAConceptNumericAsAConcept	0.418
getConceptByName_shouldGetConceptByName	0.408
saveConcept_shouldSaveNonConceptComplexObjectAsConceptComplex	0.38
getConcepts_shouldReturnConceptSearchResultsThatContainAllSearchWordsAsFirst	0.369
getDrugByMapping_shouldReturnADrugThatMatchesTheCodeAndSourceAndTheBestMapType	0.359
getConceptByName_shouldFindConceptsWithNamesInMoreSpecificLocales	0.341
getCountOfConcepts_shouldReturnACountOfUniqueConcepts	0.321
saveConcept_shouldNotUpdateConceptDataTypeIfConceptIsAttachedToAnObservation	0.312
getDrugByMapping_shouldReturnADrugThatMatchesTheCodeAndSource	0.307


org.openmrs.api.db.PatientDAOTest

  getPatients_shouldEscapeUnderscoreCharacterInIdentifierPhrase	0.424

It seems to me that the service classes is likely what we want to target.

ssabada · April 30, 2016, 10:33pm

Hello Raff,

I am the other developer that is also interested in writing Mock Tests for OpenMRS. Following https://wiki.openmrs.org/display/docs/Mock+Doc we were able to write the mock tests for EncounterTest. We noticed the mocks are used for the service objects. Looking through the other tests we found very few tests that have service objects, so we decided to try in other classes. Some classes simply add and remove objects from some set, such as PatientTest, so mock objects are not necessarily useful there, from what we understand. We also looked at classes such as PatientServiceTest to mock some non-service objects, but there were various function calls such as savePatient which run a validity check which the mock objects fails. Finally we tried for DrugOrderValidator, but we constantly run into the issue of serviceContext being null, which we believe happens because we have to extend BaseContextMockTest instead of BaseContextSensitiveTest.

Do you believe we are just not looking at good examples of tests that can be converted to mock tests, or maybe we should just start with simpler mock tests so we can get the hang of it before trying maybe some of these more difficult unit tests.

Thank you so much for your help!

ssabada · May 3, 2016, 12:47am

In the test case below, we see that in copyAndAssignToAnotherPatient in Encounter.java, it uses EncounterService to save the new Encounter object that is created. Since in the unit test, we don’t necessarily care about saving it as it has no bearing on what is returned, we just care to make sure the object returned is valid. We mock it here to save time so that it does not need to run the save method and it is considerable shorter in length, but we get a message about serviceContext being null and creating a new one. It does seem our Mock still works fine, but we were wondering why exactly this is happening and how we should counter it.

public void copy_shouldCopyAllEncounterDataExceptVisitAndAssignCopiedEncounterToGivenPatient() throws Exception {
	Encounter encounter = new Encounter();
	
	encounter.setCreator(new User());
	encounter.setDateCreated(new Date());
	encounter.setChangedBy(new User());
	encounter.setDateChanged(new Date());
	encounter.setVoided(true);
	encounter.setVoidReason("void");
	encounter.setDateVoided(new Date());
	
	encounter.setEncounterDatetime(new Date());
	encounter.setEncounterType(new EncounterType());
	encounter.setForm(new Form());
	encounter.setLocation(new Location());
	encounter.setPatient(new Patient());
	
	encounter.addObs(new Obs());
	encounter.addOrder(new Order());
	
	EncounterRole encounterRole = new EncounterRole();
	encounter.addProvider(encounterRole, new Provider());
	
	encounter.setVisit(new Visit());
	
	Patient patient = new Patient(7);
	Encounter temp = new Encounter();

	when(encounterService.saveEncounter(temp)).thenReturn(temp);
	
	Encounter encounterCopy = encounter.copyAndAssignToAnotherPatient(patient);
	
	Assert.assertNotEquals(encounter, encounterCopy);
	
	Assert.assertEquals(encounter.getCreator(), encounterCopy.getCreator());
	Assert.assertEquals(encounter.getDateCreated(), encounterCopy.getDateCreated());
	Assert.assertEquals(encounter.getChangedBy(), encounterCopy.getChangedBy());
	Assert.assertEquals(encounter.getDateChanged(), encounterCopy.getDateChanged());
	Assert.assertEquals(encounter.getVoided(), encounterCopy.getVoided());
	Assert.assertEquals(encounter.getVoidReason(), encounterCopy.getVoidReason());
	Assert.assertEquals(encounter.getDateVoided(), encounterCopy.getDateVoided());
	
	Assert.assertEquals(encounter.getEncounterDatetime(), encounterCopy.getEncounterDatetime());
	Assert.assertEquals(encounter.getEncounterType(), encounterCopy.getEncounterType());
	Assert.assertEquals(encounter.getForm(), encounterCopy.getForm());
	Assert.assertEquals(encounter.getLocation(), encounterCopy.getLocation());
	
	Assert.assertEquals(1, encounter.getObs().size());
	Assert.assertEquals(1, encounterCopy.getObs().size());
	Assert.assertEquals(1, encounter.getOrders().size());
	Assert.assertEquals(0, encounterCopy.getOrders().size());
	
	Assert.assertEquals(1, encounter.getProvidersByRole(encounterRole).size());
	Assert.assertEquals(1, encounterCopy.getProvidersByRole(encounterRole).size());
	Assert.assertEquals(true, encounter.getProvidersByRole(encounterRole).containsAll(
	    encounterCopy.getProvidersByRole(encounterRole)));
	
	Assert.assertNotNull(encounter.getVisit());
	Assert.assertNull(encounterCopy.getVisit());
	
	Assert.assertEquals(patient, encounterCopy.getPatient());
}

raff · May 4, 2016, 1:20pm

Could you please share the approach/tool you used to find the longest tests?

You are right that not all tests are good candidates for mocking. getPatientIdentifierTypeByUuid_shouldFetchPatientIdentifierTypeWithGivenUuid for example is a test that makes sure the query to the db is right. If you mock the db then you will test the mock and it doesn’t make sense.

On the other hand copy_shouldCopyAllEncounterDataExceptVisitAndAssignCopiedEncounterToGivenPatient is a good choice for using mocks. The code you pasted seems right to me. Could you please pastebin the stacktrace you get with the message about serviceContext being null? Or if you don’t get any stacktrace, debug and say in which line it occurs?

aastorg2 · May 5, 2016, 1:10am

Hello Raff,

Thanks for the response. The approached I used is the following:

I ran mvn test and dumped the output to a .txt and picked the top 5 slowest classes based on their execution time
I looked for the xml files in api/target/surefire-reports/ corresponding to the 5 classes chosen in step 1 and sorted the slowest running unit test. For example, for UserServiceTest.java, I found TEST-org.openmrs.api.UserServiceTest.xml containing:
```
<testcase name="purgeRole_shouldThrowErrorWhenRoleIsACoreRole" classname="org.openmrs.api.UserServiceTest" time="0.556"/> 
```

This is precisely the dilemma we encountered. Some test such as getPatientIdentifierTypeByUuid_shouldFetchPatientIdentifierTypeWithGivenUuid, although lengthy, should not be mocked precisely for the same reasons you mentioned.

As for copy_shouldCopyAllEncounterDataExceptVisitAndAssignCopiedEncounterToGivenPatient here is the output after running mvn test -Dtest=EncounterIT where EncounterIT is the class containing the unit test copy_shouldCopyAllEncounterDataExceptVisitAndAssignCopiedEncounterToGivenPatient which extends BaseContextMockTest instead of BaseContextSensitiveTest.

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.openmrs.EncounterIT
**ERROR - Context.getServiceContext(251) |2016-05-04 20:06:24,303| serviceContext**
**is null.  Creating new ServiceContext()**
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.497 sec - in o
rg.openmrs.EncounterIT

Results :

Tests run: 1, Failures: 0, Errors: 0, Skipped: 0

Now, there are no failures so could it be that it is expected that serviceContext is null because we don’t extend BaseContextSensitiveTest?

Thanks!

raff · May 17, 2016, 9:22am

Yes it is fine. It should not be marked as ERROR. Sorry for the late reply.