11
\$\begingroup\$

I am using reflection to check if a private variable is set to null after the logout function. This is needed because the getUser function will always attempt to set and return an user if no user is set. Am I doing something wrong or can I do something better?

This is the bean that has the logout and getUser function:

@ManagedBean(name = "authBean")
@SessionScoped
public class AuthorizationBean implements Serializable{

    //Data access object for the users
    @Inject
    UserDao userDao;

    private User user; // The JPA entity.
    public User getUser() {
        if (user == null) {
            user = (User) FacesContext.getCurrentInstance().getExternalContext().getSessionMap().get("user");
            if (user == null) {
                Principal principal = FacesContext.getCurrentInstance().getExternalContext().getUserPrincipal();
                if (principal != null) {
                    user = userDao.findByEmail(principal.getName()); // Find User by j_username.
                }
            }
        }
        return user;
    }

    /**
     * Function that handles the logout
     * @return Redirect string that points to the login page
     */
    public String doLogout() {
        // invalidate the session, so that the session is removed
        FacesContext.getCurrentInstance().getExternalContext().invalidateSession();
        user = null;
        // return redirect to login page
        return "/login.xhtml?faces-redirect=true";
    }


}

This is the test class, with the reflection:

@RunWith(PowerMockRunner.class)
@PrepareForTest(FacesContext.class)
public class AuthorizationBeanTest {

    private AuthorizationBean authorizationBean;

    @Mock
    User user;

    @Mock
    FacesContext facesContext;

    @Mock
    ExternalContext externalContext;

    @Before
    public void setUp() {
        authorizationBean = new AuthorizationBean();
        Map<String,Object> sessionMap = new HashMap<>();
        sessionMap.put("user", user);

        //Mocking the static function getCurrentInstance from FacesContext, 
        //so a mocked user can be returned for the test
        PowerMockito.mockStatic(FacesContext.class);
        PowerMockito.when(FacesContext.getCurrentInstance()).thenReturn(facesContext);
        when(facesContext.getExternalContext()).thenReturn(externalContext);
        when(externalContext.getSessionMap()).thenReturn(sessionMap);
    }

    @Test
    public void doLogoutTest() throws NoSuchFieldException, IllegalAccessException {
        assertNotNull(authorizationBean.getUser());
        assertEquals("/login.xhtml?faces-redirect=true", authorizationBean.doLogout());

        //Check through reflection if the private field user is now null
        Field userField = authorizationBean.getClass().getDeclaredField("user");
        userField.setAccessible(true);
        User testUser = (User) userField.get(authorizationBean); 
        assertNull(testUser);
    }

}
\$\endgroup\$

4 Answers 4

10
\$\begingroup\$

Personally I wouldn't do this.

The reason why is when your field change name, your test fails because the field is hardcoded there and with refactoring this isn't persisted to the test.

What should I do?

Normally your AuthorizationBeanTest is in the same package as your AuthorizationBean.
When this is correct you could use a protected method.

Example :

protected boolean isUserNull () {
    return user==null;
}

This is completely safe in your pojo, nobody could even acces the User object.
And outside the package no one will ever see this method.

The advantage is when you refactor user or the method name, your test is automatically updated.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks for your insight, I really like this solution and will be using this. I also learned something extra! I now also use the isUserNull function in AuthorizationBean, so it's not just there for test. That I didn't think of this. \$\endgroup\$ Nov 27, 2015 at 12:40
  • 1
    \$\begingroup\$ Glad we could help. \$\endgroup\$
    – chillworld
    Nov 27, 2015 at 12:53
  • 1
    \$\begingroup\$ @WesleyEgbertsen if using Guava then adding the @VisibleForTesting annotation can greatly help to clarify intent for future developers. \$\endgroup\$ Nov 27, 2015 at 13:58
  • 1
    \$\begingroup\$ "And outside the package no one will ever see this method." slight correction: Inheriting can still expose it, since the class isn't final. Omitting the protected will give it only package visibility \$\endgroup\$
    – blgt
    Nov 27, 2015 at 14:08
  • \$\begingroup\$ @blgt indeed, inheriting can expose it, no identifier would be better as then it's package private \$\endgroup\$
    – chillworld
    Nov 27, 2015 at 14:14
7
\$\begingroup\$

The answer from @chillworld answer your question but there is still some really smalls details in your code.

I like that you're using import static for the asserts statement. This leave the code free from the repetitive Assert.assertNull.

