Community
Participate
Working Groups
ASTRewrite Part of Bug 392132
*** Bug 395657 has been marked as a duplicate of this bug. ***
Created attachment 224352 [details] Proposed Patch Note: Patch of Bug 392132 has to be applied before this patch can be applied since the declarations of THROWN_EXCEPTION_TYPES_PROPERTY, and associated functions are defined in MethodDeclaration in that patch.
Created attachment 225815 [details] Proposed Patch A correction in a test case and date changes done on the previous patch.
Created attachment 226605 [details] Proposed Patched - Updated to the latest code base Markus: Need to check on: bugzilla link added at every addition - can remove if unnecessary.
(In reply to comment #5) > Created attachment 226605 [details] [diff] > Proposed Patched - Updated to the latest code base > Need to check on: bugzilla link added at every addition - can remove if > unnecessary. (1) Yes, please remove these. They only clutter the source and add no information. When we commit a change, we always add the bug number and title to the commit message. "Team > Show Annotations" makes this information readily available. In general, comments in the code should only be used to explain things that are not obvious (and that cannot be clarified by a better choice of names, or via Extract Method, etc.). (2) Try to avoid duplicating code in an if-else statement when an easy alternative is available. E.g. instead of expanding pos= rewriteNodeList(node, MethodDeclaration.THROWN_EXCEPTIONS_PROPERTY, pos, " throws ", ", "); //$NON-NLS-1$ //$NON-NLS-2$ into if (node.getAST().apiLevel() < AST.JLS8) { pos= rewriteNodeList(node, MethodDeclaration.THROWN_EXCEPTIONS_PROPERTY, pos, " throws ", ", "); //$NON-NLS-1$ //$NON-NLS-2$ } else { pos= rewriteNodeList(node, MethodDeclaration.THROWN_EXCEPTION_TYPES_PROPERTY, pos, " throws ", ", "); //$NON-NLS-1$ //$NON-NLS-2$ } , rather extract the property descriptor into a variable like this: ChildListPropertyDescriptor exceptionsProperty = node.getAST().apiLevel() < AST.JLS8 ? MethodDeclaration.THROWN_EXCEPTIONS_PROPERTY : MethodDeclaration.THROWN_EXCEPTION_TYPES_PROPERTY; pos= rewriteNodeList(node, exceptionsProperty, pos, " throws ", ", "); //$NON-NLS-1$ //$NON-NLS-2$ In some cases, you can even reuse that variable. In the tests, you can't always extract a property descriptor, but you can use code like this: List thrownExceptions= ast.apiLevel() < AST.JLS8 ? methodDecl.thrownExceptions() : methodDecl.thrownExceptionTypes(); In tests where you have to create a new exception type/name node, use a helper method that creates the right node type depending on the AST level: ASTNode createNewExceptionType(AST ast, String name) {..}
(In reply to comment #6) > (1) Yes, please remove these. They only clutter the source and add no > information. When we commit a change, we always add the bug number and title > to the commit message. "Team > Show Annotations" makes this information > readily available. +1 for now having them at all. This is especially true since with Git the history is already available locally.
(In reply to comment #7) > +1 for now having them at all. This is especially true since with Git the > history is already available locally. Of course, I meant 'not having' and not 'now having' :)
Created attachment 226743 [details] Proposed Patch Markus: Thanks for the review comments. Have incorporated them in this patch. Jay: If there are no further comments from Markus could you please release the patch?
(In reply to comment #7) > (In reply to comment #6) > > (1) Yes, please remove these. They only clutter the source and add no > > information. When we commit a change, we always add the bug number and title > > to the commit message. "Team > Show Annotations" makes this information > > readily available. > > +1 for now having them at all. This is especially true since with Git the > history is already available locally. -1. This is a red herring. (1) Many times I run into situations where the "blame" annotations say "Merge from beta java 7 branch". Yes, the information can still be dug up, but I shouldn't have to go on a wild goose chase to locate it. (2) In eclipse, you can click on the bug link and have a browser session opened locally for you. No copy/paste, no menus, dialogs, view etc. Here are the guidelines I recommend: (1) Do not mindlessly litter the code with bugzilla links. It is way more useful to annotate crucial pieces of data structures that got introduced than to annotate code. (2) Links should be provided in code only when the algorithm is tricky and was implemented the way it was implemented after a lot of discussion. (3) Nothing obvious should have link. Nor a comment.
(In reply to comment #10) > (In reply to comment #7) > > (In reply to comment #6) > > > (1) Yes, please remove these. They only clutter the source and add no > > > information. When we commit a change, we always add the bug number and title > > > to the commit message. "Team > Show Annotations" makes this information > > > readily available. > > > > +1 for now having them at all. This is especially true since with Git the > > history is already available locally. > > -1. > > This is a red herring. > > (1) Many times I run into situations where the "blame" > annotations say "Merge from beta java 7 branch". Yes, the information can > still be dug up, but I shouldn't have to go on a wild goose chase to > locate it. That might have happened in the CVS days, but not any more. You will always have the commit notes. At the risk of stating something that is off-topic, with the blame annotations, without leaving eclipse, you get to see all the information including the delta (patch, files changed etc.) that went in. > Here are the guidelines I recommend: Agree about the commenting guidelines, although I would personally like to limit the comment to some useful information about the code change.
Created attachment 226796 [details] ASTRewritingMethodDeclTest v0.1 You forgot the first case in ASTRewriteAnalyzer. Fixed that and released the implementation (without tests) as http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=d22cc5d84178d171847e96f4fb5b97fb98c95448 I've added http://wiki.eclipse.org/JDT_Core_Committer_FAQ#Comments_in_code . Feel free to update that entry. Re tests: - Note that we usually don't use the "this.method()" notation. "this.*" can be argued to make sense for field accesses to avoid confusion with local variables. But for methods, it's really unnecessary. - A lot more code can be avoided in the ASTRewritingMethodDeclTest by extracting a few helper methods, see attachment. I didn't release the ASTRewritingMethodDeclTest, because the JLS8 AST is actually never tested. This is a major design flaw in the whole org.eclipse.jdt.core.tests.rewrite.describing package. Other tests like ASTVisitorTest at least have the infrastructure to execute all tests for all AST versions (I've just updated ASTVisitorTest to actually run for JLS4 and 8, and I fixed the failing JLS8 test).
(In reply to comment #12) > I didn't release the ASTRewritingMethodDeclTest, because the JLS8 AST is > actually never tested. This is a major design flaw in the whole > org.eclipse.jdt.core.tests.rewrite.describing package. Other tests like > ASTVisitorTest at least have the infrastructure to execute all tests for all > AST versions (I've just updated ASTVisitorTest to actually run for JLS4 and > 8, and I fixed the failing JLS8 test). Thanks for Markus. I will work on making the existing tests run on multiple AST levels and add support for future rewrite tests too. Perhaps we can make the converter tests smarter too as we seem to be simply duplicating the tests for each level. But perhaps not now.
Created attachment 226981 [details] Make tests run on all AST levels Here is the patch to make the ASTRewritingMethodDeclTest and ASTRewritingTypeDeclTest run on all AST levels. Of course, we need to to update the other ones too. But these two are the ones that have been added or going to be added with new tests as part JSR 308 work. There are no failures in the two suites that have been enabled for multiple levels. Manoj, applying this patch and adding your new test to one of those suites will automatically enabled your tests run for all levels of AST8. If your test fails for any level, you need to adjust the test to either skip some levels or fix it. Markus, can you quickly glance this patch and see if this is what you had mind? Thanks!
(In reply to comment #14) > Created attachment 226981 [details] > Markus, can you quickly glance this patch and see if this is what you had > mind? Thanks! Sorry, not yet. The tests are hard-coding the AST levels and no wonder all tests are passing. Manoj, you will have to adjust the current tests also to use the apiLevel instead of hard-coding.
Created attachment 226985 [details] update to the run on all AST level patch This is the right patch and as expected there are failures. Most of them can fixed by simply skipping a particular level. From what I saw several of them were for the deprecated extra dimensions.
Created attachment 227051 [details] Proposed Patch This Patch has to be applied after https://bugs.eclipse.org/bugs/attachment.cgi?id=226985 has been applied (exclude ASTRewritingMethodDecl from that patch). This patch has merged changes from that patch as well as the suggested attachment of https://bugs.eclipse.org/bugs/attachment.cgi?id=226796. Jay: With this, the failure list reduces to 20 odd for the ASTRMD and all of them falls in the exceptionInfo bucket. Once that issue is fixed, this patch can be checked.
The approaches in comment 16 and comment 17 look good, but: (1) The reusable infrastructure (suite(), getName(), apiLevel field, deprecated references to AST.JLS[2,3,4] constants, createAST*() methods) should be moved to ASTRewritingTest. Please also add and use a constant for AST.JLS4, since that constant will be deprecated soon. The createAST methods should be consolidated to be apiLevel-agnostic. (2) The early return in ASTRewritingMethodDeclTest#testMethodTypeParameterAdds() must be at the beginning of the test. (3) Old problem: All the "public static Test setUpTest(..)" methods in the org.eclipse.jdt.core.tests.rewrite.describing package are wrong and should be removed.
Some more observations: (4) We have quite a few tests now that don't run in all JLS levels. Nevertheless, we still generate a test case, have the setUp and tearDown overhead, and produce an always-green test. A better solution is to avoid creation of tests for JLS levels they don't support. Implementation could be to parse the end of the test name: - If it's test*_only2 or test*_only2_3_4 etc., then only create for the given levels. - If it's test*_min3 or test*_min8, then create for levels that are at least the given number. - If it doesn't end with one of these patterns, create for all levels. (5) In ASTRewritingTest#setUp(), we call waitUntilIndexesReady(). This is unnecessary and slows down the tests by about .4s per test. Should be removed.
(In reply to comment #18) > The approaches in comment 16 and comment 17 look good, but: > > (1) The reusable infrastructure (suite(), getName(), apiLevel field, > deprecated references to AST.JLS[2,3,4] constants, createAST*() methods) > should be moved to ASTRewritingTest. Please also add and use a constant for > AST.JLS4, since that constant will be deprecated soon. The createAST methods > should be consolidated to be apiLevel-agnostic. Not the suite() method, perhaps. That will make us use some more reflection, which can be avoided. We would end up having little bit of boiler plate code in all test suites, but that's better than using reflection to find the right constructor, invoke it with parameters etc.
Created attachment 227205 [details] Updated rewrite test framework I have rewritten the rewrite test framework to to enable choosing select levels for any tests. At present, there are about 23 failures. That's because they indirectly trigger/access the older extra dimension property. Once the rewrite analyzer and co is fixed (via bug 396576), they should start passing too. Perhaps, the way forward is to release just the fix alone for bug 396576 and then release this patch. After that, all new tests can be modified to use the new framework and released. Some notes: 1. I see a bunch of setupSuite() - are they required any more? 2. ASTRewritingStatementsTest builds tests via call to buildModelTestSuite and I have removed that code. We will probably loose ordering of tests if any. I am not sure what the implications are at this point.
(In reply to comment #21) > Created attachment 227205 [details] > Updated rewrite test framework When this is cooked and ready to serve, please remember to hook this into RunAllJava8Tests - TIA.
Looks promising, but the many @deprecated tags and copies of suite() method implementations should go away. The buildModelTestSuite stuff doesn't look flexible enough to support our scenarios, so I'm fine with dropping that (at least I don't have time to fix that framework as well). The goal of this refactoring is that we only have to touch a few places when the next JLS* constant is added, and that these places can easily be found by searching for references to the latest JLS*. Now, we still have to update each and every suite() method, ASTRewritingTest#getASTLevelsForTest(String), etc. The JUnit framework also uses reflection to create instances of TestCase, so we can just as well do that. Alternatively, we could also create an ITestCreator#createTest(int apiLevel) interface and pass an instance of this to the generic test suite builder method, but that's overkill for a test framework. But a comment on ASTRewritingTest(String, int) that refers to the builder method would be nice. I like the format of the test name suffixes, but the parsing and processing should be made generic, so that we don't have to update it for new AST levels. Just split the levels at the "_", and then use each integer as AST level. I only see one setUpSuite() in ASTRewritingTest. That one just calls super and is therefore useless. And a non-private constant like this doesn't make sense, right? /** @deprecated using deprecated code */ final static int JLS2_INTERNAL = AST.JLS2;
Created attachment 227287 [details] Updated rewrite test framework I have got rid of all the suite methods in sub tests and moved the code that adds tests to suite to ASTRewritingTest using reflection. (In reply to comment #23) > I like the format of the test name suffixes, but the parsing and processing > should be made generic, so that we don't have to update it for new AST > levels. Just split the levels at the "_", and then use each integer as AST > level. This won't work for the _since case. We will have to know the upwards level anyway, don't we?
> This won't work for the _since case. We will have to know the upwards level > anyway, don't we? That's true. Not even I want to use reflection to find all fields named JLS* in the AST class ;-). But you should still get rid of the LEVEL_* and SINCE_* constants and just do the processing in addMethodsToSuite(..). Then we have just one list of JLS* levels where can add future levels.
Created attachment 227316 [details] Patch with Markus's proposed suggestion Markus, I have made changes as you suggested just for demonstration. I have to say I don't like that we end up with lot of duplication of code - code that creates new instance of tests. Every time a new level is added, we need to add that piece of code all previous _since versions. May be we can do it a bit smarter, but I don't see how that will provide us future-proof code. Agree, we would be touching just one method as against two and creating just one new constant against three. But the code changes are not any lesser. Considering that there are bugs blocked by this, I will go ahead and release the previous patch with few tweaks and comments so that it will help anyone who is adding the new level in future. But now that the infrastructure is provided in ASTRewritingTest, we can always make further changes in ASTRewritingTest without affecting other tests.
Release the support code here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=b0f361db26dd11f4be4974027111b70136b11d5b With this, our tests are stronger by little over 1500 in count and take 4+ minutes to complete! Manoj, you can update your test for this one and other rewrite bugs and release them. Also, please hook the new test in RunAllJava8Tests if it isn't done already.
(In reply to comment #27) > Release the support code here: > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?h=BETA_JAVA8&id=b0f361db26dd11f4be4974027111b70136b11d5b > > With this, our tests are stronger by little over 1500 in count and take 4+ > minutes to complete! You are misinforming the general public about our testing :) The test suite has 70000 tests and takes more than an hour to finish.
(In reply to comment #22) > When this is cooked and ready to serve, please remember to hook this into > RunAllJava8Tests - TIA. I have done this along with new test cases for bug 396576. The price for having it in the RunAllJava8Tests is that the rewrite tests are run on all levels, not just level 8. Manoj, you can just add the new test in the appropriate suite for the levels you want and post the new patch.
(In reply to comment #28) > (In reply to comment #27) > > With this, our tests are stronger by little over 1500 in count and take 4+ > > minutes to complete! > > You are misinforming the general public about our testing :) The test > suite has 70000 tests and takes more than an hour to finish. I meant to say 4+ extra minutes. The good news is, four extra minutes won't make a big difference to the current 1 hour, would it? :)
Created attachment 227386 [details] Proposed Fix Jay: Have enabled additional 7 more tests with this patch which includes the thrownExceptions tests as well. Have made another (unrelated to thrownexceptions) change for returntype tests enabling more tests (included in the 7). No additional tests were deemed necessary for the thrownExceptionTypes since the existing tests with the additional change tests the new types.
Created attachment 227396 [details] Proposed Fix incorporated a couple of consistency comments.
Thanks, Manoj, I have released the test fixes here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=f50d9bbda9dc7f3959092c23f3ad0c06015c43b5
(In reply to comment #26) > Created attachment 227316 [details] [diff] > Patch with Markus's proposed suggestion > > Markus, I have made changes as you suggested just for demonstration. I have > to say I don't like that we end up with lot of duplication of code - code > that creates new instance of tests. I agree that this solution is not better. But there's a much simpler way to implement this, see ASTRewritingTest#createSuite(Class): http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=c90773ded969a955dbf869e7d3cd599898423a64
