Access rights of fields regarding JUnit tests

Question

Suppose I have a class which implements a stack in the style of a linked list:

public class Stack {
    private StackNode base;

    public void push(Object item) {
        if (base == null) {base = new StackNode(item);}
        // remaining implementation not shown
    }
}

and a JUnit test class for it:

public class TestStack {
    @Test
    public void testPush() {
        Stack st = new Stack();
        st.push(new Integer(3)); 

        assertEquals((Integer)st.base.getContents(), new Integer(3)); //Error!
        // other test cases not shown
    }
}

My question is: what is the best practice for solving the issue of the test class's access rights to base?

I am aware of this question which discusses making an accessor, and I know that I could also put my Stack and TestStack code into a package and make the base field package-private.

I'm wondering what the best course of action is. I know I shouldn't be using pop in push's unit test, and vice versa, as this couples those unit tests.

I think that making a (public) accessor is bad practice if I don't want client code to access the field, so is the right call to make an accessor for the field (or the field itself) package-private?

Another informative question discusses options for private methods, but this is regarding a public method, and to test it I need to access a private field, so the many statements of "if you need to access private methods/classes, you're doing it wrong" doesn't seem to apply here.

If you're just reading that value, why not simply implement a getter for that? — QBrute, May 17 '17 at 13:53
Why do you need to test that in any case? If it's a stack, isn't there a public pop() method you can call to check if it's correctly saving the value? — Florian Schaetz, May 17 '17 at 14:01
Test the behavior - not the implementation. The internal guts should not matter. You should be able to change the implementation as needed. As long as the signature of the public method doesn't change the unit tests should not need to be changed. — Andrew S, May 17 '17 at 14:09
Still, a nice newbie question, as you actually sat down and did some prior research. My vote for that ... have fun practicing upvoting ... and don't forget to accept the most helpful answer at some point. — GhostCat, May 17 '17 at 14:26

score 4 · Accepted Answer · answered May 17 '17 at 14:10

4

We should write tests that exercise the public API. It's not always possible though. In this case I would write a test that exercises two methods together, push and pop because these two make a functional stack:

public class Stack {
    private StackNode base;

    public void push(Object item) {
        ...
    }

    // I know this wasn't part of your code
    public Object pop() {
        ...
    }
}

public class TestStack {
    @Test
    public void testOnePush() {
        // GIVEN
        Stack st = new Stack();

        // WHEN
        st.push(new Integer(3)); 
        Object popped = st.pop();

        assertEquals(popped, new Integer(3));
    }
}

If this kind of testing is not possible, then it's okay to expose base either directly or through a getter. In this case I prefer to write a warning comment, i.e. it's only for testing:

public class Stack {
    StackNode base; // package-private for testing reasons
}

It's possible to warn the other developers with method names too:

public class Stack {
    private StackNode base;

    /** For unit tests only!!! */
    public StackNode getBaseForTests() {
         return base;
    }
}

answered May 17 '17 at 14:10

Tamas Rev

7,008
5
32
49

1

Haha, I put down the conceptual idea; you put down some code. Nice pair of answers methinks ... you got my vote for that. – GhostCat May 17 '17 at 14:12
1

And you may want to `assertSame` rather than `assertEquals` in your case. I think it would be surprising if they were different instances. – Michael May 17 '17 at 14:15
Also, adding methods specifically for testing is a *terrible* idea. Tests support the code, not the other way around. If you find your self needing to do that, you need to redesign your class. – Michael May 17 '17 at 14:21
1

@Michael I disagree. Everything in IT is about balancing. If adding that one package protected method makes your life 10 times easier compared to spending hours of other activities ... then that can be the right choice. It depends on the context; and on questions like "how often will this code be touched in the future" for example. One simply needs the experience to make that call to determine the "return on investment" for specific implementation choices. – GhostCat May 17 '17 at 14:23
So @tamas-rev what you're saying is this is **not** an instance of bad coupling to put `push` and `pop` test code in the same test case? Even though `pop` having an issue means that my `push` tests might break, too? – NickZ May 17 '17 at 14:34
@GhostCat Nah. If you need to test some implementation detail that desperately (hint: you don't) then use reflection. You would never, ever write a test that looked like that if you were writing the test *before* the class. – Michael May 17 '17 at 14:37
1

Reflection breaks when you do refactoring; calling a method does not. I think this is really getting into the opinion space: I avoid using *reflection* at all cost; you have a different opinion. I am simply saying that I don't agree with the "absoluteness" in your statements. – GhostCat May 17 '17 at 14:40
2

@NickZ yes. The trick is that a unit test - a really confusing term - tests a functional unit. A functional unit is something that has no exact definition. In this case though, the `push` method is meaningless without a `pop` method, so... I believe I read about this exact example in a unit testing book / blog post. – Tamas Rev May 17 '17 at 14:41
@GhostCat I recommended not to use either. See Andrew S's comment on the question. It doesn't get more succinct than that. – Michael May 17 '17 at 14:42

score 1 · Answer 2 · answered May 17 '17 at 14:08

It seems that you have read a lot of question, but didn't get to the core of the message.

The core point is: you do not test internals of your production code. You are interested in verifying the public contract, the what. You want to avoid doing that by verifying the how.

In other words: if you have a class that implements a stack, then the ideal unit test for such a class ... simply checks the behavior of an object of that class. Meaning: you verify that pushing and popping values leads to the expected results. That popping an empty stack throws an exception, ... and so on.

If you really want to check the how part, one solution that works without changing access modifiers is to use Mockito and its @InjectMock annotation.

In other words: you could mock a StackNode object, and @InjectMock does some reflection magic to give you a Stack object using that mocked StackNode. Now you can use the Mockito verify() approach to ensure that this StackNode sees the calls you would expect it to see.

But of course: that means that you invest a lot of time and energy to write a test that fully depends on the implementation details of Stack. As soon as you decide to change it, that whole unit test will break; and you will have to completely rewrite that as well.

And that is why you prefer to test the what, not the how.

Beyond that: is is common best practice that your class under test and its test class life in the same package (not in the same project). And having a package-protected getter for a field that says // unit test only is an informal, but surprisingly well-working solution, too.

Access rights of fields regarding JUnit tests

2 Answers2