REST module and powermock

Development
,
1

Dear @raff

while working on

I figured why not make all members of the RestServiceImpl private (some are package-private just for ease of testing, seems lazy) and remove the package-private helper method RestServiceImpl.addSupportedSearchHandler(SearchHandler) (which is only used for testing)

and mock the static Context.getRegisteredComponents(SearchHandler.class) with powermock. Which is the actual thing we want to control in our tests.

Now powermock was excluded from the openmrs-test dependency. Is that simply because it wasnt needed until now?

Just so I know that removing the exclusions:

https://github.com/openmrs/openmrs-module-webservices.rest/blob/master/pom.xml#L103-L110

is ok.

2

Excluding Powermock helps write better code by having to eliminate static calls and inject dependencies properly :slight_smile: Also Powermock is sometimes causing trouble with our custom module classloading in tests (sorry I don’t have an example at hand to support that).

Unfortunately, Context.get…Service() is used all over openmrs codebase and we still need to test that code so I’ve introduced ContextMockHelper in BaseContextMockTest and BaseContextSensitiveTest.

See an example of BaseContextMockTest in RequiredDataAdviceTest and BaseContextSensitiveTest in EncounterTest.

And a wiki about mocking.

If you write new code, which doesn’t depend on any methods calling Context, it is better to inject dependencies properly (via a constructor or fields annotated with @Autowired) and test with mockito.

Exposing private methods as package-private is not the best approach for testing, but sometimes it’s a fast enough workaround to test them. It’s very unlikely you will call package-private methods in production by mistake. Calling by reflection in my opinion is worse as it makes the codebase harder to refactor.

The ideal solution for RESTWS-610 would be to refactor the class by extracting the logic to a separate class with public methods, which can be tested properly, without workarounds. It can be done easily, but I guess I didn’t look back when writing it the first time :slight_smile:

3

I would really like to clean this up :slight_smile: because its hard to add [RESTWS-613] - OpenMRS Issues with the current state of affairs. And I agree, PowerMock can be a pain.

So what do you think about:

adding methods to interface RestServiceHelper

<T extends SearchHandler> List<T> RestServiceHelper.getRegisteredSearchHandlers()

<T extends DelegatingSubclassHandler> List<T> RestServiceHelper.getRegisteredSubclassHandlers()

then add dependency RestServiceHelper to RestServiceImpl wire them via spring. Then mock this in the tests.

Is that right?

4

Yes, sounds right.

If you want to practice refactoring I would also move the whole SearchHandler logic out of RestServiceImpl to a new SearchHandlerMatcher class. It’s a bit more over what you need though :slight_smile:

5

sounds good! but I’ll start with this first and maybe on the way to solving [RESTWS-613] - OpenMRS Issues create a new SearchHandlerMatcher

6

dear @raff and @wyclif this is my first stab at the issue https://issues.openmrs.org/browse/RESTWS-610 see https://github.com/openmrs/openmrs-module-webservices.rest/pull/230

the RestServiceImpl+Test gets way cleaner. Im sure there is still somethings I can do better, so please keep the comments coming :smile:

not sure how I would write tests for RestHelperServiceImpl without powermock though.