Unit test code for a private median-of-three function, using reflection [closed]

Question

A Junit test class is used to test the class LibraryCounter. Reflection was used to test the private methods. This made the code much more complicated.

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.junit.Test;
import static org.junit.Assert.*;

public class LibraryCounterTest {

 public LibraryCounterTest() { }

 @Test
 public void medianEasyTest() {
     System.out.println("median");
     int[] sample = {7,8,9};
     Class[] args = new Class[3];
     args[0] = int[].class;
     args[1] = Integer.TYPE;
     args[2] = Integer.TYPE;

     try {
           Method m = LibraryCounter.class.getDeclaredMethod("median", args);
          m.setAccessible(true);
          try {
              Object o = m.invoke(null, sample, 0, sample.length-1);
              int result = (int)o;
              assertEquals(8, result);
          } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException ex) {
             Logger.getLogger(LibraryCounterTest.class.getName()).log(Level.SEVERE, null, ex);
        }

    } catch (NoSuchMethodException | SecurityException ex) {
         Logger.getLogger(LibraryCounterTest.class.getName()).log(Level.SEVERE, null, ex);
    }

     assertEquals(7, sample[0]);
     assertEquals(8, sample[1]);
     assertEquals(9, sample[2]);
 }
}

This is just the first and simplest test. The method being tested has the following signature private static int median(int[], int, int) If the method was public then everything inside the outermost try could be condensed to one line.

Here is the method being tested from the LibraryCounter class:

/*
*Sorts first, center and last element, swaps the new center element with the one before the new last and returns its value.
*/
private static int median(int[] sample, int start, int end) {
    if(sample.length < 3) {
        throw new IllegalArgumentException("arrays of length three or greater");
    }

    int center = start + ((end-start)/2);

    if(sample[start] > sample[end])
       swap(sample, start, end);

    if(sample[start] > sample[center])
       swap(sample, start, center);

    if(sample[center] > sample[end])
       swap(sample, center, end);

int secondLast = end - 1

    swap(sample, center, secondLast );

    return sample[secondLast];

}

//swaps two elements in array given their positions
private static void swap(int[] sample, int x, int y) {
    int temp = sample[x];
    sample[x] = sample[y];
    sample[y] = temp;
}

Answer 1

As I've said in my comments, I'm not a huge fan of testing private methods. It couples the tests very closely to the implementation of your class which makes even simple refactorings much harder than they need to be. That said, lets have a look at your test.

Naming

medianEasyTest tells me nothing really about what the test is expecting. median is the name of the method you're calling. EasyTest tells me nothing about what you're passing, or what you're expecting back. This doesn't make the intention of the test easy to read.

Exceptions

You're catching exceptions in your test and more than that, whilst you're logging them, you're swallowing the exceptions. I really don't like that. If your code throws an exception, you should let the exception escape the test and fail it. Having the test fail on assertEquals(7, sample[0]); when it's actually failed because it threw an IllegalAccessException is just confusing.

Setup

If you're going to be running multiple tests against the same private methods, then it makes sense to do a lot of the prep work either in a method called explicitly, or in a Before method on the test class (notice that I'm not catching any exceptions. If they happen, because your class structure changes you want it to fail:

@Before
public void setupPrivateCalls() {
   Class[] args = new Class[3];
   args[0] = int[].class;
   args[1] = Integer.TYPE;
   args[2] = Integer.TYPE;

   medianMethod = LibraryCounter.class.getDeclaredMethod("median", args);
   medianMethod.setAccessible(true);
}

Method medianMethod;

Double checking

You're checking that sample hasn't been updated at the end of your test. Do you really need to do that?

Putting it together

Putting all of that together, you end up with an actual test method that looks more like this:

public void testMedianIs8From789() {
     System.out.println("median");
     int[] sample = {7,8,9};

     int result  = (int)medianMethod.invoke(null, sample, 0, sample.length-1);
     assertEquals(8, result);
 }

Exceptions from privates

This is probably subjective, but I don't like the way your private method is throwing an exception:

private static int median(int[] sample, int start, int end) { if(sample.length < 3) { throw new IllegalArgumentException("arrays of length three or greater"); }

If your private throws that exception, it's because something under your classes control (one of it's public methods) has failed to supply the right data. Validation should be performed at the public interface to the class, you shouldn't need to perform it again in your privates.