review: test: extract NameFilterTest from FilterTest

[Rohitesh-Kumar-Jain]

This PR aims to extract the NameFilterTest from FilterTest, and placing it in its correct location.

This PR involves pure refactoring, no new test added or removed.

Extraction has been done safely as the code coverage for NameFilter remains the same before and after extracting the test.

Link to #3967

(I thought it will be nicer to first extract the NameFilterTest before adding new tests).

[slarse]

Hi @Rohitesh-Kumar-Jain,

There's nothing wrong with the PR, but if we do this for one of the filter classes it should reasonably be done for all of them. Do you intend to give each filter class the same treatment?

[Rohitesh-Kumar-Jain]

but if we do this for one of the filter classes it should reasonably be done for all of them. Do you intend to give each filter class the same treatment?

Yes, this makes sense to me, I am willing to give each filter class the same treatment.

If the PR is okay pls merge this one, I will close #3975, and raise a new PR after this PR gets merged.

[slarse]

@monperrus I'll leave it to you to decide if this is desirable. Personally I feel like each filter should have its own test class (the FilterTest class is 1000+ lines, which is too much). But it will lead to some amount of duplication in setup code and the like.

[monperrus]

Hi @Rohitesh-Kumar-Jain, @slarse

We all agree that organized tests are important. Yet, this is less important than the bug revealing power of tests, which comes from the assertions.

Our goal in GSOC is to improve the quality of the oracles, by adding new assertions. It is not to move tests.

So in order to stay focused, we may not split FilterTest in many small tests. We would rather look for filter methods in the Descartes report in order to add new assertions.

WDYT?

[slarse]

Oh, right, I clean forgot about the GSOC goals. This is pretty unrelated to that, so I would second focusing on improving the tests rather than reorganizing them (which is a project in and of itself).

[Rohitesh-Kumar-Jain]

Our goal in GSOC is to improve the quality of the oracles, by adding new assertions. It is not to move tests.

Yeah, I fully agree with this, but I am not planning to first move all the tests, and then add assertions.

My plan is :

1. first convert the file to Junit5.
2. Extract a particular test class.
3. add more assertions to that particular test class.
4. move to the next iteration of the loop.

Extracting a class, doesn't take much time, and writing assertions in a cleaner environment seems much easier, also placing the test in its correct location enables more IDE features like testing with code coverage, which takes only 10 seconds to run in contrast to Jacoco which takes 12 mins to generate the full report.

But if we aren't extracting the code, should assertions remain in a single method like this, or should I make separate methods for the testing different methods of the same class like this. (If we want to follow the first I believe it's better to extract the test, test extraction is fairly simple, and I won't extract another test without improving assertions of the test that I extracted before).

[Rohitesh-Kumar-Jain]

My plan is :

first, convert the file to Junit5.
Extract a particular test class.
add more assertions to that particular test class.
move to the next iteration of the loop.

For example, I first converted the FilterTest to Junit5, then I will extract NameFilterTest for the NameFilter class, and after that will add assertions for the NameFilter class, One iteration completes, after this, I will extract RegexFilterTest for the RegexFilter, and will add assertions for the RegexFilterTest, the second iteration completes

[slarse]

Extracting a class, doesn't take much time, and writing assertions in a cleaner environment seems much easier

I completely agree with this, but the problem I see is if this gets done half-way. That is to say, you extract half of the filter tests into separate classes and improve those, and leave the other half in FilterTest. Looking at the report from Descartes, the spoon.reflect.visitor.filter package (which FilterTest primarily targets) has comparatively high line coverage and almost perfect mutation coverage and test strength. Targeting FilterTest for major refactoring and strengthening therefore doesn't seem like it aligns with your GSOC project's goals.

In addition, FilterTest is a rather hard test class because it contains a lot of testing anti-patterns (god tests, tests a whole bunch of different classes, unclear test goals overall).

Given all of that, completing the refactoring of FilterTest doesn't really seem to be in your best interests, and thus it seems likely to me that it won't get completed.

also placing the test in its correct location enables more IDE features like testing with code coverage

Code coverage is not affected by where the test class is located. Perhaps there's some "auto-run" feature that doesn't work if the test class is in an (according to the IDE) unexpected location, but you can still run with coverage and get the coverage data.

But if we aren't extracting the code, should assertions remain in a single method like this, or should I make separate methods for the testing different methods of the same class like this

One test should test one concept. So, the latter option.

@Rohitesh-Kumar-Jain Perhaps you should take a look at some other package where you can make a larger impact?

[Rohitesh-Kumar-Jain]

In addition, FilterTest is a rather hard test class because it contains a lot of testing anti-patterns (god tests, tests a whole bunch of different classes, unclear test goals overall).
Hi @slarse,

I would be glad if the org may select a package for me to work upon, the reason I selected NameFilter class, because its code was easy to understand 😅 and hence easier to write the tests for getting a start.

But I believe it will be better if someone from the org decides on which package to work upon.

One test should test one concept.

Understood 👍🏼

So I am closing this PR, and now will work upon #3975, finish that and after that will work upon the package decided by the org.

[slarse]

the reason I selected NameFilter class, because its code was easy to understand

Totally understand that, and so it's fitting that you create a couple of test cases there just to get started with what a good test looks like (I'll guide you in #3975).

package decided by the org

Looking at the report you generated for Spoon, perhaps spoon.processing or spoon.refactoring? They both look pretty poorly tested overall. spoon.template and spoon.support.template also looks pretty poorly tested.

Oh, and as for not getting coverage in IntelliJ IDEA, I just tried running with coverage. By default, it will only include coverage for the same package as the test is located in. You can manually add other packages to the coverage scope (there's a button that pops up where coverage should be, just click "Edit").

[Rohitesh-Kumar-Jain]

Looking at the report you generated for Spoon, perhaps spoon.processing or spoon.refactoring? They both look pretty poorly tested overall. spoon.template and spoon.support.template also looks pretty poorly tested.

Thanks for selecting the packages : )

By default, it will only include coverage for the same package as the test is located in. You can manually add other packages to the coverage scope (there's a button that pops up where coverage should be, just click "Edit").

Thanks for letting me know this