-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
review: test: add tests for substitution class #4018
review: test: add tests for substitution class #4018
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.
These tests are a little bit tell-tale of the "chasing coverage" problem I've mentioned a few times. They don't really test the core functionality of these methods. Your tests are designed to essentially test the methods as getters (as the contracts say). But the substitute
methods are not plain getters, but rather as the name implies, they are meant to perform substitutions.
I would urge you to carefully study the templating documentation to get a better idea of what the purpose of these methods is.
It is a bit hard to understand how to use these things. To get you started, here's one complete test of substituteStatement
that meaningfully tests the actual substitution.
@Test
public void testSubstituteStatementWithTemplatedInitializer() {
// contract: Given a statement with a templated initializer, substituteStatement should
// return a new statement with the initializer replaced with the value bound to the template
// parameter
// arrange
Factory factory = createFactoryWithTemplates();
CtClass<?> targetClass = factory.Class().create("TargetClass");
String templateExecutableName = "executable";
int templateVariableIndex = 0;
String templateVariableName = "s";
String initializerToSubstitute = "My chosen initializer";
CtStatement expectedStatement = factory.createLocalVariable(
factory.Type().stringType(), templateVariableName, factory.createLiteral(initializerToSubstitute)
);
StatementWithTemplatedInitializer template = new StatementWithTemplatedInitializer();
template._initializer_ = factory.createLiteral(initializerToSubstitute);
// act
CtStatement substitutedStatement = Substitution.substituteStatement(targetClass, template, templateVariableIndex, templateExecutableName);
// assert
assertEquals(expectedStatement, substitutedStatement);
}
private static class StatementWithTemplatedInitializer extends ExtensionTemplate {
TemplateParameter<String> _initializer_;
public void executable() {
String s = _initializer_.S();
}
}
To be clear, I would like you to rewrite these tests to actually test some form of substitution.
Thanks for sharing this, I will go through this.
I am glad that you showed me how to correctly test this. I will rewrite tests after going through the resources that you shared |
Should I refactor the wrongly extracted test (that I extracted in #4016) in this PR only ? |
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.
Now the tests are much better! Some minor comments on this revision, see code.
Co-authored-by: Simon Larsén <slarse@kth.se>
Co-authored-by: Simon Larsén <slarse@kth.se>
Co-authored-by: Simon Larsén <slarse@kth.se>
As all the suggestions are resolved for tests, should I refactor the wrongly extracted test (that I extracted in #4016) |
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.
Nice, LGTM!
As all the suggestions are resolved for tests, should I refactor the wrongly extracted test (that I extracted in #4016)
Let's keep this PR clean and to the point, and move that test in a separate PR.
Co-authored-by: Simon Larsén <slarse@kth.se>
#1818
Hi, I have added 3 new tests for methods of the
Substitution
class :testSubstituteMethodBody
should kill this mutation :testSubstituteStatement
should kill this mutation :testSubstituteFieldDefaultExpression
should kill this mutation :Link to #3967