test: improve tests for NameFilter class. #3975

Rohitesh-Kumar-Jain · 2021-06-04T06:31:07Z

This PR aims to improve the code coverage of the class NameFilter.

Link to #3967

Rohitesh-Kumar-Jain · 2021-06-04T06:34:30Z

I have added one additional test to improve the line coverage of the tests for NameFilter class.

I would be glad if @slarse or maybe someone else may tell me what should be the scope for a single PR and a commit for improving tests.

slarse · 2021-06-04T06:56:52Z

what should be the scope for a single PR and a commit for improving tests.

There's no easy answer to that, it depends. Arguably, a PR can't really be too small, so there's really no minimum scope. Somewhat arbitrarily we can say that a maximum scope would be adding tests relating to a single public method. That is to say, don't target multiple pieces of functionality with a single PR.

monperrus · 2021-06-04T08:01:28Z

In the meeting @slarse suggested it is not necessary to run Descartes all the tim

Fully agree. You can run Descartes every other day, in order to:

verify that the new merged tests indeed improve the Descartes metrics
select your next test target.

slarse · 2021-06-04T09:02:39Z

@Rohitesh-Kumar-Jain See my comment in #3967. For measuring code coverage, using Descartes is incredibly inefficient.

You can run Descartes every other day, in order to:

Considering that it takes @Rohitesh-Kumar-Jain 20 hours to run Descartes on Spoon, I wouldn't recommend running it more than once a week.

Rohitesh-Kumar-Jain · 2021-06-04T09:18:33Z

Considering that it takes @Rohitesh-Kumar-Jain 20 hours to run Descartes on Spoon, I wouldn't recommend running it more than once a week.

Yes it keeps running for 20 hours in the background, my effort is to just run this, but still, I have to wait almost a day to see the new report. Although Descartes can be run with subsets however in this case the reports aren't as expected after targeting the exact class and the test.

slarse · 2021-06-04T09:24:33Z

Yes it keeps running for 20 hours in the background, my effort is to just run this, but still, I have to wait almost a day to see the new report. Although Descartes can be run with subsets however in this case the reports aren't as expected after targeting the exact class and the test.

That's most likely because there are other tests that exercise the code you target. A large part of Spoon's test code is not unit tests, but rather run the entire thing from parsing a model to printing a result, and so there can be many tests that touch e.g. NameFilter without them being called anything relating to Filter.

Rohitesh-Kumar-Jain · 2021-06-04T11:27:38Z

That's most likely because there are other tests that exercise the code you target. A large part of Spoon's test code is not unit tests, but rather run the entire thing from parsing a model to printing a result, and so there can be many tests that touch e.g. NameFilter without them being called anything relating to Filter.

Thanks for letting me know this

Rohitesh-Kumar-Jain · 2021-06-05T11:29:28Z

Hi,

I feel this line of code assertEquals("Foo", foo.getSimpleName()); is redundant in testNameFilter method, as this assertion has nothing to do with NameFilter class, also this assertion has been called for about 11 more times in other 6 files, so I believe removing this line of won't reduce test quality and is redundant ?

looping in @nharrand

Rohitesh-Kumar-Jain · 2021-06-06T09:31:20Z

Hi team,

Now there are two options available :

I have extracted the NameFilterTest form FilterTest in review: test: extract NameFilterTest from FilterTest #3978, so after the extraction is merged I can close this PR and raise a new one for improving the test in the NameFilterTest.
If we don't wish to extract, we can close review: test: extract NameFilterTest from FilterTest #3978, and simply just focus on this PR.

slarse · 2021-06-07T06:46:00Z

Hi @Rohitesh-Kumar-Jain,

As one point of feedback, avoid working on things in parallel that conflict with each other. If either this or #3978 is merged, there will be a merge conflict between them.

With that said, this PR needs a little bit of work. When it comes to tests, it's important that each test tests one thing, one concept. That doesn't necessarily mean that each test needs to have precisely one assert, but it does mean that you shouldn't test happy-path behavior (everything works as it should) and crashes in the same test. Having a lot of assertions in the same test is also indicative that it's testing too many concepts.

