How to test method which uses other method - Mockito, java, junit

Question

I would like to test method which use another one? I tried do it using Mockito like below:

EDIT: Full method

public String createUrlAddress(TypeOfInformation typeOfInformation, String icao) {
    switch (typeOfInformation) {
        case METAR:
            urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_METAR + icao;
            break;
        case TAF:
            urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_TAF + icao + StaticValues.TAF_4_HOURS_BEFORE_NOW;
            break;
        case CITY_PAIR_METAR:
            urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRS
                    + pc.getDepartureAndArrivalTime().get("departureTime") //get departure time from hashmap
                    + StaticValues.END_TIME_STRING
                    + pc.getDepartureAndArrivalTime().get("arrivalTime")
                    + StaticValues.STATION_STRING
                    + pc.getOriginIcao()
                    + ","
                    + pc.getDestinationIcao()
                    + StaticValues.MOST_RECENT_FOR_TYPED_STATIONS;
            System.out.println(urlAddress);
            break;
        case CITY_PAIR_TAFS:
            urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRS_TAFS
                    + pc.getDepartureAndArrivalTime().get("departureTime")
                    + StaticValues.END_TIME_STRING
                    + pc.getDepartureAndArrivalTime().get("arrivalTime")
                    + StaticValues.STATION_STRING
                    + pc.getOriginIcao()
                    + ",%20"
                    + pc.getDestinationIcao()
                    + StaticValues.MOST_RECENT_FOR_TYPED_STATIONS_TAFS;
            System.out.println(urlAddress);
            break;
        default:
            System.out.println("Wrong Type of informations");
    }
    return urlAddress;
}

Tests:

@Test
    public void forGivenTypeOfInformationAndIcaoReturnUrl() {
        HashMap<String,Long> departureAndArrivalTimeTest = new HashMap<>();
        departureAndArrivalTimeTest.put("departureTime", 1499264449L);
        departureAndArrivalTimeTest.put("arrivalTime", 1499282449L);
        PageControllerForNearestCity pcSpy = Mockito.spy(pc);
        Mockito.when(pcSpy.getDepartureAndArrivalTime()).thenReturn(departureAndArrivalTimeTest);

        Mockito.when(pcSpy.getOriginIcao()).thenReturn("EPWA");
        Mockito.when(pcSpy.getDestinationIcao()).thenReturn("EDDF");

        assertThat(StaticValuesForTest.URL_ADDRESS_FOR_CITY_PAIR_METAR).isEqualTo(xmlParser.createUrlAddress(TypeOfInformation.CITY_PAIR_METAR, "EPGD")); }

How can I use my mocks in that case? Is it good approach or I have to do it in other way? I would like to add that I won't add these variables as arguments for this method.

PS I thought that the method has only one resposibility, just create a string, am I wrong? Should it be divided into another one like a "Service"?

Thank you for support

Answer 1

I think you are in the right direction! You are mocking the dependencies of your code and that dependency is exactly the PageControllerForNearestCity!

One observation about the mock, you have to inject it on xmlParser, like this:

@Test
public void forGivenTypeOfInformationAndIcaoReturnUrl() {
    // here you created the mock
    PageControllerForNearestCity pcSpy = Mockito.spy(pc);
    // I'll assume that xmlParser is the subject of your test
    // and that you're injecting the mock like code below 
    xmlParser.setPageController(pcSpy);

    // Configure the mock and then you do the assertion
    assertThat(...)
}

PS I thought that the method has only one resposibility, just create a string, am I wrong? Should it be divided into another one like a "Service"?

Your method is good! It really do one thing and well and that is building an url from TypeOfInformation

My suggestion is that you refactor your code, after you write your test codes and make it pass! You can remove code duplication and make it more readable!

Remeber this:

'Any fool can write code that a computer can understand. Good programmers write code that humans can understand.'

Martin Fowler

Good coding!

Edit Some examples of your code with some refactoring

public String createUrlAddress(TypeOfInformation typeOfInformation, String icao) {
    String urlAddress;
    switch (typeOfInformation) {
        case METAR:
            urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_METAR + icao;
            break;
        case TAF:
            urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_TAF + icao + StaticValues.TAF_4_HOURS_BEFORE_NOW;
            break;
        case CITY_PAIR_METAR:
            // We delegate the build logic to pc because
            // all the information needed to build the url
            // is in the PageControllerForNearestCity class
            urlAddress = pc.urlAddressForCityPairMetar(); 
            break;
        case CITY_PAIR_TAFS:
            // Same
            urlAddress = pc.urlAddressForCityPairTafs();
            break;
        default:
            System.out.println("Wrong Type of informations");
    }
    return urlAddress;
}

class PageControllerForNearestCity {

    public String urlAddressForCityPairMetar() {
        return urlBasedOn(StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRSS, ",", StaticValues.MOST_RECENT_FOR_TYPED_STATIONS);
    }
    public String urlAddressForCityPairTafs() {
        return urlBasedOn(StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRS_TAFS, ",%20", StaticValues.MOST_RECENT_FOR_TYPED_STATIONS_TAFS);
    }
    // This method removes the duplication that I mentioned before
    private String urlBasedOn(String mainUrl, String separator, String endString) {
        return mainUrl
                + this.getDepartureAndArrivalTime().get("departureTime")
                + StaticValues.END_TIME_STRING
                + this.getDepartureAndArrivalTime().get("arrivalTime")
                + StaticValues.STATION_STRING
                + this.getOriginIcao()
                + separator
                + this.getDestinationIcao()
                + endString;
    }
}

Note that after this refactoring, your forGivenTypeOfInformationAndIcaoReturnUrl test method will become much simpler. But you will have to create test for urlAddressForCityPairMetar() and urlAddressForCityPairTafs().

