Do not remove the just-added annotation from the list of annotations.

[lahodaj]

Here:
#8142 (comment)
was a note that TestMethodController.setTestMethods must first be called with an empty list, and only then with the real values. Looking at the code, the issue is that first the new annotations are added, and then the old ones are removed. And when one annotation replaces another on the same line, the removal will remove registration of the newly-added annotation in the annotationLines/TestMethodAnnotation.DOCUMENT_ANNOTATION_LINES_KEY map.

This was not obvious for Java, as Java support runs several providers, and calls setTestMethods after running each provider (passing cumulative results into the method). As a consequence, in many cases, the setTestMethods was first called with an empty list, effectively replicating the PHP workaround.

The proposal here is to:

• first remove and then add annotations, which should avoid the above problem
• for Java, collect the test methods, and call setTestMethods only once
• remove the workaround from PHP.

[tmysik]

Thank you for the PHP fix.

CC @junichi11 @troizet

[junichi11]

I fixed ComputeTestMethodAnnotations against the delivery branch: #8196
So, can't we include this PR in NB25?

[troizet]

I quickly tested it. Unfortunately, the problem is not completely solved. When renaming a method, the annotations are updated, but not when moving the method to another line.

Also, while I was playing around with renaming a method and moving it to another line, I ran into a bug where when a method is renamed, the action still shows the old method name.

[tmysik]

@lahodaj please, see the bug above (see the screenshot). thank you

[junichi11]

Example to reproduce it with PHP:

1. Download composer.phar
   • latest-stable: https://getcomposer.org/download/latest-stable/composer.phar)
   • manual: https://getcomposer.org/download/
2. Set PHP interpreter (Options > PHP > General)

3. Set the composer.phar (Options > PHP > Frameworks & Tools > Composer)

4. Create a sample project

   • File > New Project > Samples > PHP > Calculator - PHPUnit Sample Application

5. Right-click the php project > Composer > Install (dev)

6. Open Test Files: src/CalculatorTest.php

[ebarboni]

this PR should be resynchronized after #8205 is in on master as #8196 will conflict.

[junichi11]

I've verified it is fixed:

Thank you!

[troizet]

It works. Thank you very much!

[mbien]

regarding #8201 (comment)

just to be sure: is NB26 correct or is this meant for NB25/delivery?

If the target is changed, this would need to be rebased too, since the branch is post spec version bump to avoid it creating a huge merge commit. + tests need to run again and according to our rules it would need one new approval.

Its all doable - but someone would have to decide what to do ;)

[lahodaj]

I apologize for belated comment, I planned to do this sooner. If the original patch would have worked, I would have been tempted to push it to NB25. But the current patch could probably use some baking time, so I'd prefer NB 26. Unless PHP needs the fix, but I suspect it is OK with the current workaround.

My understanding is that #8196 is not yet in master - I'll wait for that, rebase, and if all goes well, I'll merge this PR to master.

[lahodaj]

And thanks for the testing!