As a rule of thumb, you should only call the method under test once in the test itself. This doesn't always hold (sometimes you specifically want to test what happens if you call it twice, for example), but I find that most often this rule is good.

Furthermore, I strongly recommend that you follow the arrange/act/assert pattern (AAA). You can look it up in your preferred search engine.

So, in terms of this PR, break out your new assertions into separate test cases and write them according to all of the rules we've discussed, including a contract for the test clearly specifying what it's supposed to test. Note that the original test that you altered here is not a good test, as it does not say what it's actually supposed to verify ("NameFilter is tested and works" is extremely vague).

Rohitesh-Kumar-Jain · 2021-06-07T07:25:32Z

As one point of feedback, avoid working on things in parallel that conflict with each other. If either this or #3978 is merged, there will be a merge conflict between them.

I will keep this in mind for future PRs.

Furthermore, I strongly recommend that you follow the arrange/act/assert pattern (AAA). You can look it up in your preferred search engine.

I will surely have a look at this, thanks for making me aware of the AAA pattern.

monperrus · 2021-06-07T09:16:14Z

This PR looks good in the sense that it add new assertions.

We don't need to remove assertEquals("Foo", foo.getSimpleName());, unless it is incorrect, which is not the case here.

slarse · 2021-06-07T09:48:25Z

This PR looks good in the sense that it add new assertions.

IMO this is not an inherently good thing, arbitrarily adding new assertions to an existing test is not sustainable. It then becomes unclear what that test is supposed to verify. The original test doesn't have a clear goal (the contract essentially just says "test that name filter is correct"), which doesn't make things better.

"God tests" that verify everything about some method or class are a recipe for maintainability degradation.

As for the removal of the guard assertion, I agree that it's unrelated to remove. I don't think it should have been there to begin with as it is essentially just clutter, but that's a separate issue.

monperrus · 2021-06-07T11:11:59Z

arbitrarily adding new assertions to an existing test is not sustainable

yes, it's an engineering tradeoff between adding assertions to an existing test (if the test intention is the same), and creating a new test.

slarse · 2021-06-07T11:33:30Z

arbitrarily adding new assertions to an existing test is not sustainable

yes, it's an engineering tradeoff between adding assertions to an existing test (if the test intention is the same), and creating a new test.

If the test intention is the same, I'm all for adding a "missing" assertion, in no way do I mean that there should be only one assertion per test. In the test case in this PR, I don't think the test intention is clear, and so wouldn't want to add anything to it.

monperrus · 2021-06-07T13:07:22Z

In the test case in this PR, I don't think the test intention is clear, and so wouldn't want to add anything to it.

OK for me. @Rohitesh-Kumar-Jain is NameFilter in the Descartes report?

Rohitesh-Kumar-Jain · 2021-06-08T06:25:12Z

Sorry for the late reply

OK for me. @Rohitesh-Kumar-Jain is NameFilter in the Descartes report?

yes, it is here

monperrus · 2021-06-08T07:45:09Z

OK, then let's finish this PR :) it's in our plan.

Could you document the test intention of the added assertions with a comment line // contract: before each of them? Thanks!

Rohitesh-Kumar-Jain · 2021-06-08T08:10:13Z

OK, then let's finish this PR :) it's in our plan.

Could you document the test intention of the added assertions with a comment line // contract: before each of them? Thanks!

I have added contract before each assertion : )

src/test/java/spoon/test/filters/FilterTest.java

slarse

Hi @Rohitesh-Kumar-Jain,

This is a good start. Please see my comments.

I also have some general comments.

Also note that each test should have one contract. The assertions you've added in this test each have their own contract, and so should be separate tests. Yes, there are tests in the test suite which have multiple contracts, but that's not great. One test should test one concept <==> one test should have one contract.

In other words: break out your added assertions into separate tests.

slarse · 2021-06-09T06:15:18Z

src/test/java/spoon/test/filters/FilterTest.java

 		assertEquals(1, elements.size());
+		// contract: tests NameFilter constructor for null value case


The contract should not say what the test does, but what is expected to happen. For example, this one ought to say contract: NameFilter constructor should throw IllegalArgumentException when passed null

The same problem exists with all of the contracts you have written here, they just say what the test does, while they should detail the expected outcome of some action.

Got it 👍🏼