The problem is you're cluterring the code with comments that serve no purpose.

        //Mocking the static function getCurrentInstance from FacesContext, 
        //so a mocked user can be returned for the test
        PowerMockito.mockStatic(FacesContext.class);
        PowerMockito.when(FacesContext.getCurrentInstance()).thenReturn(facesContext);
        when(facesContext.getExternalContext()).thenReturn(externalContext);
        when(externalContext.getSessionMap()).thenReturn(sessionMap);

Your code is clean enough that you don't have to explain yourself with comment that are just what the code is doing. Reserve comments for when you need to take my attention to something special, something unusual that by reading the code I would not understand immediately.

\$\endgroup\$
6
\$\begingroup\$

You need to test that:

  • When DoLogOut is called, then invalidateSession() is called and the user is redirected to the login page.

  • And that when GetUser is called AFTER DoLogOut, that a new session is created

  • And that when GetUser is called again, then the same user is returned.

The value of the privete user field should NOT be tested, as it is only an internal implementation detail.

Also what should GetUser do if the user has not yet logged in?

\$\endgroup\$
0
2
\$\begingroup\$

Is it not okay. Every time you think you should use reflection in a unit test, there's something wrong in the design of your class. Difficult testing reflects bad design and easy testing reflects good design (most of the time).

I don't know much about JPA, so here are the premises to my answer :


FacesContext.getCurrentInstance().getExternalContext().getSessionMap().get("user");

This would return null if your session was invalidated by the logout method. Otherwise it returns the currently logged in user.


FacesContext.getCurrentInstance().getExternalContext().getUserPrincipal();

I honestly have no idea what this does..


userDao.findByEmail(principal.getName());

That's simple, no need for explanation


From what I understand, right now your getUser method has two responsabilities.

  1. "Log in" the user
  2. Find the user in a context

You should have a method to initialize a user (or log it in, whatever floats your boat :p) and one to retrieve the currently logged in user.

This way the user of the class has a clear understanding of what's happening "under the hood".

As I said, I don't know JPA and it makes my confidence over what I'm saying a little less high, but I know that using reflection in a unit test it bad. Usually it's private because it's an implementation detail. That's exactly what it is at the moment. Your user variable is useful to you and no one else. Your unit tests should test what is useful to your client. And if the unit test have a hard time doing it, your client will have a hard time using your code.

Overall, JPA or not : Reflection in unit test = bad unit test = bad design choice.

Think about refactoring your class to change this and I'm sure one day you'll think "Darn, that guy from Code Review was right, I would've been stuck if I had keep the implementation as it was."

Otherwise, @chillworld's answer is good, that's something you can do. But I wouldn't encourage it for such a trivial case! :)

\$\endgroup\$
8
  • \$\begingroup\$ This would return null if your session was invalidated by the logout method. Otherwise it returns the currently logged in user. Maybe not, since the log in method may just initialize the Principal the user for the authentication framework. The user in the session correspond to the User that he mapped with the Principal to logged in the user. So you have User which is from the business domain and Principal is the "technical" user. \$\endgroup\$
    – Marc-Andre
    Nov 27, 2015 at 15:53
  • \$\begingroup\$ @Marc-Andre By "log in method", you mean the current getUser or another hypothetical method? And I don't know what's the Principal, so it doesn't help me to understand what you're saying ahah \$\endgroup\$
    – IEatBagels
    Nov 27, 2015 at 15:56
  • \$\begingroup\$ No what I meant is the logged in method of the framework is setting the Principal for you, it will not set the User object of your domain. So when you do call getUser() user is maybe not in the session. Principal -> docs.oracle.com/javase/6/docs/api/java/security/Principal.html a Principal is a term used to represent someone or something in regards to log in. \$\endgroup\$
    – Marc-Andre
    Nov 27, 2015 at 15:58
  • \$\begingroup\$ I am using a security realm in glassfish so all the protected pages that use the AuthorizationBean will never be accessed if the user doesn't log in through the security container. So the getUser function is really a way to show the user his information for example, his username in the bar. That's why the bean is also SessionScoped. The principal is in my application the email of an user. \$\endgroup\$ Nov 27, 2015 at 16:04
  • 1
    \$\begingroup\$ It's not specifically realated to the web libraries. For now I will use @chillworld's answer. If someday a blinking light appears above my head, I will remember your post. \$\endgroup\$ Nov 27, 2015 at 16:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.