test: improve the test for SpoonTagger class #3982
Conversation
|
Just FYI I will get back to reviewing your PRs tomorrow, was a bit busy with other things today :) |
Yeah no problem |
There was a problem hiding this comment.
This is a good find!
For tests that require that require a "clean slate" when running, it's better to use a temporary directory instead of deleting select files. You can easily create a temporary directory with JUnit5 like so:
@Test
public void testSpoonTagger(@TempDir File tempDir) {
// contract: ...
Launcher launcher = new Launcher();
launcher.setSourceOutputDirectory(tempDir.getAbsolutePath());
// do the test here
}| @@ -87,8 +87,13 @@ public void testRuntimeProcessorManager() { | |||
|
|
|||
| @Test | |||
| public void testSpoonTagger() { | |||
| // contract: this tests the process method of the SpoonTagger class. | |||
There was a problem hiding this comment.
Like I mentioned in the other review, a contract should state what is expected to happen given what the test does. An appropriate contract here would be: contract: after running SpoonTagger, the file spoon/Spoon.java should exist in the output directory
There was a problem hiding this comment.
Thanks for correcting the contract, I will make sure that future test contracts are more appropriate.
| final Launcher launcher = new Launcher(); | ||
| launcher.addProcessor("spoon.processing.SpoonTagger"); | ||
| // we need to make sure that the spoon/Spoon.java file doesn't already exist in the system, as it will cause the assertion to be irrelevant |
There was a problem hiding this comment.
This is an example of a good comment: it says why we do something.
| final Launcher launcher = new Launcher(); | ||
| launcher.addProcessor("spoon.processing.SpoonTagger"); | ||
| // we need to make sure that the spoon/Spoon.java file doesn't already exist in the system, as it will cause the assertion to be irrelevant | ||
| File file = new File (launcher.getModelBuilder().getSourceOutputDirectory() + "/spoon/Spoon.java"); | ||
| // deleting the Spoon.java if it exists |
There was a problem hiding this comment.
This is an example of a bad comment: it just restates what the code does. The code already says that.
There was a problem hiding this comment.
Thanks for pointing out, I will avoid redundant comments in future
There was a problem hiding this comment.
Note that as with any rule of thumb, there are exceptions. Sometimes we simply fail to write code that is clear enough to be understood, and then we need to put a comment in to clarify. That wasn't the case here though, the code was very clear.
Thanks for letting me know this, I will keep this in my mind for future tests |
|
@Rohitesh-Kumar-Jain Given your responses I'm not sure it was clear, but my comments here were not meant only for future PRs, but for this one as well! You can see it as "changes requested" on the PR review. Please address the comments :) |
Ik this is meant for this PR only, I was saying that I will keep these things in my mind for future PRs. |
I understood that. What I did not know for sure was whether you understood that I meant for the comments to be fixed in this PR, and not only as suggestions for future PRs, and so I clarified the fact. |
|
@Rohitesh-Kumar-Jain This looks good to me, are you done here? |
Yes, this was a small class and I believe all 3 Descartes parameters have been improved to 100% 🎉 |
There was a problem hiding this comment.
Yes, this was a small class and I believe all 3 Descartes parameters have been improved to 100%
That's excellent!
@monperrus ping for merge, one pseudo-tested method defeated!
| @@ -86,8 +87,10 @@ public void testRuntimeProcessorManager() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testSpoonTagger() { | |||
| public void testSpoonTagger(@TempDir File tempDir) { | |||
| // contract: after running SpoonTagger, the file spoon/Spoon.java should exist in the output directory | |||
There was a problem hiding this comment.
@Rohitesh-Kumar-Jain By the way, this is a well specified contract. Nice.
monperrus
changed the title
Hurray, that's an achievement, per the plan pf GSOC! @Rohitesh-Kumar-Jain thanks a lot for your patience and determination to understand what a good test is and how to improve Spoon's test suite. @slarse thanks a lot for the mentoring |
Hi, I have improved the mutation strength from 0% to 100% for the SpoonTagger class.
Here's the report before improvement.
Actually, the test was poorly written, it had overlooked the scenario when the Spoon.java file already exists. I have taken care of that scenario and have manually tested the mutation "Removed all instructions in the method", and it is getting killed now.
I feel the PR is ready for merging.