src/test/java/spoon/test/filters/FilterTest.java

Rohitesh-Kumar-Jain · 2021-06-09T11:44:04Z

Hi @slarse, thanks for reviewing PRs : )

In other words: break out your added assertions into separate tests.

Understood 👍🏼

slarse

Just two things left to fix, final contract seems incorrect and there's a rogue parentheses on one of the @Test annotations.

slarse · 2021-06-10T06:11:51Z

src/test/java/spoon/test/filters/FilterTest.java

+
+	@Test()
+	public void testNameFilterGetType() {
+		// contract: getType method returns the object of class NameFilter


This contract is not correct, the NameFilter.getType() is not tested to return a NameFilter object.

should the contract be like this : getType method returns the type of class which is begin used ?

There's no one answer to what a contract should say, but it cannot be incorrect. What the getType method actually is supossed to return is the CtNamedElement class, which is not an object of class NameFilter, and thus the comment here is explicitly incorrect.

The contract comment should state in human-readable terms what the test is supposed to verify. One potential contract here would be: // contract: NameFilter.getType() should return the CtNamedElement class

A less specific one would be: // contract: NameFilter.getType() should return the correct class, where the test itself then specifies that correct class.

Note that we don't usually test getters directly, because they are trivial pieces of code, and so the test and its contract here necessarily become a bit contrived.

Note that we don't usually test getters directly, because they are trivial pieces of code

I have one question, so I can skip adding tests for getter like getFactory() in this class ?

I have one question, so I can skip adding tests for getter like getFactory() in this class ?

Directly testing that would be fairly far down the priority list, yes. Note however that this isn't a pure getter in the sense that it doesn't just return a static value or a field of the class, but actually calls some other method. In that sense, it's not pointless to test it, but it would for example be much more important to thoroughly test spoon.template.Substitution.getFactory (the method that's delegated to), as that method is more complex. Overall, the Substitution class looks fairly poorly tested when looking at the Coveralls report, so perhaps that'd be a good target for you!

src/test/java/spoon/test/filters/FilterTest.java

slarse · 2021-06-10T12:33:22Z

src/test/java/spoon/test/filters/FilterTest.java