Answer 2

@rafaelim After your response I updated my test class and injected mock to the class like below:

@Before
    public void setUp() throws Exception {
        departureAndArrivalTimeTest = new HashMap<>();
        xmlParser = new XmlParser();
        pc = new PageControllerForNearestCity();
        departureAndArrivalTimeTest.put("departureTime", 1499264449L); //second arg dep time in sec
        departureAndArrivalTimeTest.put("arrivalTime", 1499282449L); //second arg arr time in sec
    }

    @Test
    public void forGivenTypeOfInformationAndIcaoReturnUrl() {
        PageControllerForNearestCity pcSpy = Mockito.spy(pc);
        xmlParser.setPc(pcSpy);
        Mockito.when(pcSpy.getDepartureAndArrivalTime()).thenReturn(departureAndArrivalTimeTest);

        Mockito.when(pcSpy.getOriginIcao()).thenReturn("EPWA");
        Mockito.when(pcSpy.getDestinationIcao()).thenReturn("EDDF");


        assertThat(StaticValuesForTest.URL_ADDRESS_FOR_METAR).isEqualTo(xmlParser.createUrlAddress(TypeOfInformation.METAR, "EPGD"));
        assertThat(StaticValuesForTest.URL_ADDRESS_FOR_TAF).isEqualTo(xmlParser.createUrlAddress(TypeOfInformation.TAF, "EPGD"));
        assertThat(StaticValuesForTest.URL_ADDRESS_FOR_CITY_PAIR_METAR).isEqualTo(xmlParser.createUrlAddress(TypeOfInformation.CITY_PAIR_METAR, "EPGD"));
        assertThat(StaticValuesForTest.URL_ADDRESS_FOR_CITY_PAIR_TAF).isEqualTo(xmlParser.createUrlAddress(TypeOfInformation.CITY_PAIR_TAFS, "EPGD"));
    }

Test passed, but its a little bit unreadable, I have to work with "clean code" I think.

EDIT: @davidxxx please look at this:

public class UrlAddressService {

    PageControllerForNearestCity pc = new PageControllerForNearestCity();

    public String createUrlForMetar() {

        String urlAddressForMetars = new StringBuilder()
                .append(StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRS)
                .append(pc.getDepartureAndArrivalTime().get("departureTime"))
                .append(StaticValues.END_TIME_STRING)
                .append(pc.getDepartureAndArrivalTime().get("arrivalTime"))
                .append(StaticValues.STATION_STRING)
                .append(pc.getOriginIcao())
                .append(",")
                .append(pc.getDestinationIcao())
                .append(StaticValues.MOST_RECENT_FOR_TYPED_STATIONS_METARS)
                .toString();

        return urlAddressForMetars;
    }

    public String createUrlForTaf() {

        String urlAddressForTafs = new StringBuilder()
                .append(StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRS_TAFS)
                .append(pc.getDepartureAndArrivalTime().get("departureTime"))
                .append(StaticValues.END_TIME_STRING)
                .append(pc.getDepartureAndArrivalTime().get("arrivalTime"))
                .append(StaticValues.STATION_STRING)
                .append(pc.getOriginIcao())
                .append(",%20")
                .append(pc.getDestinationIcao())
                .append(StaticValues.MOST_RECENT_FOR_TYPED_STATIONS_TAFS)
                .toString();

        return urlAddressForTafs;
    }
}

And createUrlAddress method:

public String createUrlAddress(TypeOfInformation typeOfInformation, String icao) {
        switch (typeOfInformation) {

            case METAR:
                urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_METAR + icao;
                break;
            case TAF:
                urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_TAF + icao + StaticValues.TAF_4_HOURS_BEFORE_NOW;
                break;
            case CITY_PAIR_METAR:
                urlAddress = addressService.createUrlForMetar();
                break;
            case CITY_PAIR_TAFS:
                urlAddress = addressService.createUrlForTaf();
                break;
            default:
                System.out.println("Wrong Type of informations");
        }
        return urlAddress;
    }

Do you think that it is better approach? I cannot reduce code during building a URL String, because there are 3 different parts of code for Tafs and Metars. Could you show me the best way how to test it if my test are bad?

Answer 3

Your test enters too much in implementation details.
You mock the own processings/logic of your method. So it makes the test brittle and we can wonder what you assert really.
Besides, the test is complicated to read and to maintain.

At last, the processing associated to each case matters. It is the main logic of your method :

case CITY_PAIR_METAR:

   urlAddress = StaticValues.MAIN_URL_ADDRESS_FOR_CITY_PAIRS
        + pc.getDepartureAndArrivalTime().get("departureTime") //get departure time from hashmap
        + StaticValues.END_TIME_STRING
        + pc.getDepartureAndArrivalTime().get("arrivalTime") //get arrival time from hashmap
        + StaticValues.STATION_STRING
        + pc.getOriginIcao()
        + ","
        + pc.getDestinationIcao()
        + StaticValues.MOST_RECENT_FOR_TYPED_STATIONS;
   System.out.println(urlAddress);

It should be tested without mocking as you actually doing.

To do it, you should separate responsabilities by introducing a new class.
The actual class should only have a controller/dispatcher role and the new class should perform the logic with a public method by case.

In this way, you class under test could have a dependency on this class and you could mock them in a straight way.

Your actual method could finally look like :

private AddressService addressService;

public String createUrlAddress(TypeOfInformation typeOfInformation, String icao) {
                switch (typeOfInformation) {

        (...)
                    case CITY_PAIR_METAR:
                        urlAddress = addressService.createUrl();
                        break;
         (...)
                    default:
                        System.out.println("Wrong Type of informations");
                }

               return urlAddress;
 }