review: test: add test for insertAllFields method #3995

Rohitesh-Kumar-Jain · 2021-06-16T12:59:23Z

Hi, I have added a new test for insertAllFilelds method of the Substituion class, this test improves code coverage, mutation converge and test strength all from 0% to 100%.

Here's the report before improvement.

I have manually tested that this test is killing that mutation.

Under the hood, it is testing more code,

so I will run Descartes again when the iterations for this PR will end and this gets merged. Until then I believe I should work on some other poorly tested classes suggested by @slarse?

Link to #3967

slarse

Hi @Rohitesh-Kumar-Jain,

It's great that your testing the Substitution class, it looks woefully undertested.

The PR needs some work, however. I've added quite a lot of comments in the code itself. In addition, I think this also calls for creating a new test class, SubstitutionTest, because we will need to add more tests for that class in particular.

Ping me when you're done :)

slarse · 2021-06-17T06:33:03Z

src/test/java/spoon/test/template/testclasses/InsertAllFieldsTemplate.java

+package spoon.test.template.testclasses;
+
+import spoon.template.Local;
+import spoon.template.Parameter;
+import spoon.template.StatementTemplate;
+
+public class InsertAllFieldsTemplate extends StatementTemplate {
+    @Parameter
+    String _parameter_;
+    String testString = "goodName";
+
+    @Local
+    public InsertAllFieldsTemplate(String parameter) {
+        _parameter_ = parameter;
+    }
+
+    @Override
+    public void statement() {
+        System.out.println(_parameter_);
+    }
+}


Define this template right after the test instead, as a nested type in the test file. This is a crucial part of the test so we want to keep it close.

Also, consider its name. Does this template really insert all fields? I think a name such as SingleFieldTemplate is more descriptive.

Finally, the template does not align with the test. From what I can surmise of the contract and test, what you're testing is that the single field testString is inserted. To test just that, you should create a minimal template for testing just that.

public class InsertAllFieldsTemplate extends StatementTemplate { String testString = "goodName"; @Override public void statement() { } }

Good catch 😅

src/test/java/spoon/test/template/TemplateTest.java

slarse · 2021-06-17T07:07:08Z

src/test/java/spoon/test/template/TemplateTest.java

@@ -900,6 +901,28 @@ public void testSubstituteInnerClass() {
 		assertEquals("x.Result$Inner",fr.getDeclaringType().getQualifiedName());
 	}

+	@Test
+	public void testSubstitutionInsertAllFields() {
+		// contract: insertAllFields method of the Substitution class inserts all the fields from a given template by substituting all the template parameters by their values.


This contract does not adequately describe the test. What actually happens in this test is that a single field, String testString = "goodName"; is inserted into the created class. There is no substitution going on, there is only a single as-is field insertion.

A more apt description would be something like "Substitution.insertAllFields inserts the only field from a single-field template into the target class"

Additionally, the test actually only verifies that the field has the correct name, so it doesn't verify the field as a whole.

Yes, I have changed the contract, I thoughtt I was supposed to write what the method is doing in the contract, than what is test is supposed to do.

test actually only verifies that the field has the correct name, so it doesn't verify the field as a whole.

Yes, I have added another assertion for checking the assignment of the field.

The contract should state any relevant preconditions for the test, method/class/etc is being tested, and what the expected out come is. How one gets from the preconditions to the expected outcome is an implementation detail a test in general is not concerned with :)

Sometimes it may be necessary to test an implementation detail, especially when testing corner cases or reproducing bugs. Then it may be relevant to explain such things in the contract. So, as with everything, this is not a rule set in stone, but rather a general guideline.

Assume that pretty much all of my feedback is either a general guideline, or just my opinion, or both.

Co-authored-by: Simon Larsén <[email protected]>

Rohitesh-Kumar-Jain · 2021-06-17T12:48:25Z

I think this also calls for creating a new test class, SubstitutionTest

Yes, I have created a new test class for SubstitutionTest, and the PR is once again ready to review @slarse.

slarse

Hi @Rohitesh-Kumar-Jain,

So, this is good, but I think it can be even better. I also think that my initial review was a bit unclear and blabbery, and had it been better this would have been ready to merge now. See the review comments for more details.

An additional note: I think this is a test that could benefit from arrange, act and assert comments, because it's not entirely clear at first glance which phase is where, especially when it comes to arrange and act, as the arrangement is so large.

src/test/java/spoon/test/template/SubstitutionTest.java

slarse · 2021-06-17T14:07:54Z

src/test/java/spoon/test/template/SubstitutionTest.java

+        List<String> actualFieldNames = targetType.getFields().stream()
+                .map(CtField::getSimpleName)
+                .collect(Collectors.toList());
+        // verifies that the field has correct name