@@ -111,13 +111,46 @@ public void setup() throws Exception {

 	@Test
 	public void testNameFilter() throws Exception {
-		// contract: legacy NameFilter is tested and works
+		// contract: the constructor of the class NameFilter creates and initialises an object.


This contract is also not correct, this test is not for NameFilter's constructor. The test verifies that NameFilter finds the expected amount of elements when filtering with it (it's a very weak test).

The original comment was technically correct, it was just pointless. But you don't have to change this contract when adding your other test methods, as you don't touch this test method. I'm guessing this is just left over from when you modified this method.

Either fix this comment such that it's correct, or revert to the original contract.

slarse · 2021-06-10T13:16:27Z

src/test/java/spoon/test/filters/FilterTest.java

+
+	@Test
+	public void testNameFilterGetType() {
+		// contract: getType method returns the datatype that the NameFilter class returns


I'm truly sorry for going on and on here, but this contract still doesn't make sense to me. It says that a method returns the datatype that a class returns. But a class does not return anything, and so it does not make sense.

It's very important to be able to succinctly specify what a test is supposed to verify, so we need to keep iterating on this until you've nailed it. Feel free to just comment in this comment with suggestions before committing to something.

I'm truly sorry for going on and on here

no no, it's my fault that iterations are still going on, had I raised a perfect PR, no iterations would have occurred. I will try to come with a succinct contract.

I would like to go with the one suggested by you only // contract: NameFilter.getType() should return the CtNamedElement class

slarse · 2021-06-10T13:19:59Z

src/test/java/spoon/test/filters/FilterTest.java

@@ -111,13 +111,46 @@ public void setup() throws Exception {

 	@Test
 	public void testNameFilter() throws Exception {
-		// contract: legacy NameFilter is tested and works
+		// contract: NameFilter finds the expected amount of elements when filtering


Rohitesh-Kumar-Jain · 2021-06-10T16:38:38Z

I am sorry for taking so many iterations, I am going to spend tomorrow reading lots of existing tests of Spoon so that my next PR may require lesser iterations : )

slarse

@Rohitesh-Kumar-Jain

I am sorry for taking so many iterations, I am going to spend tomorrow reading lots of existing tests of Spoon so that my next PR may require lesser iterations : )

Don't be sorry, it's perfectly normal and expected for the first contributions to a project to be take some time :)

@monperrus good for merge

monperrus · 2021-06-11T07:06:13Z

Many thanks @Rohitesh-Kumar-Jain and @slarse

add testNameFilterThrowsException

1947cfc

This comment has been minimized.

Sign in to view

Rohitesh-Kumar-Jain added 2 commits June 6, 2021 14:53

add more assertions and remove the redundant one

c68f244

remove separte test case which is now redundant

5bd6499

Rohitesh-Kumar-Jain changed the title ~~WIP: test: improve tests for NameFilter class.~~ review: test: improve tests for NameFilter class. Jun 6, 2021

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

review: test: extract NameFilterTest from FilterTest #3978

Closed

Rohitesh-Kumar-Jain changed the title ~~review: test: improve tests for NameFilter class.~~ WIP: test: improve tests for NameFilter class. Jun 7, 2021

add contracts to the assertions

3fbd091

Rohitesh-Kumar-Jain commented Jun 8, 2021

View reviewed changes

src/test/java/spoon/test/filters/FilterTest.java Show resolved Hide resolved

slarse suggested changes Jun 9, 2021

View reviewed changes

break assertions into separate tests

ae2e352

Rohitesh-Kumar-Jain changed the title ~~WIP: test: improve tests for NameFilter class.~~ review: test: improve tests for NameFilter class. Jun 9, 2021

slarse suggested changes Jun 10, 2021

View reviewed changes

improve contract and remove rouge parentheses

f2d36c3

slarse suggested changes Jun 10, 2021

View reviewed changes

change contract of the testNameFilter

422365b

slarse reviewed Jun 10, 2021

View reviewed changes

improve contract for testNameFilterGetType

e039a75

slarse approved these changes Jun 11, 2021

View reviewed changes

monperrus changed the title ~~review: test: improve tests for NameFilter class.~~ test: improve tests for NameFilter class. Jun 11, 2021

monperrus merged commit 414b6d6 into INRIA:master Jun 11, 2021

Rohitesh-Kumar-Jain deleted the improve-testnamefilter branch June 11, 2021 07:50

Rohitesh-Kumar-Jain mentioned this pull request Jun 14, 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: improve tests for NameFilter class. (INRIA#3975)

0e4b576

		assertEquals(1, elements.size());
		// contract: tests NameFilter constructor for null value case

test: improve tests for NameFilter class. #3975

test: improve tests for NameFilter class. #3975

Conversation

Rohitesh-Kumar-Jain commented Jun 4, 2021

Rohitesh-Kumar-Jain commented Jun 4, 2021

slarse commented Jun 4, 2021

This comment has been minimized.

monperrus commented Jun 4, 2021

slarse commented Jun 4, 2021

Rohitesh-Kumar-Jain commented Jun 4, 2021

slarse commented Jun 4, 2021

Rohitesh-Kumar-Jain commented Jun 4, 2021

Rohitesh-Kumar-Jain commented Jun 5, 2021

Rohitesh-Kumar-Jain commented Jun 6, 2021

slarse commented Jun 7, 2021

Rohitesh-Kumar-Jain commented Jun 7, 2021

monperrus commented Jun 7, 2021

slarse commented Jun 7, 2021

monperrus commented Jun 7, 2021

slarse commented Jun 7, 2021

monperrus commented Jun 7, 2021

Rohitesh-Kumar-Jain commented Jun 8, 2021

monperrus commented Jun 8, 2021

Rohitesh-Kumar-Jain commented Jun 8, 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

Rohitesh-Kumar-Jain commented Jun 9, 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

slarse Jun 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohitesh-Kumar-Jain Jun 10, 2021 • edited Loading

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 10, 2021

slarse left a comment

Choose a reason for hiding this comment

monperrus commented Jun 11, 2021

slarse Jun 12, 2021 •

edited

Loading

Rohitesh-Kumar-Jain Jun 10, 2021 •

edited

Loading