[NETBEANS-3799] Improve extracting types from PHP annotations

[czukowski]

See Jira for details: https://issues.apache.org/jira/browse/NETBEANS-3799

There were some unused test methods for the class I've modified (and failing if renamed to be included when running tests), according to git history, they were just added and never actually tested:

https://github.com/emilianbold/netbeans-releases/blame/d5e07b7d6c95bf5342eb8bc351e2d09b88d5e0ad/php.api.annotation/test/unit/src/org/netbeans/modules/php/api/annotation/util/AnnotationUtilsTest.java

I've included these test cases to run and modified the expected values so that they don't fail, specifically not to expect ManyToOne type.

[junichi11]

@czukowski Thank you for your contribution. We can't merge this soon because currently, the merge window is closing. (We can merge this after NB11.3 is released)

@tmysik Should increase the spec version?
It seems that this is used in php.doctrine2 and php.symfony2.

Maybe, should also add unit tests to php.doctrine2?

[czukowski]

@junichi11 yes, I'm aware of that, it can wait.

[tmysik]

@junichi11 @czukowski

Should increase the spec version?
It seems that this is used in php.doctrine2 and php.symfony2.

It depends. Without increasing the spec version, Auto Update will not notice and only new NB release will contain this change - is this what we want? If not, we need to increase the spec version of:

• php.api.annotation module; but also
• php.doctrine2 and php.symfony2 (so Auto Update can offer their update) together with their dependency update on php.api.annotation, of course

Maybe, should also add unit tests to php.doctrine2?

That would be nice.

Thanks!

[tmysik]

Please, see my comment in this PR about spec version increase and feel free to comment.

The change itself seems fine to me (personally I do not like the change of the test, it is not a typical "NetBeans way" but I am definitely not against it - if it passes :)

Thank you.

[czukowski]

@tmysik I've had my doubts about changing the tests, but it wasn't easy to tell what were the individual tests actually doing (are they testing different things or same thing with different test cases?), so I've found out about the Enclosed and Parametrized things, but it just didn't work if the main test class extended NbTestCase. Is there a way to make both "NetBeans way" and keeping the testing logic and test data separated? I'll be happy to make the changes.

[tmysik]

@czukowski I don't think it is (easily) doable. As I wrote, it is "just unusual" but no problem. AFAIK, NbTestCase is based on JUnit 3 (?) that is pretty old now so please, keep it as it is.

Thank you for your work, once the dependencies are "solved" I am ready to approve this PR.

[neilcsmith-net]

Without increasing the spec version, Auto Update will not notice and only new NB release will contain this change - is this what we want?

@tmysik this is not really correct! This should only be required if the PR is against a release branch as a pushed update fix, and we haven't done that much. For master, the release manager increments every spec version before the merge window reopens. If you put a spec version change in a PR it probably won't merge cleanly.

cc/ @ebarboni

[tmysik]

@neilcsmith-net
You can be right, that is why I wrote: "is this what we want?" :)

Let me explain - in NetBeans, we used to have daily builds that were used by some of our users (in fact, quite many of them). For these users, no "Update available" would appear without increasing (all) the spec versions of the given modules. These users would need to go to our download page and download the whole, new/fresh NB build again to have this change in their NetBeans.

@czukowski
I don't know whether this is possible now too so sorry for the confusion. I believe @neilcsmith-net will be a much better person to advise here :)

Thanks!

CC @junichi11

[tmysik]

Based on the comments in this PR, I guess I can approve this change as it is.

[junichi11]

@tmysik Thank you for your answer, Tomas!

@czukowski
I've investigated where @RunWith is used.

$ find . -name '*.java' | xargs grep "@RunWith"

Results

