-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Smell free tests #459
base: master
Are you sure you want to change the base?
Smell free tests #459
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -37,7 +38,9 @@ | |||
* | |||
* @author Gordon Fraser | |||
*/ | |||
public class Mutation implements Comparable<Mutation> { | |||
public class Mutation implements Comparable<Mutation>, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fc51111, why does the Mutation
class need to be Serializable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final BytecodeInstruction original; | ||
private final transient BytecodeInstruction original; | ||
|
||
private final InsnList mutation; | ||
private final transient InsnList mutation; | ||
|
||
private final InsnList infection; | ||
private final transient InsnList infection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do all these fields need to be transient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had to make the "Mutation" class Serializable. In turn, we made these 3 fields transient because the respective classes were not Serializable (and we decided that it didn't make sense to make them Serializable).
protected transient Mutation mutation; | ||
protected Mutation mutation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does not the mutation
field need to be transient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had to make the "Mutation" class Serializable, so the "mutation" field does not need to remain transient.
|
||
return c1 - c2; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I can't remove this. We made this change because we decided that it would be better to normalize the secondary criteria. In our case, we wanted the smelliness of each test smell to vary between 0 and 1.
jose/smell-free-tests-evosuite@677f74d
Regarding "RhoTestSuiteSecondaryObjective.java", instead of just returning 0 or 1, we decided to calculate the smelliness of each test suite, normalize the respective values, and return the difference between the two. We calculate the difference because we considered that a test suite that is a lot less smelly than the other should have more weight than a test that is just a bit less smelly than another.
Removing this would most likely break our test smell optimization process. At the very least, there would be some unexpected results...
Is this change likely to break the rest of the code? Or are there any chances that we can keep it like this?
import java.util.LinkedHashMap; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather keep the verbose imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -75,7 +75,7 @@ public class DefaultTestCase implements TestCase, Serializable { | |||
/** | |||
* Coverage goals this test covers | |||
*/ | |||
private transient Set<TestFitnessFunction> coveredGoals = new LinkedHashSet<>(); | |||
private final Set<TestFitnessFunction> coveredGoals = new LinkedHashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does not the coveredGoals
need to be transient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"TestRedundancy.java" (i.e., one of the implemented test smells) uses the method "getCoveredGoals()" of "DefaultTestCase.java":
Set<TestFitnessFunction> testGoals = testChromosome.getTestCase().getCoveredGoals();
We were having problems because "testGoals" was null. In turn, this was happening because "coveredGoals" (a field in "DefaultTestCase.java") was transient, hence the change.
@@ -34,7 +34,8 @@ public abstract class ExecutableChromosome<E extends ExecutableChromosome<E>> ex | |||
|
|||
private static final long serialVersionUID = 1L; | |||
|
|||
protected transient ExecutionResult lastExecutionResult = null; | |||
//protected transient ExecutionResult lastExecutionResult = null; | |||
protected ExecutionResult lastExecutionResult = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does not the lastExecutionResult
need to be transient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same explanation as "ExecutionResult.java"
import java.util.*; | ||
import java.util.stream.IntStream; | ||
|
||
public class ExecutionResult implements Cloneable { | ||
public class ExecutionResult implements Cloneable, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the ExecutionResult
class need to be Serializable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class "RottenGreenTests" (i.e., one of the implemented test smells) has the following line of code:
ExecutionResult lastExecutionResult = chromosome.getLastExecutionResult();
where "chromosome" is of type "TestChromosome". Originally, in "ExecutableChromosome.java" (where the "getLastExecutionResult()" method is implemented), we had the following field:
protected transient ExecutionResult lastExecutionResult = null;
We were having problems because the variable "lastExecutionResult" in "RottenGreenTests.java" was null. Therefore, it was necessary to make 2 changes:
1 - In "ExecutableChromosome.java", remove "transient" from the field:
protected ExecutionResult lastExecutionResult = null;
2 - Make ExecutionResult Serializable
This pull request extends EvoSuite with new secondary criteria that optimize test smell metrics. The paper Automatic Generation of Smell-free Unit Tests published at SBFT'23 provides more details on that.