Problem
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);
}
}
Solution
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.
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.
You need to test that:
-
When
DoLogOutis called, theninvalidateSession()is called and the user is redirected to the login page. -
And that when
GetUseris called AFTERDoLogOut, that a new session is created -
And that when
GetUseris 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?
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.
- “Log in” the user
- 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! 🙂