./webcommon/javascript2.editor/test/unit/src/org/netbeans/modules/javascript2/editor/parser/Ecma6TestSuite.java:@RunWith(Suite.class)
grep: ./java/spellchecker.bindings.java: ディレクトリです
grep: ./java/performance.java: ディレクトリです
grep: ./java/xml.tools.java: ディレクトリです
./java/junit.ant.ui/src/org/netbeans/modules/junit/ant/ui/AntJUnitTestMethodNode.java:                    if ((text.contains("@RunWith") || text.contains("@org.junit.runner.RunWith")) //NOI18N
grep: ./java/refactoring.java: ディレクトリです
./java/testng/src/org/netbeans/modules/testng/TestGenerator.java://                     * put the @RunWith(...) and @Suite.SuiteClasses(...)
./java/testng/src/org/netbeans/modules/testng/TestGenerator.java://                         * put the @RunWith(...) annotation
./java/testng/src/org/netbeans/modules/testng/TestGenerator.java://                         * just after the @RunWith(...) annotation
./java/testng/src/org/netbeans/modules/testng/TestGenerator.java://        /* @RunWith(Suite.class) */
grep: ./java/jellytools.java: ディレクトリです
./java/junit/test/qa-functional/src/org/netbeans/test/junit/junit4/CreateProjectTest.java:        lines.add("@RunWith(Suite.class)");
./java/junit/test/qa-functional/src/org/netbeans/test/junit/junit4/CreateProjectTest.java:        lines.add("@RunWith(Suite.class)");
./java/junit/src/org/netbeans/modules/junit/JUnit4TestGenerator.java:                     * put the @RunWith(...) and @Suite.SuiteClasses(...)
./java/junit/src/org/netbeans/modules/junit/JUnit4TestGenerator.java:                         * put the @RunWith(...) annotation
./java/junit/src/org/netbeans/modules/junit/JUnit4TestGenerator.java:                         * just after the @RunWith(...) annotation
./java/junit/src/org/netbeans/modules/junit/JUnit4TestGenerator.java:        /* @RunWith(Suite.class) */
grep: ./java/api.java: ディレクトリです
grep: ./java/selenium2.java: ディレクトリです
./java/form/test/unit/src/org/netbeans/modules/form/layoutdesign/LayoutTestSuite.java:@RunWith(TestsInPkgSuite.class)
grep: ./java/websvc.saas.codegen.java: ディレクトリです
./java/junit.ui/src/org/netbeans/modules/junit/ui/actions/TestSingleMethodSupport.java:			if ((text.contains("@RunWith") || text.contains("@org.junit.runner.RunWith")) //NOI18N
./platform/openide.util.lookup/test/unit/src/org/openide/util/lookup/ProxyLookupResultListenerTest.java:@RunWith(Parameterized.class)
./platform/api.scripting/test/unit/src/org/netbeans/api/scripting/JavaScriptEnginesTest.java:@RunWith(Parameterized.class)
grep: ./extide/options.java: ディレクトリです
./ide/libs.graalsdk/test/unit/src/org/netbeans/libs/graalsdk/JavaScriptEnginesTest.java:@RunWith(Parameterized.class)

Currently, It seems that it is used only in several files.
IMHO, maybe, you should add unit tests without changing the existing test code (or like existing code) from the next time if there is no reason we have to change it.
Then, could you please add unit tests to php.doctrine2 if possible?

[neilcsmith-net]

@tmysik that's an interesting point that maybe can be discussed on dev@ AFAIK we haven't had a valid update centre for any daily build since moving to Apache. And ideally the daily build would be on the same task as @ebarboni redone Jenkins pipeline, and potentially could have UC there.

This process hasn't changed much as far as I know, except ant increment-spec-versions is now done after release rather than at release branching stage - just means pending PRs with spec version changes can have problems.

Enough chatter here on this topic, and apologies for the noise 😄 Can follow up on dev@ if need be.

[czukowski]

@junichi11 well, if the code is not very well readable, it kind of calls for action to do something about it, at least in my opinion, so I wouldn't say there was absolutely no reason to touch the tests 😀

Also this is the first time ever I've done anything in Java, while it didn't seem hard (perhaps given the size of the actual change), I was trying to be extra cautious not to break anything and really understand what the code in question does, and I've found myself having to re-examine each test method in order to see whether it does exactly the same thing as the next one, just with another set of inputs or maybe is there a subtle difference in the test logic itself, which I couldn't see at the first glance, and this reading and re-reading the test methods was quite time consuming. I would like to try and make some more contributions, but I would feel uncomfortable writing tests that I myself would have trouble reading later on.

[junichi11]

@czukowski Writing readable code is good. But I am worried that there might be problems at the moment(like NbTestCase)...
So, maybe, if you do the same thing at least for existing test codes, I would point out it. (There would be no problem if you were an expert of NetBeans and were familiar with NetBeans and you could fix problems when something happens.) I just would like to avoid breaking the tests...

[czukowski]

@junichi11 maybe we should discuss and find a common ground on how the tests should be written in the most optimal way possible, both avoiding the code duplication and ensuring they stay in line and will remain compatible as NetBeans is moving forward. As @tmysik has pointed out, the current test suite implementation is based on an older JUint version, and I've noticed the compiler complaining about some deprecated usage too, so I think the time to deal with it is slowly approaching anyway.

[tmysik]

@czukowski @junichi11

My opinion is - (always) make the patch simple and readable so it is easy to review (rollback, fix, etc.). This is maybe not the case for the test change in this PR but maybe (a) it is not easily doable and/or (b) it is not a bad thing since the author knows exactly what he is doing :) I don't use JUnit so I am not able to judge (a) and of course I hope for (b) :)

[junichi11]

@czukowski @tmysik
OK, my idea: It would be easy to review if changing the test code is separated from the same PR. (Add tests like existing code once, then change it, create a PR as new one.) Plus if there is a problem, we should be able to revert it easily :)
I'm not an expert of JUnit. So, if you find a problem, please ask about that via the mailing list :) I hope someone helps you.
Thanks.

