review: test: add test for removeAnnotation method

[Rohitesh-Kumar-Jain]

#1818

Hi, I have added a new test for the method removeAnnotation :

These tests are intended to kill this mutation:

Link to #3967

[slarse]

Hi @Rohitesh-Kumar-Jain,

Sorry about the slow response these days. I'm prioritizing your PRs as I know you're doing the GSOC thing, but I'm just not around that much during vacation times.

Anyway, the test here looks pretty good, but there are a couple of fundamental mistakes/miscomprehensions.

First of all, we don't typically test implementations, but rather test the public interfaces (e.g. CtElement). Second, you're not actually explicitly testing CtElementImpl in this test, you're testing the CtClass interface. How that interface's removeAnnotation method is implement is an unspecified implementation detail that could change at any time, making the test's specification non-robust.

To fix this, I would either a) place this test inside CtClassTest or b) explicitly instantiate a CtElementImpl and test that. I would favor a) as we don't typically test specific implementation details, but the choice is up to you.

[Rohitesh-Kumar-Jain]

Sorry about the slow response these days. I'm prioritizing your PRs as I know you're doing the GSOC thing, but I'm just not around that much during vacation times.

It's perfectly fine, do enjoy vacations 🎉, I am too going to grandma's house just after this ends

Thanks for prioritizing PRs : )

First of all, we don't typically test implementations, but rather test the public interfaces

I will keep this in mind for future

How that interface's removeAnnotation method is implement is an unspecified implementation detail that could change at any time, making the test's specification non-robust.

That didn't came to my mind, thanks for letting me know

To fix this, I would either a) place this test inside CtClassTest or b) explicitly instantiate a CtElementImpl and test that.

a) seems fine to me as well

[Rohitesh-Kumar-Jain]

a) seems fine to me as well

For a) realized I first need to migrate CtClassTest it to Junit5

[slarse]

For a) realized I first need to migrate CtClassTest it to Junit5

This PR is already started, so I'm perfectly fine with you skipping that in this case.

[slarse]

Very good @Rohitesh-Kumar-Jain!