migrate namefiltertest to Junit 5

[Rohitesh-Kumar-Jain]

Hi team,

In this commit I have:

• migrated nameFilterTest from Junit 4 to Junit 5
• Restructured the nameFilterTest, where it should have been placed
• Added a testNameFilterThrowsException, to improve the line coverage from 56% to 66%.

Statistics of nameFilter class.

I guess in the next commit the objective could be to improve the line coverage from 66% to 100% ?

[slarse]

Hi @Rohitesh-Kumar-Jain,

This pull request does too many different things. The number one rule of pull requests is to do one thing (see the contributing guidelines. This rule sometimes gets complicated when it's hard to do interdependent things separately, but your modifications here are completely orthogonal: extraction of part of a test class into a separate test class, migration to JUnit5 of said part, and addition of a test case. Each of these can stand on their own.

The reason it's so important to keep PRs focused on one thing is that it makes it much easier to review them. A rule of thumb: if you can't summarize all of the changes of a PR with the title, then it's not focused.

Also, I don't see any edits to the original test class from which you extracted the test case. Perhaps you forgot this?

Could you please do the following?

1. Restructure this PR into three separate ones; each one doing one thing
   • I would advise migrating the entire FilterTest.java class to JUnit5 before doing anything else, if you're set on extracting nameFilterTest into a separate file.
2. If the PR is related to an existing issue, reference that issue with #<issue number>

As a side note, don't take this as a scolding or anything of the sort, I'm just trying to help you to write more concise PRs :)

[Rohitesh-Kumar-Jain]

Hi @slarse,

A rule of thumb: if you can't summarize all of the changes of a PR with the title, then it's not focused.

Thanks for sharing this rule.

Also, I don't see any edits to the original test class from which you extracted the test case. Perhaps you forgot this?

Yes, I should have removed the NameFilterTest from filterTest.

I would advise migrating the entire FilterTest.java class to JUnit5 before doing anything else,

Yes, I feel that I should first migrate some test classes to Junit5 before doing anything else, but I feel that it might be better If I start with a smaller test file, and after that migrate filterTest to Junit5.

if you're set on extracting nameFilterTest into a separate file.

I feel that nameFilterTest isn't at the correct place currently, it should be in spoon/legacy instead of being in spoon/test/filters/testclasses
But I feel this is against the current structure of the tests. There is even an outdated discussion about this #2137 and a recent discussion #3677, but these discussion lack a solid conclusion in the end.

If the PR is related to an existing issue, reference that issue with #

I will keep this in mind.

As a side note, don't take this as a scolding or anything of the sort, I'm just trying to help you to write more concise PRs :)

Not at all, that's the whole idea of students contributing to real world projects, so that they can learn the best practises, I am glad that I am getting detailed review on PR.

I have a question, to which branch I should be pushing my commits?

[Rohitesh-Kumar-Jain]

I will close this PR after 1 day (following the 1 day rule for closing PRs), and will raise an atomic PR (will migrate only one testclass to Junit5 in it).

[slarse]

but I feel that it might be better If I start with a smaller test file, and after that migrate filterTest to Junit5.

It might, but I don't think you'll have any trouble migrating FilterTest. It's a big class but it does very little fancy stuff. I just tried migrating with IntelliJ IDEA, and it could do it automatically, with only 17 insertions and 13 deletions. Just swapping the imports fixes most of the problems.

I feel that nameFilterTest isn't at the correct place currently, it should be in spoon/legacy instead of being in spoon/test/filters/testclasses
But I feel this is against the current structure of the tests. There is even an outdated discussion about this #2137 and a recent discussion #3677, but these discussion lack a solid conclusion in the end.

You are right that the test is not strictly in the correct place, and I'm not opposed to the idea of moving it. The referenced issues are however mostly about test resources, more so than test classes (although that is part of it).

Edit: Actually, #3677 is about 50:50 about the messy test organization and test resources! So we should reach a consensus there before we start moving tests.

Not at all, that's the whole idea of students contributing to real world projects, so that they can learn the best practises, I am glad that I am getting detailed review on PR.

Good! I just wanted to mention this to set the tone. You'll probably get more than a few reviews from me, and they can seem harsh if you're not mentally prepared.

I will close this PR after 1 day (following the 1 day rule for closing PRs)

The 1 day rule is for integrators: They must wait at least 1 day before merging a new PR to allow other integrators the time to also have a look. Contributors can close their PRs at any time they wish.

[Rohitesh-Kumar-Jain]

I just tried migrating with IntelliJ IDEA, and it could do it automatically,

I will also try to do it automatically thanks for letting me know this.

So we should reach a consensus there before we start moving tests.

Yes, we should quickly reach a consensus, the further we delay, the more work will be left behind.

You'll probably get more than a few reviews from me, and they can seem harsh if you're not mentally prepared.

I believe to learn something, and to change old habits one needs to step out of his comfort zone, no worries :P

[Rohitesh-Kumar-Jain]

Hi @slarse, @monperrus & @nharrand

Thanks for helping me merge my last PR, as #3946 is merged now, I was wondering what will be the best thing to do after that.

As the structure of the tests & tests resource is yet to be finalised, so I believe I should wait before extracting the nameFilterTest into a separate file.

Should I next try to improve the line coverage from 55% to 100% ? But as the test Strength and mutation coverage of nameFilterTest is already 100%, should we prioritize other classes or it will be worth improving the line coverage?

or should I migrate other tests to Junit5 (this time more complex tests involving more manual changes)?

[slarse]

Hi @Rohitesh-Kumar-Jain,

I'm not familiar enough with GSOC to give any advice on this in that respect, so I'll defer that question to @monperrus and @nharrand.

If you're just looking to do some "practice contributions" (which seems like a good way to get started regardless of the particular goals of your GSOC project), I'd say you can do pretty much anything. The important thing would be the process of performing PRs, which requires some amount of practice.

As you are to improve the test suite, I would personally very much appreciate if you convert the test classes you intend to work on to JUnit5 before you start improving them, as the more JUnit4 tests we add, the harder migration will be. So perhaps just migrate a few test classes to begin with?

[Rohitesh-Kumar-Jain]

Hi @slarse,

Thanks for your suggestions.

As you are to improve the test suite, I would personally very much appreciate if you convert the test classes you intend to work on to JUnit5 before you start improving them, as the more JUnit4 tests we add

I haven't yet decided on which classes I will be working on.

So perhaps just migrate a few test classes, to begin with?

This seems a good start to me : )

I have one question, do I need to push one PR at a time, and wait for it to get merged to raise another? or I can raise more than one PR at a time from different branches?

[slarse]

I have one question, do I need to push one PR at a time, and wait for it to get merged to raise another? or I can raise more than one PR at a time from different branches?

You can have several PRs open at any one time (one of the benefits of opening PRs from separate branches). Just try to avoid opening PRs that conflict with each other, that is to say, edit the same parts of the same files. There's no risk of that when migrating test classes from JUnit4 to JUnit5, though, as you just operate on one file at a time.