[czukowski]

Ok, so essentially I did it the other way around. But it is in a separate commit, so it should still be easy to review it, right? As for breaking anything, if my change breaks any tests, I assume it is my responsibility to fix it even before asking anybody to review (and if I miss it and ask for review, then I trust you to return this to me to fix, before you actually start reviewing the code).

By the way, I can see that Travis is running quit a lot of jobs, I'm guessing I need to watch 'Test php modules' if I'm editing any of those, do you know of any other Travis jobs to keep an eye on?

[junichi11]

@czukowski Separating commit is good, of course. In this case, reviewers have to review at least 2 things at the same time(in the same PR). Source code changes and test code changes. The main changes are not changing test codes but fixing/improving the issue, I suppose. I don't think that we have to change the test codes absolutely if they work fine.
So, although I wrote above, I think that changing test codes should be separated from the same PR for simple :) We should change the test codes at last(after the issue is fixed) because there should be no problem if we add tests like existing codes. When we find a problem, we can also revert it(only test code changes) easily if there are no new changes.

do you know of any other Travis jobs to keep an eye on?

I'm not sure what this means, sorry. If you have a question about the Travis CI, please try asking on the mailing list.

I'll approve this PR after you add unit tests to php.doctrine2 :)
If that is not doable, please let me know. I'll just approve this because I've verified this works fine with my dev build.
Thank you for your work!

[czukowski]

@junichi11 I think I could work with separating parts to different PRs in principle, but there is a couple of things bothering me:

• If I work on a class and in the process I update its tests along with the existing tests (if this becomes necessary or somehow results in an improvement, otherwise there's no point in touching them), then, as you say, these are two things, that maybe should be done separately. But if I update tests of just a single class separately from the other work, there's not much worth in this kind of refactoring, it's not immediately clear what is the purpose of the change and what good does it bring. It should probably cover at least the whole module tests, but that could be be a lot of work, including for the reviewers.
• Wouldn't splitting PRs mean that the second one will have to wait until the first one is merged, and due to merge window timings, it could mean a long time between the two PRs that are otherwise closely related?

Also, having to write tests only to rewrite them later seems like unnecessary work to me :)

I'll approve this PR after you add unit tests to php.doctrine2

Do you mean add tests for methods wherever AnnotationUtils.extractTypesFromParameters is used? Sure, I can at least try, maybe I'll learn something new.

[junichi11]

@czukowski Please don't misunderstand. I don't say "don't change tests". (although I prefer the "usual" way at the moment because the "unusual" way is not used in other modules yet (except for several files).)

Yes, we can say that for all PRs. (I also thought the same thing before.) Unfortunately, in the current way(PRs are not merged into the master branch until the next release), we just have to wait. So, smaller changes are better to avoid conflicting other changes. I don't think that we need to hurry to change tests.

Anyway, let's separate it once if you would like to change it.

Do you mean add tests for methods wherever AnnotationUtils.extractTypesFromParameters is used?

Yes, there is the cause in php.api.annotation but your problem is for php.doctrine2, right? maybe annotation parsers? (If there is no existing tests, please add them your way.)

[junichi11]

Looks good. Thank you.

[junichi11]

Apache NetBeans 12.0 merge windows OPEN : https://lists.apache.org/thread.html/r931f122d719f407fbc188a996ed45373f770ee618b80d0531c285210%40%3Cdev.netbeans.apache.org%3E

Merging as I assume that you follow the ASL v2. I suggest that you submit icla if possible: https://www.apache.org/licenses/contributor-agreements.html

Thanks.