This is the kind of comment that I would recommend you to avoid: it just says what the code does. The assertion is entirely clear on its own and should not have a comment explaining what it asserts.

slarse · 2021-06-17T14:10:12Z

src/test/java/spoon/test/template/SubstitutionTest.java

+    public class SingleFieldTemplate extends StatementTemplate {
+        String testString = "goodName";
+
+        @Override
+        public void statement() { }
+    }


This is a personal preference of mine, and others may think otherwise, but I would put this below the test. I like to read files top-down, so if a method A uses a method or class B which is defined in the same file, then A is defined before B.

Again, I think this is a matter of taste, and you should form your own opinion. If you agree, then move the class down. If you disagree, feel free to keep it where it is.

I will place it downwards with no issues, actually, the first programming language that I learnt was C++, and in that one should declare the function at the top of the file calling the function, so probably I have this habit from there.

Totally understand, and as I said it's a matter of taste. Some prefer declaring functions before they are used, while others like me prefer to declare them just after they are used. There's no right or wrong, but in Java I find that the latter is more common.

src/test/java/spoon/test/template/SubstitutionTest.java

Co-authored-by: Simon Larsén <[email protected]>

Rohitesh-Kumar-Jain · 2021-06-17T17:10:28Z

@slarse feel free to suggest changes for 3rd iteration if the PR needs improvement. I understand the purpose of GSoC is not to churn a lot of code, but to merge quality code in production and for students to learn new things and change their old habits : )

slarse

@slarse feel free to suggest changes for 3rd iteration if the PR needs improvement. I understand the purpose of GSoC is not to churn a lot of code, but to merge quality code in production and for students to learn new things and change their old habits : )

This test is now of much higher quality than most other tests in Spoon (including tests I've written myself). I can't speak for the purpose of GSoC as I'm not officially involved, but the code quality here is very good. You're learning, and my reviews will grow shorter and shorter as you do.

add test for insertAllFields method

cc5af44

slarse suggested changes Jun 17, 2021

View reviewed changes

Rohitesh-Kumar-Jain and others added 2 commits June 17, 2021 14:21

improve assertion

5e7aa0b

Co-authored-by: Simon Larsén <[email protected]>

implement suggestions received in review

cb9bbc0

slarse suggested changes Jun 17, 2021

View reviewed changes

slarse reviewed Jun 17, 2021

View reviewed changes

src/test/java/spoon/test/template/SubstitutionTest.java Outdated Show resolved Hide resolved

Rohitesh-Kumar-Jain and others added 3 commits June 17, 2021 22:11

Update src/test/java/spoon/test/template/SubstitutionTest.java

33bc577

Co-authored-by: Simon Larsén <[email protected]>

Update src/test/java/spoon/test/template/SubstitutionTest.java

bfd8b98

Co-authored-by: Simon Larsén <[email protected]>

implement suggestions received in iteration-2

d9e2eaa

slarse approved these changes Jun 18, 2021

View reviewed changes

slarse merged commit 9d43ea3 into INRIA:master Jun 18, 2021

Rohitesh-Kumar-Jain deleted the add-test-for-insertallfields branch June 18, 2021 06:23

Rohitesh-Kumar-Jain mentioned this pull request Jun 21, 2021

track progress of @Rohitesh-Kumar-Jain - GSoC 2021 student #3967

Closed

monperrus mentioned this pull request Aug 19, 2021

release Spoon 9.1.0 #4104

Merged

woutersmeenk pushed a commit to woutersmeenk/spoon that referenced this pull request Aug 29, 2021

test: add test for insertAllFields method (INRIA#3995)

78b7c60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review: test: add test for insertAllFields method #3995

review: test: add test for insertAllFields method #3995

Rohitesh-Kumar-Jain commented Jun 16, 2021

slarse left a comment

slarse Jun 17, 2021

slarse Jun 17, 2021

Rohitesh-Kumar-Jain Jun 17, 2021

slarse Jun 17, 2021

Rohitesh-Kumar-Jain Jun 17, 2021

slarse Jun 17, 2021

Rohitesh-Kumar-Jain commented Jun 17, 2021

slarse left a comment

slarse Jun 17, 2021

slarse Jun 17, 2021

Rohitesh-Kumar-Jain Jun 17, 2021

slarse Jun 18, 2021

Rohitesh-Kumar-Jain commented Jun 17, 2021

slarse left a comment

review: test: add test for insertAllFields method #3995

review: test: add test for insertAllFields method #3995

Conversation

Rohitesh-Kumar-Jain commented Jun 16, 2021

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohitesh-Kumar-Jain commented Jun 17, 2021

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohitesh-Kumar-Jain commented Jun 17, 2021

slarse left a comment

Choose a reason for hiding this comment