lundi 18 janvier 2021

Wrapping static method call into instance method for the purpose of unit testing?

We have a class that calls a static method from another class to retrieve a map of properties, from which then some values are to be used

public SomeReturnType methodUnderTest(HttpServletRequest httpServletRequest) { 
  ...
  String value = SomeClass.getPropertiesAsMap(httpServletRequest).get(SOME_PROPERTY);
  ...

The corresponding part of the unit test looks like as follows; it requires some expect statements to set up the static method call

HttpServletRequest httpServletRequestMock = mock(HttpServletRequest.class);
Properties properties = ... //create some test properties
SomeClass someClassMock = mock(SomeClass.class); //1
expect(httpServletRequestMock.getAttribute(SOME_ATTRIBUTE_NAME_FROM_SOMECLASS)).andReturn(someClassMock); //2 
expect(httpServletRequestMock.getAttribute(SOME_OTHER_ATTRIBUTE_NAME_FROM_SOMECLASS)).andReturn(properties); //3
var actual = underTest.methodUnderTest(httpServletRequestMock); 

During the code review of this some team member started to argue that the static method call should be wrapped as such into an instance method of a newly created wrapper class; and this wrapper class should be injected into our class under test and be called from there as follows instead of the above

String value = wrapperClassForSomeClass.getPropertiesAsMap(httpServletRequest).get(SOME_PROPERTY);
    

Reasoning for his comment was that this way the above test lines 1-2-3 can be simplified like this

WrapperClassForSomeClass wrapperClassForSomeClassMock = mock(WrapperClassForSomeClass.class);
expect(wrapperClassForSomeClassMock.getPropertiesAsMap(HttpServletRequestMock)).andReturn(properties);

and "R" (Repeatable) of the FIRST unit testing principles is getting fulfilled, i.e. not depending on other class

While this reasoning might possibly be correct, modifying code for testing purposes on the other hand is generally considered as not necessarily a good practice. At least for me in this particular case creating a wrapper class with the sole responsibility of calling a static method for the purpose of unit testing seems to be dubious. So is this comment valid after all or not?

(Note: SomeClass is legacy it is not an option to modify it e.g. by changing the static method to instance or so)

Aucun commentaire:

Enregistrer un commentaire