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