gmantele / vollt

Java libraries implementing the IVOA protocol: ADQL, UWS and TAP
http://cdsportal.u-strasbg.fr/taptuto/
30 stars 29 forks source link

Remove final qualifier for methods #83

Closed marcdexet-cnrs closed 6 years ago

marcdexet-cnrs commented 6 years ago

A lot of methods in the uws package are qualified as final. It make them impossible to mock with frameworks as Mockito, and it's very painful to setup a whole UWS stack to just test a simple class.

This very basic test throws out exception

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import org.mockito.junit.MockitoJUnitRunner;

@RunWith(MockitoJUnitRunner.class)
class SomeTests {

    @Test tests() {
              JobList jobList = mock(JobList.class);
              when(jobList.getName()).thenReturn("joblist-name");
     }
}

The thrown exception is

org.mockito.exceptions.misusing.MissingMethodInvocationException: (...) 1. you stub either of: final/private/equals()/hashCode() methods. Those methods cannot be stubbed/verified. Mocking methods declared on non-public parent classes is not supported.

Could you please remove final qualifiers to use current java tooling.

gmantele commented 6 years ago

All final qualifiers are generally in the library for a good reason. The idea is to prevent modifying fix behaviours of the library. If I remove them, I will allow the modification of all these behaviours which may break most of what I designed the library for. This library allow extensions and modifications but not of some core functionalities. I still want the library do several things as I expect, and especially as a UWS service (in your case). So, sorry, but I won't remove these final qualifiers.

I do not know Mockito, but isn't it possible to specify which function you want to test?

Anyway, I already provide some JUnit tests in the directory test. I admit, they are not complete since I started them after few stable versions of the library, but I update them for each new bug report or new feature when it is possible.

Is there any special reason why you want to use a tool like Mockito?

marcdexet-cnrs commented 6 years ago

TLDR;

Mockito is a quasi-standard for mocking in unit testing. Unit testing is crucial. Mockito helps you to respect good practices in testing and Mockito can not mock final methods.

Long version :)

Well, I understand the usual motivations of the final use in java. But it could imply a too tightly coupling for testing good practices. I'm working to refactoring a cosmological application for IAS laboratory (Orsay) and I write unit testing for each component. That's the good practice.

The current way of doing this is to mock or stub the associated component of the under test class, and it's a bad practice to set up a whole stack to just test a class or method.

I put a test case for illustration: I want a flexible and not hard coded architecture to detect new kind of job (easy to do with Spring).

I have such a class

public class MagycUWSFactory extends AbstractUWSFactory {
      public MagycUWSFactory(List<JobThreadFactory> factories) {
        this.factories = factories;
    }
    public JobThread createJobThread(UWSJob job) throws UWSException {
        String jobListName = job.getJobList().getName();
        JobThreadFactory factory = findJobThreadFactory(jobListName);
        return factory.newJobThread(job);
    }

    protected JobThreadFactory findJobThreadFactory(String jobListName) throws UWSException {
        return factories.stream()
                .filter( f -> f.getName() == jobListName )
                .findFirst()
                .orElseThrow( () -> new UWSException(String.format("No registered job with %s name", jobListName)));
    }

I just want to put createJobThread under unit test, a trivial task.

With Mockito, you just have to mock called methods : it's easy and direct

               //__GIVEN__
        jobThreadFactory = mock(JobThreadFactory.class);
        factory = new MagycUWSFactory(Arrays.asList(jobThreadFactory));
               JobThread jobThread = mock(JobThread.class) 
                when(jobThreadFactory.newJobThread( any() ).thenReturn(jobThread );

                //__WHEN__
        when(jobThreadFactory.getName()).thenReturn(JOBLIST_NAME);

               JobThread returnedJobThread = factory.createJobThread(job);

                //__THEN__
        assertThat(returnedValue).isEqualTo(jobThreadFactory);

And that's all.

But as all is final and final methods can not be overridden by Mockito, I have to build a pretty full of actual object just to test my small function, with all the internal mechanism of theses out-of-testing-scope to implement just to test this small methods

           //__GIVEN__
            params = new UWSParameters(new HashMap<String, Object>());
        job = new UWSJob(params);
        jobList = new JobList(joblistName);
        uws = mock(UWS.class);
        jobList.setUWS(uws);
        jobList.addNewJob(job);

        jobThreadFactory = mock(JobThreadFactory.class);
               // Tell what to do when getName is called
        when(jobThreadFactory.getName()).thenReturn(JOBLIST_NAME);
        factory = new MagycUWSFactory(Arrays.asList(jobThreadFactory));

                //__WHEN__
        when(jobThreadFactory.getName()).thenReturn(JOBLIST_NAME);

               JobThread returnedJobThread = factory.createJobThread(job);

                //__THEN__
        assertThat(returnedValue).isEqualTo(jobThreadFactory);
....

with a local test class out-of-testing-scope

    public class LocalJobThread extends JobThread {

        public LocalJobThread(UWSJob j) throws NullPointerException {
            super(j);
        }

        @Override
        protected void jobWork() throws UWSException, InterruptedException {
        }
    }

Mocking is good because you can emulate behaviors without to modify core functionalities. final or lack of interfaces make mocking very difficult to use.

In my work, I don't use final anymore for methods.

My 2 cents :)

marcdexet-cnrs commented 6 years ago

And I can advocate that putting new unit test with Mockito is very, very easy and ravishing task. Not a heavy and boring and time consumming. You keep the Type security as Mockito just override existing methods, you keep the flexibility of refactoring of your IDE. Mocks are Java object, not a loose and unmaintainable scripted executable.

Try it, you will love it.

vforchi commented 6 years ago

Have you tried this? It looks like it is possible to mock final classes and methods: https://www.baeldung.com/mockito-final

marcdexet-cnrs commented 6 years ago

@vforchi Oh you could save my day :) Thanks!

marcdexet-cnrs commented 6 years ago

@vforchi your solution from baeldung is a valid one, thank you.

I quote the solution from https://www.baeldung.com/mockito-final for futur users


Before Mockito can be used for mocking final classes and methods, it needs to be configured.

We need to add a text file to the project’s src/test/resources/mockito-extensions directory named org.mockito.plugins.MockMaker and add a single line of text:

mock-maker-inline

Mockito checks the extensions directory for configuration files when it is loaded. This file enables the mocking of final methods and classes.

marcdexet-cnrs commented 6 years ago

I validate solution proposed by @vforchi ; I close the issue.

gmantele commented 6 years ago

Great! :+1:

Sorry that I could not have helped you....it would have mean a too big change in the whole libraries. But anyway, if you find anything weird with the help of Mockito, do not hesitate to report it.