test: unite all tests for Substitution in one place #4016
Conversation
Rohitesh-Kumar-Jain
changed the title
monperrus
changed the title
|
Thanks @Rohitesh-Kumar-Jain |
| @Test | ||
| public void testSimpleTemplate() { | ||
| Launcher spoon = new Launcher(); | ||
| spoon.addTemplateResource(new FileSystemFile("./src/test/java/spoon/test/template/testclasses/SimpleTemplate.java")); | ||
| spoon.buildModel(); | ||
|
|
||
| Factory factory = spoon.getFactory(); | ||
|
|
||
| CtClass<?> testSimpleTpl = factory.Class().create("TestSimpleTpl"); | ||
| //whitespace seems wrong here | ||
| new SimpleTemplate("HelloWorld").apply(testSimpleTpl); | ||
|
|
||
| Set<CtMethod<?>> listMethods = testSimpleTpl.getMethods(); | ||
| assertEquals(0, testSimpleTpl.getMethodsByName("apply").size()); | ||
| assertEquals(1, listMethods.size()); | ||
| } |
There was a problem hiding this comment.
This is not a direct test of Substitution.java, strictly speaking it should not have been moved. This is a higher-level test on a whole template.
There was a problem hiding this comment.
@slarse I thought it is was probably meant for testing Substitution.insertAll(), but was testing Substitution.insertAllMethods() only. That was the use case of the test I got after going through it.
There was a problem hiding this comment.
The method under test here is StatementTemplate.apply(). While that method in turn uses Substitution, it also uses a bunch of other classes and methods, so saying that this is a test of the Substitution class in isolation is a stretch of the imagination.
This is an end-to-end test of using a template, and so is perfectly suitable to be located in TemplateTest.
There was a problem hiding this comment.
I'm sorry. I hurried to push the PR, I see that this extraction was wrong and unnecessary. I will be more sincere next time, it won't happen again, and I will fix this soon in the other PR which is already open for this class.
There was a problem hiding this comment.
You don't need to be sorry, I just noted that moving this seemed a bit off and made a note of it. A PR in Spoon is (at least) a two-person job, and both you and @monperrus missed this detail in this case. And that's OK, we can fix it later.
it won't happen again
It might happen again, and that's fine. We all make mistakes from time to time, especially when the "correct" solution isn't clear-cut (which with testing it rarely is).
As I created a new class
SubstitutionTestand merged 4 new tests in it, I thought I should extract two existing tests so all tests can be together which were Specifically written for theSubstitutionclass.Now all tests related to this class are at the same place
link to #3967