I'm a student studying software development (1st year) and the teaching language we are using is Java. We have covered basics, and most of OOP, but I've been practicing making a Shop Administration System and I've come up against something I can't reckon with.
I'm trying to unit test two classes which are both abstract superclasses of several other classes I plan on implementing, as per the UML below
Person Superclass and Employee subclass - both abstract
I've read through a series of posts on here and I see a lot of people were recommending things like power mock and mockito for making mock objects. I'm probably trying to learn too much at once as it is but basically I landed on concrete "wrapper" private classes in the unit test class that i used to polymorphically create the Employee objects (technically EmployeeWrapper objects), then unit testing all the public methods through the wrapper class.
I'm vaugely familiar with the term "bad code smell" and this really stinks. Is there a standard way of testing abstract superclasses without using things like Mockito and Power Mock? Or do i just need to suck it up and use things like that?
This is the code for the classes (with all method bodies removed so you dont have to read through a load of unimportant details
import java.time.LocalDateTime;
import java.util.Hashtable;
import java.util.Iterator;
public abstract class Employee extends Person {
private double hourlyRate;
private double hoursPerWeek;
private LocalDateTime dateOfEmploymentStart;
private LocalDateTime dateOfEmploymentEnd;
private Hashtable<LocalDateTime, Integer> shifts;
private static final double MINIMUM_WAGE = 8.0;
/**
* Constructor for Employee for all fields except dateOfHire, which is set to {@code LocalDateTime.now()}
*
* @param name
* @param email
* @param phoneNumber
* @param hourlyRate
* @param weeklyHours
* @throws IllegalArgumentException if name if blank or null
*/
public Employee(String name, String email, String phoneNumber, double hourlyRate, double weeklyHours) throws IllegalArgumentException {
super(name, email, phoneNumber);
this.setHourlyRate(hourlyRate);
this.setWeeklyHours(weeklyHours);
this.setDateOfEmploymentStart(LocalDateTime.now());
this.shifts = new Hashtable<LocalDateTime, Integer>();
}
/**
* Constructor for Employee that sets name, email and phoneNumber to provided args; and sets hourly rate and weeklyHours to 0
*
* @param name
* @param email
* @param phoneNumber
* @throws IllegalArgumentException if name is blank or null
*/
public Employee(String name, String email, String phoneNumber) throws IllegalArgumentException {
this(name, email, phoneNumber, MINIMUM_WAGE, 0);
}
/**
* Constructor for Employee that sets only name
*
* @param name
* @throws IllegalArgumentException
*/
public Employee(String name) throws IllegalArgumentException {
this(name, null, null);
}
}
and the Unit test class (with all test cases bar one removed, and that one method body is left empty - again to stop clutter:
import static org.junit.jupiter.api.Assertions.*;
import java.time.LocalDateTime;
import java.util.Hashtable;
import java.util.Set;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
class EmployeeTest {
private class EmployeeWrapper extends Employee {
public EmployeeWrapper(String name, String email, String phoneNumber, double hourlyRate, double weeklyHours) throws IllegalArgumentException {
super(name, email, phoneNumber, hourlyRate, weeklyHours);
}
public EmployeeWrapper(String name, String email, String phoneNumber) throws IllegalArgumentException {
super(name, email, phoneNumber);
}
public EmployeeWrapper(String name) throws IllegalArgumentException {
super(name);
}
}
private String nameValid, emailValid, phoneNumberValid;
private String nameInvalid, emailInvalid, phoneNumberInvalid;
private double hourlyRateValid, hourlyRateInvalidLow;
private double weeklyHoursValid, weeklyHoursInvalid;
private final double DEFAULT_HOURLY_RATE = 8;
private final double DEFAULT_WEEKLY_HOURS = 0;
private final String DEFAULT_EMAIL = "no email provided";
private final String DEFAULT_PHONE_NUMBER = "no phone number provided";
private final double MINIMUM_WAGE = 8.0;
private Employee employee;
private Hashtable<LocalDateTime, Integer> shiftsValid, shiftsInvalidEmpty;
private LocalDateTime dateTimeValid, dateTimePast, dateTimeFuture;
@BeforeEach
void setUp() throws Exception {
// valid employee
nameValid = "testname";
phoneNumberValid = "123456789";
emailValid = "[email protected]";
hourlyRateValid = 10.50;
weeklyHoursValid = 7.5;
employee = new EmployeeWrapper(nameValid, emailValid, phoneNumberValid, hourlyRateValid, weeklyHoursValid);
// test data
nameInvalid = "";
emailInvalid = "[email protected]";
phoneNumberInvalid = "";
hourlyRateInvalidLow = 5;
weeklyHoursInvalid = -10;
dateTimeValid = LocalDateTime.of(2015, 6, 15, 13, 30);
dateTimePast = LocalDateTime.MIN;
dateTimeFuture = LocalDateTime.MAX;
shiftsValid = new Hashtable<LocalDateTime, Integer>();
shiftsValid.put(dateTimeValid, 6);
shiftsValid.put(dateTimeFuture, 3);
shiftsInvalidEmpty = new Hashtable<LocalDateTime, Integer>();
}
@Test
void testEmployeeConstructorValidAllArgs() {
}
}
This is my first post of Stack Overflow so i apologise profusely if i have omitted any revelent details.
If you see any other stupid things i've done in the code I'll also gladly take any criticism!
edit: thanks everyone for the responses they have been amazing, i really really appreciate it!
First let me say, that your approach is absolutely viable. I am just sharing my own way of doing it, because it spares copy pasting tests common between different implementations.
I don't specifically test abstract classes. Because we are testing functionality and it can be overridden in subclasses. I'll use your Person class for this setup, but i will simplify it a bit.
public abstract class Person {
private String name;
private String email;
public Person(String name, String email) {
this.setName(name);
this.email = email;
}
public String getName() {
return this.name;
}
public void setName(String name) {
if (name == null || name.isEmpty()) {
throw new IllegalArgumentException("missing name");
}
this.name = name;
}
public String getEmail() {
return this.email;
}
public void setEmail(String email) {
this.email = email;
}
}
Student
public class Student extends Person {
private String university;
public Student(String name, String email, String university) {
super(name, email);
this.university = university;
}
public String getUniversity() {
return this.university;
}
public void setUniversity(String university) {
this.university = university;
}
}
Child
public class Child extends Person {
private String school;
public Child(String name, String email, String school) {
super(name, email);
this.school = school;
}
public String getSchool() {
return this.school;
}
public void setSchool(String school) {
this.school = school;
}
@Override
public String getName() {
return "I am not saying!";
}
}
So we have the abstract Person, a Student, whose specific thing is a university and a Child. Having a school is what is specific for the child, but it also changes the behaviour of getName(), it does not disclose its name. This might be desired, but for this example we'll assume it was incorrect to override getName() like this.
When dealing with abstract classes i make an abstract test class, which holds common setup and tests for common functionality provided by the abstract class - Person in this case.
public abstract class PersonBaseTests {
protected static final String EXPECTED_NAME = "George";
private Person person;
@BeforeEach
public void setUp() {
this.person = getConcretePersonImplementation();
}
/**
* @return new instance of non-abstract class extending person
*/
protected abstract Person getConcretePersonImplementation();
//common tests
@Test
public void testGetName_ShouldReturnCorrectValue() {
assertEquals(EXPECTED_NAME, this.person.getName());
}
@Test
public void testConstructor_ShouldThrowIllegalArgumentExceptionOnMissingName() {
Executable invalidConstructorInvocation = getConstructorExecutableWithMissingName();
IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, invalidConstructorInvocation);
assertEquals("missing name", exception.getMessage());
}
protected abstract Executable getConstructorExecutableWithMissingName();
//other common tests
}
The test classes extending the base must provide the concrete implementation to be tested. They will also inherit the tests, so you don't need to write them again. If you still have not learned about interfaces, lambdas and stuff like that, you can ignore the constructor exception test and everything related to it, and focus on getName() test. It tests that the getter correctly returns the name of the Person. This will obviously fail for Child, but that's the idea. You can add tests for getting and setting email, phone, etc.
So, student tests
public class StudentTests extends PersonBaseTests {
@Override
protected Person getConcretePersonImplementation() {
return new Student(PersonBaseTests.EXPECTED_NAME, "mail", "Cambridge");
}
@Override
protected Executable getConstructorExecutableWithMissingName() {
//setup invocation which will actually fail
return new StudentConstructorExecutable(null, "[email protected]", "Stanford");
}
private static final class StudentConstructorExecutable implements Executable {
private final String name;
private final String email;
private final String university;
private StudentConstructorExecutable(String name, String email, String university) {
this.name = name;
this.email = email;
this.university = university;
}
@Override
public void execute() throws Throwable {
//this will invoke the constructor with values from fields
new Student(this.name, this.email, this.university);
}
}
//write tests specific for student class
//getUniversity() tests for example
}
Again, ignore the Executable and everything related to the constructor test, if you have not learned it yet. Student tests provide concreete instance of Student for the common inherited tests, and you can write additional tests for specific functionality - get/set university.
Child tests
public class ChildTests extends PersonBaseTests {
@Override
protected Person getConcretePersonImplementation() {
return new Child(PersonBaseTests.EXPECTED_NAME, "", "some school");
}
@Override
protected Executable getConstructorExecutableWithMissingName() {
//this can be ignored
return () -> new Child(null, "", "");
}
//write tests specific for child class
//getSchool() tests for example
}
Again, a concrete instance is provided for the common tests - this time of type Child. And you can add tests for any additional functionality provided by Child class - get and set school in this example. Then you can write more test classes for every additional subclass of Person.
Like this you keep common tests at one place and every concrete implementation of abstract class you write is completely tested, without test duplication. About the failing test, if the change in behaviour of getName() is intentional, you can just override it in ChildTests to take that into account. If it is not intentional, you know, that Student.getName() is correct, while Child.getName() is not, but you wrote the test only once.