Community
Participate
Working Groups
When adding line comments to source, I find it useful to be able to keep them aligned in columns. Unfortunately, the code formatter removes the extra whitespace that makes them aligned, destroying the alignment. Example: public void storeSomething () throws IOException { buffer.clear (); buffer.putLong (0); // backlink to previous version of this object buffer.putInt (1); // version identifier buffer.flip (); // prepare to write channel.write (buffer); // write } It would be useful if an option could be added to enable this kind of alignment in the formatter. The alignment column would I think most usefully be on a per-method basis, but I can also see per-file being a useful option for some people.
Any support for "tabular" layouts would be great. Another example: void method foo(int arg1, String arg2, char arg3) { this.val1 = arg1; this.value2 = arg2; this.c = arg3; } Not sure if this is easily possible with the current implementation, but actually "align fields in columns" already realizes this style.
I have an issue with how single-line comments like these are handled that is very close to what this bug describes, but maybe not exactly, so I have some questions for the committers: So for background, code like this: buffer.putLong (0); // backlink to previous version of this object buffer.putInt (1); // version identifier buffer.flip (); // prepare to write channel.write (buffer); // write will currently get reformatted like this (no matter what your settings are): buffer.putLong (0); // backlink to previous version of this object buffer.putInt (1); // version identifier buffer.flip (); // prepare to write channel.write (buffer); // write the behavior I'd like to see is that it keeps the original whitespace (whether that lines up or not). I don't think it's necessary for the formatter to have an option to turn the 2nd snippet into the 1st for example... just to respect the indentation the author provided. So, my questions are: 1. should I file a new bug for this or can we use this bug to cover it? 2. if I were to create a patch that solved the issue as I described it and hooked that up to the existing behavior of "Enable line comment formatting" when it is turned off, would that be acceptable?
btw, I checked out the source and I think I know how to fix this, but the org.eclipse.jdt.core plugin has compile problems for me on both helios and indigo. I'm using head of :pserver:anonymous@dev.eclipse.org:/cvsroot/eclipse for org.eclipse.jdt.core and I've updated my helios install using the "Check for updates" feature... using the dev version Indigo has the same results. The wiki pages Building, and CVS_Howto don't help much.. is there a better resource for would-be contributors? The problem I'm getting is that INSTANCE cannot be resolved in lines like these: IEclipsePreferences defaultPreferences = DefaultScope.INSTANCE.getNode(JavaCore.PLUGIN_ID);
(In reply to comment #3) > The problem I'm getting is that INSTANCE cannot be resolved in lines like > these: > IEclipsePreferences defaultPreferences = > DefaultScope.INSTANCE.getNode(JavaCore.PLUGIN_ID); Where do you get this? I don't see any signs that DefaultScope.INSTANCE ever existed in the history. Nor do I see the line you quoted.
I see it on org.eclipse.jdt.internal.core.JavaCorePreferenceInitializer line 97 (among other places in the model source tree within org.eclipse.jdt.core).
(In reply to comment #5) > I see it on org.eclipse.jdt.internal.core.JavaCorePreferenceInitializer line 97 > (among other places in the model source tree within org.eclipse.jdt.core). Thanks, I see it now: that field has been recently added in v20101217 and jdt.core has been adapted to the new feature just a few days ago. At this point you may want to either switch jdt.core back to something like the tag v_B28 (which corresponds to 3.7 M4) or use a build > 20101217 HTH
(In reply to comment #2) > I have an issue with how single-line comments like these are handled that is > very close to what this bug describes, but maybe not exactly, so I have some > questions for the committers: > > So for background, code like this: > buffer.putLong (0); // backlink to previous version of this object > buffer.putInt (1); // version identifier > buffer.flip (); // prepare to write > channel.write (buffer); // write > > will currently get reformatted like this (no matter what your settings are): > buffer.putLong (0); // backlink to previous version of this object > buffer.putInt (1); // version identifier > buffer.flip (); // prepare to write > channel.write (buffer); // write > > the behavior I'd like to see is that it keeps the original whitespace (whether > that lines up or not). I don't think it's necessary for the formatter to have > an option to turn the 2nd snippet into the 1st for example... just to respect > the indentation the author provided. > > So, my questions are: > 1. should I file a new bug for this or can we use this bug to cover it? I guess what you're reporting here is just the same as the original reported issue. So there isn't a need for another bug. > 2. if I were to create a patch that solved the issue as I described it and > hooked that up to the existing behavior of "Enable line comment formatting" > when it is turned off, would that be acceptable? Sounds reasonable. Hope you're not getting the problem in comment 3 now, btw. Thanks!
Created attachment 186374 [details] patch that addresses comment 2 the attached patch preserves indentation of single-line comments when the formatting line comments option is disabled. the behavior when formatting of line comments is turned on is still to replace all the intervening whitespace with a single space.
You need to get org.eclipse.equinox.preferences from HEAD.
Sorry, I don't know what you mean with regards to org.eclipse.equinox.preference. I just checked out the latest nightly build and applied my patch and the tests pass, so it seems fine to me. If I check out org.eclipse.equinox.preference from HEAD I just get a readme.txt that says this: The Equinox project has moved to the RT cvs repository dev.eclipse.org:/cvsroot/rt. See http://wiki.eclipse.org/Equinox_Eclipse_to_RT_Migration for more information. I'm not sure how much more time I have to invest in seeing this through, but if my patch needs changes it'd help to get a bit more detail of what the issues are with it. Thanks.
any update on this?
Frédéric, would you have a chance to review that patch? Thanks.
(In reply to comment #8) > Created attachment 186374 [details] > patch that addresses comment 2 > > the attached patch preserves indentation of single-line comments when the > formatting line comments option is disabled. the behavior when formatting of > line comments is turned on is still to replace all the intervening whitespace > with a single space. (In reply to comment #12) > Frédéric, would you have a chance to review that patch? > > Thanks. Sorry, I have to reject it. First because it hasn't been implemented as a new preference but modifying an existing one instead. Second, the change does not verify that not deleting the spaces is done only before line comments, hence I immediately suspected that it would have undesired side effects. I verified that by running all formatter tests (RunAllFormatterTests) and I actually got one failure in FormatterRegressionTests.test713(). Moreover, I got more than 8,000 failures while running massive regression tests *only* for the Eclipse 3.0 workspace (~ 10,000 units) when the 'format line comments' preference is turned off (please have a look at the javadoc of FormatterMassiveRegressionTests test suite to see how to run those massive tests...). That explains why this is a no-go for this patch.
(In reply to comment #13) > (In reply to comment #8) > > Created attachment 186374 [details] [details] > > patch that addresses comment 2 > > > > the attached patch preserves indentation of single-line comments when the > > formatting line comments option is disabled. the behavior when formatting of > > line comments is turned on is still to replace all the intervening whitespace > > with a single space. > > (In reply to comment #12) > > Frédéric, would you have a chance to review that patch? > > > > Thanks. > > Sorry, I have to reject it. > > First because it hasn't been implemented as a new preference but modifying an > existing one instead. > > Second, the change does not verify that not deleting the spaces is done only > before line comments, hence I immediately suspected that it would have > undesired side effects. I verified that by running all formatter tests > (RunAllFormatterTests) and I actually got one failure in > FormatterRegressionTests.test713(). > > Moreover, I got more than 8,000 failures while running massive regression tests > *only* for the Eclipse 3.0 workspace (~ 10,000 units) when the 'format line > comments' preference is turned off (please have a look at the javadoc of > FormatterMassiveRegressionTests test suite to see how to run those massive > tests...). > > That explains why this is a no-go for this patch. Ok I'm done with this. I could go through a bunch of reasons why, but that would be as big a waste of time as working on this bug has been. If you want to close this bug as wontfix or whatever that would be fine. I personally will think twice before contributing to eclipse again, as it really hasn't been much fun.
Ray provided you with a well coded patch and unit test. You made no effort to tell him (in advance) what you expected of him or the patch. You could have easily laid out the requirements for a new property or which tests needed to pass and how to run them. Instead, all you did was make him jump through silly hoops, like checking files out of non-existent directories, and make this entire process as unfriendly as possible. Do you really want to improve Eclipse or is running the formatter and having it mess things up actually desired behavior? I could understand your responses if Ray was some developer who didn't know how to code and was just being a pain about some absurd thing, but Ray has been extremely patient and is an *excellent* developer who wanted to help improve a tool that we all use for hours on end, every single day. This is clearly a bug in Eclipse and now it will never get fixed. Even worse, you alienated at least two developers from ever wanting to help out with Eclipse. Nice job representing the Eclipse 'community' Frederic and Oliver. Be sure to pass a link to this issue along to your bosses for your next performance review.
(In reply to comment #10) > Sorry, I don't know what you mean with regards to > org.eclipse.equinox.preference. I misunderstood your post. Some changes have been made to the preference bundle lately and this is causing grief with some builds because the field INSTANCE on the difference scopes don't exist.
(In reply to comment #14) > > Ok I'm done with this. I could go through a bunch of reasons why, but that > would be as big a waste of time as working on this bug has been. If you want > to close this bug as wontfix or whatever that would be fine. I personally > will think twice before contributing to eclipse again, as it really hasn't > been much fun. I'm definitely sorry if have hurt you by rejecting your patch, but any change in any software has to avoid regression. We do have lot of tests in JDT/Core to verify that this is always the case and the patch you proposed didn't reach this goal. Contributing is real good thing and definitely helps Eclipse developers, but we cannot accept patches which would introduce regressions and we will never change this rule. Perhaps the mood of my answer was quite rude, but as I'm no longer on Eclipse development since the beginning of January, I took extra time to review your patch and I surely didn't take enough time to minimize the impact of this reject. Again, I'm sincerely sorry if that made you feel we do not like contribution, but having patches not passing review is something quite normal for a developer. I often experiment it and I think it's the normal process during the development stage. A few people are able to deliver right code at the first try. So, I would have only word, hoping it will not hurt again: please try it again :-)
(In reply to comment #3) > The problem I'm getting is that INSTANCE cannot be resolved in lines like > these: > IEclipsePreferences defaultPreferences = > DefaultScope.INSTANCE.getNode(JavaCore.PLUGIN_ID); I was wrong with my last comment (comment 16). So I was quite right to tell you that you should get the preference bundle from HEAD. The fields INSTANCE have been added on all scopes types and therefore you need to check out the preference bundle from HEAD to get a version that defines them. Maybe I was not clear enough about that.
(In reply to comment #15) > This is clearly a bug in Eclipse and now it will never get fixed. Even worse, > you alienated at least two developers from ever wanting to help out with > Eclipse. Nice job representing the Eclipse 'community' Frederic and Oliver. Be > sure to pass a link to this issue along to your bosses for your next > performance review. Let's comment this bug before it goes off track with offensive comments. To be clear this is not a bug. This is a missing feature. This bug is set as an enhancement and I think this is the right way to categorize it. Let me know what I did wrong. Tell me my mistake. I simply asked Frédéric to review the given patch. Frédéric worked on the code formatter for a long time and I believe he is the most appropriate person to review a patch. Now he moved to a different project so reviewing patches for JDT/Core is done in his spare time. I thank him for doing it so fast. I see nothing wrong with what he did. Frédéric gave feedback why the patch is not ready to be released. I read again what he wrote and I saw nothing that could have been offensive. In general, patches are going through several iterations before they can be released. Other people have contributed patches and we always follow the same process of iterating over the patch till it is ready to be released. This is a *normal* process. Now let's come back to the feedbacks provided by Frédéric. I think the misunderstanding about reusing an existing option comes from the comment 7. Your feature is about keeping the line comments on subsequent lines aligned after formatting. This is quite different from the purpose of the option you used. So I think a new option should have been used. I missed the comment from Ayushman and we didn't discuss about it. This is just about adding a new option, so this is clearly not a big deal to change. This being said, a patch should at least pass existing tests or at least raise a concern why the tests are failing and what should be changed to get them to pass. I think Ray had issues with setting up properly his workspace with the missing INSTANCE fields. So this could explain why he didn't run the tests. I thought that the explanations in http://wiki.eclipse.org/Equinox_Eclipse_to_RT_Migration were good enough to get the workspace properly set up. In comment 10, Ray, at the end, asked for feedback on his patch. So Frédéric gave some and now you are alienated with this. Tell me why. Now when you say this: >Ray provided you with a well coded patch and unit test. You made no effort to >tell him (in advance) what you expected of him or the patch. You could have >easily laid out the requirements for a new property or which tests needed to >pass and how to run them. Ray never asked about how to run the tests. We could therefore assume he knew how to do that. We never said the patch will be thrown away. This is what you said. We only need a few iteration on it to make it good enough to be released. To add a new option you need to: 1) Add a new constant in: org.eclipse.jdt.core.formatter.DefaultCodeFormatterConstants with its javadoc 2) Use the new constant to update: org.eclipse.jdt.internal.formatter.DefaultCodeFormatterOptions 3) Provide a patch that is using the new option 4) Provide a regression test that shows the new option works as expected 5) Run all tests to make sure that no new regressions have been introduced: thi includes all tests from JDT/Core, but also the tests from JDT/UI and JDT/Text. Do you need more details ? I'd like to understand why Ray overreacted in comment 14. Would you prefer that we don't verify patches and we released them as is ? We never said that Ray is a bad developer, nor he doesn't know how to write quality code. Next time, before you react the way you did, you could simply send me a message either as a comment in the bug report or privately to my email. I also provided patches to other components in Eclipse and they have not always be accepted as is. I didn't go nuts about it. I improved them and they were finally released. Now let's hope the tone of the next comments will be a bit more relaxed. I would be pleased to guide Ray to install the preference bundle from the rt repository.
(In reply to comment #17) > (In reply to comment #14) > > > > Ok I'm done with this. I could go through a bunch of reasons why, but that > > would be as big a waste of time as working on this bug has been. If you want > > to close this bug as wontfix or whatever that would be fine. I personally > > will think twice before contributing to eclipse again, as it really hasn't > > been much fun. > > I'm definitely sorry if have hurt you by rejecting your patch, but any change > in any software has to avoid regression. We do have lot of tests in JDT/Core to > verify that this is always the case and the patch you proposed didn't reach > this goal. > > Contributing is real good thing and definitely helps Eclipse developers, but we > cannot accept patches which would introduce regressions and we will never > change this rule. > > Perhaps the mood of my answer was quite rude, but as I'm no longer on Eclipse > development since the beginning of January, I took extra time to review your > patch and I surely didn't take enough time to minimize the impact of this > reject. > > Again, I'm sincerely sorry if that made you feel we do not like contribution, > but having patches not passing review is something quite normal for a > developer. I often experiment it and I think it's the normal process during the > development stage. A few people are able to deliver right code at the first > try. > > So, I would have only word, hoping it will not hurt again: please try it again > :-) Thanks for pointing out what was wrong with that patch. I probably did overreact a bit in my earlier comment, and I apologize for that. I agree that rejecting the patch is the right thing to do if it's breaking tests, which apparently it does. My frustration stems from the unclear guidelines on what would be a good solution to this bug, and also no guidelines on what tests need to pass and how to run them. Part of that is a eclipse documentation issue. I don't expect committers to handhold every developer submitting patches, but I couldn't find useful info on the eclipse wiki on how to contribute, build or test eclipse, and it's frustrating to hit so many avoidable roadblocks along the way after being simply told to submit a patch if you want this fixed. That said, I'm not trying to cast blame; I know we're all busy and contributing on our spare time, so thanks for the time you've put into reviewing this. I agree that creating a separate preference for this (i.e. preserve_end_of_line_comment_indentation) is probably an easier fix than changing existing behavior, especially if it breaks a bunch of existing tests. It still seems confusing to me that "comment_format_line_comment=false" would change comment indentation, but if it could be turned off as a separate preference, that seems fine. I also assume though that a new preference would need to be surfaced in the UI in order to be accepted as a patch, which increases the scope of the change more than I'm comfortable with taking on myself. If doing just the backend change is acceptable I could possibly take another look, otherwise I'll just hope someone else takes up the mantle on this bug.
Ok, I hope we can come together and get this one banged out. I'd hate to see it languish in the state it's in now, so I'll do what I can to get an acceptable patch. In fact I have a patch now that uses a new preference and it passes all tests in FormatterRegressionTests, including test713() but so far I'm not able to run the other tests to see if it's worth posting / reviewing. I've been running the formatter tests by right-clicking and doing Run As>Junit Plug-in Test, but that doesn't work for me for FormatterMassiveRegressionTests, so I haven't been able to run it. I read the instructions in the javadoc and saw about unzipping the test source and adding this to the arguments: -DinputDir=/test/full-source-R3_0 -DoutputDir=/tmp -DlogDir=/tmp So I did that but all I ever get is this: java.lang.IllegalArgumentException: Bundle "org.eclipse.jdt.core.tests.model" not found. Possible causes include missing dependencies, too restrictive version ranges, or a non-matching required execution environment. This is followed by a stacktrace which I can provide, but I'm guessing it's just a classpath issue that I don't know how to fix. Perhaps the launch file shouldn't be setup as a Junit Plug-in test? I tried a few things but nothing seemed to help. Maybe it's easiest if someone could attach a working .launch file and I could edit it to work for me? Or maybe it's too environment specific and I just need to learn the process for creating this .launch file? As for the unit tests for the other projects like JDT / UI etc I assume those are run from the test.xml ant file? I tried that and get this: ray@featuritis:~/indigo-nightly/org.eclipse.jdt.ui.tests $ ant -f test.xml -Declipse-home=/Applications/eclipse-indigo-nightly/ Buildfile: /Users/ray/indigo-nightly/org.eclipse.jdt.ui.tests/test.xml init: leaksuite: BUILD FAILED /Users/ray/indigo-nightly/org.eclipse.jdt.ui.tests/test.xml:48: The following error occurred while executing this line: java.io.FileNotFoundException: /Applications/eclipse-indigo-nightly/plugins/org.eclipse.test/library.xml (No such file or directory) at java.io.FileInputStream.open(Native Method) at java.io.FileInputStream.<init>(FileInputStream.java:106) at org.apache.tools.ant.helper.ProjectHelper2.parse(ProjectHelper2.java:268) at org.apache.tools.ant.helper.ProjectHelper2.parse(ProjectHelper2.java:177) at org.apache.tools.ant.ProjectHelper.configureProject(ProjectHelper.java:82) at org.apache.tools.ant.taskdefs.Ant.execute(Ant.java:393) at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:291) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106) at org.apache.tools.ant.Task.perform(Task.java:348) at org.apache.tools.ant.Target.execute(Target.java:390) at org.apache.tools.ant.Target.performTasks(Target.java:411) at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1397) at org.apache.tools.ant.Project.executeTarget(Project.java:1366) at org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41) at org.apache.tools.ant.Project.executeTargets(Project.java:1249) at org.apache.tools.ant.Main.runBuild(Main.java:801) at org.apache.tools.ant.Main.startAnt(Main.java:218) at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280) at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109) Any help you could provide in getting the tests to run would be appreciated. I also haven't tried to surface this new preference in the eclipse UI. If that's a requirement I may need some guidance on how to get started on that as well. Thanks.
(In reply to comment #21) > Ok, I hope we can come together and get this one banged out. I'd hate to see it > languish in the state it's in now, so I'll do what I can to get an acceptable > patch. In fact I have a patch now that uses a new preference and it passes all > tests in FormatterRegressionTests, including test713() but so far I'm not able > to run the other tests to see if it's worth posting / reviewing. > Passing all formatter is a real good news :-) Please also run all model tests (RunModelTests test suite) to be sure that there's no other side effect. > I've been running the formatter tests by right-clicking and doing Run As>Junit > Plug-in Test, but that doesn't work for me for FormatterMassiveRegressionTests, > so I haven't been able to run it. I read the instructions in the javadoc and > saw about unzipping the test source and adding this to the arguments: > -DinputDir=/test/full-source-R3_0 > -DoutputDir=/tmp > -DlogDir=/tmp > So I did that but all I ever get is this: > java.lang.IllegalArgumentException: Bundle "org.eclipse.jdt.core.tests.model" > not found. Possible causes include missing dependencies, too restrictive > version ranges, or a non-matching required execution environment. > This is followed by a stacktrace which I can provide, but I'm guessing it's > just a classpath issue that I don't know how to fix. Perhaps the launch file > shouldn't be setup as a Junit Plug-in test? I tried a few things but nothing > seemed to help. Maybe it's easiest if someone could attach a working .launch > file and I could edit it to work for me? Or maybe it's too environment specific > and I just need to learn the process for creating this .launch file? > Hum, really weird as FormatterMassiveRegressionTests belongs to org.eclipse.jdt.core.tests.model plug-in!? Which test projects do you have loaded in your workspace? You should have all JDT/Core test plugins, i.e.: - org.eclipse.jdt.core.tests.builder - org.eclipse.jdt.core.tests.compiler - org.eclipse.jdt.core.tests.model - org.eclipse.jdt.core.tests.performance (necessary to get the full-source-R3_0.zip file) If this not the case, you can load them from dev.eclipse.org:/cvsroot/eclipse repository as anonymous. > As for the unit tests for the other projects like JDT / UI etc I assume those > are run from the test.xml ant file? I tried that and get this: > > ray@featuritis:~/indigo-nightly/org.eclipse.jdt.ui.tests $ ant -f test.xml > -Declipse-home=/Applications/eclipse-indigo-nightly/ > Buildfile: /Users/ray/indigo-nightly/org.eclipse.jdt.ui.tests/test.xml > > init: > > leaksuite: > > BUILD FAILED > /Users/ray/indigo-nightly/org.eclipse.jdt.ui.tests/test.xml:48: The following > error occurred while executing this line: > java.io.FileNotFoundException: > /Applications/eclipse-indigo-nightly/plugins/org.eclipse.test/library.xml (No > such file or directory) > at java.io.FileInputStream.open(Native Method) > at java.io.FileInputStream.<init>(FileInputStream.java:106) > at > org.apache.tools.ant.helper.ProjectHelper2.parse(ProjectHelper2.java:268) > at > org.apache.tools.ant.helper.ProjectHelper2.parse(ProjectHelper2.java:177) > at > org.apache.tools.ant.ProjectHelper.configureProject(ProjectHelper.java:82) > at org.apache.tools.ant.taskdefs.Ant.execute(Ant.java:393) > at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:291) > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) > at > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) > at java.lang.reflect.Method.invoke(Method.java:597) > at > org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106) > at org.apache.tools.ant.Task.perform(Task.java:348) > at org.apache.tools.ant.Target.execute(Target.java:390) > at org.apache.tools.ant.Target.performTasks(Target.java:411) > at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1397) > at org.apache.tools.ant.Project.executeTarget(Project.java:1366) > at > org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41) > at org.apache.tools.ant.Project.executeTargets(Project.java:1249) > at org.apache.tools.ant.Main.runBuild(Main.java:801) > at org.apache.tools.ant.Main.startAnt(Main.java:218) > at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280) > at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109) > > Any help you could provide in getting the tests to run would be appreciated. > I also haven't tried to surface this new preference in the eclipse UI. If > that's a requirement I may need some guidance on how to get started on that as > well. Thanks. For JDT/UI tests you should also be able to run them using the test suite org.eclipse.jdt.ui.tests.AutomatedSuite. For that you need to have the last version (current last version: v20110111-0800) of the org.eclipse.jdt.ui.tests plug-in loaded in your workspace (from the same dev.eclipse.org repository). I also encourage you to run JDT/Text tests which is the plugin having also formatting tests. The test suite is org.eclipse.jdt.text.tests.JdtTextTestSuite and you should be able to run it as JUnit test as soon as the last version of the following plug-ins will be loaded in your workspace: - org.eclipse.filebuffers.tests (current last version: v20100824-0800) - org.eclipse.jdt.text.tests (current last version: v20110111-0800) - org.eclipse.jface.text.tests (current last version: v20100824-0800) - org.eclipse.text.tests (current last version: v20101123-0800) Note that the last versions of JDT/JUI and JDT/Text tests plugins should be used with the last available I-build otherwise you'll have compatibility issues... HTH
(In reply to comment #21) > I also haven't tried to surface this new preference in the eclipse UI. If > that's a requirement I may need some guidance on how to get started on that as > well. Thanks. Have a look at the JDT/Core and JDT/UI patches written for a simple example of adding a new formatter preference: 1) bug 150741 (JDT/Core) 2) bug 303163 (JDT/UI)
> Hum, really weird as FormatterMassiveRegressionTests belongs to > org.eclipse.jdt.core.tests.model plug-in!? Which test projects do you have > loaded in your workspace? You should have all JDT/Core test plugins, i.e.: > - org.eclipse.jdt.core.tests.builder > - org.eclipse.jdt.core.tests.compiler > - org.eclipse.jdt.core.tests.model > - org.eclipse.jdt.core.tests.performance (necessary to get the > full-source-R3_0.zip file) > Yeah it makes no sense... I played with some more settings and now I put it into a bad state where running any test gives me this error. I think it has to do with how I dealt with the "API baseline isn't set for this workspace" error I was getting. I turned that to "ignore" which helped before, but I changed something that made it break and I'm not sure what. I may have to scrap my workspace and start over. What is the proper way to deal with that API baseline error? Or is the fact that I'm getting that on a fresh checkout a bad sign already?
(In reply to comment #24) > > Hum, really weird as FormatterMassiveRegressionTests belongs to > > org.eclipse.jdt.core.tests.model plug-in!? Which test projects do you have > > loaded in your workspace? You should have all JDT/Core test plugins, i.e.: > > - org.eclipse.jdt.core.tests.builder > > - org.eclipse.jdt.core.tests.compiler > > - org.eclipse.jdt.core.tests.model > > - org.eclipse.jdt.core.tests.performance (necessary to get the > > full-source-R3_0.zip file) > > > Yeah it makes no sense... I played with some more settings and now I put it > into a bad state where running any test gives me this error. I think it has to > do with how I dealt with the "API baseline isn't set for this workspace" error > I was getting. I turned that to "ignore" which helped before, but I changed > something that made it break and I'm not sure what. I may have to scrap my > workspace and start over. What is the proper way to deal with that API > baseline error? Or is the fact that I'm getting that on a fresh checkout a bad > sign already? Not sure this is related to the fact that you didn't have an API baseline... BTW, to set properly the API baseline for 3.7 I-builds: 1) install a 3.6.0 build somewhere on your disk 2) create a new baseline (let say 3.6.0) with the location pointing to the eclipse directory of the 3.6.0 installed build 3) select the created baseline as default But I stronger advice to restart from a brand new workspace and load JDT/Core project (org.eclipse.jdt.core) and all test projects I mentioned in comment 22 + org.eclipse.test.performance + org.eclipse.test.performance.win32 (if you're running tests on a windows box) Then everything should work properly.
(In reply to comment #25) > (In reply to comment #24) > > > Hum, really weird as FormatterMassiveRegressionTests belongs to > > > org.eclipse.jdt.core.tests.model plug-in!? Which test projects do you have > > > loaded in your workspace? You should have all JDT/Core test plugins, i.e.: > > > - org.eclipse.jdt.core.tests.builder > > > - org.eclipse.jdt.core.tests.compiler > > > - org.eclipse.jdt.core.tests.model > > > - org.eclipse.jdt.core.tests.performance (necessary to get the > > > full-source-R3_0.zip file) > > > > > Yeah it makes no sense... I played with some more settings and now I put it > > into a bad state where running any test gives me this error. I think it has to > > do with how I dealt with the "API baseline isn't set for this workspace" error > > I was getting. I turned that to "ignore" which helped before, but I changed > > something that made it break and I'm not sure what. I may have to scrap my > > workspace and start over. What is the proper way to deal with that API > > baseline error? Or is the fact that I'm getting that on a fresh checkout a bad > > sign already? > > Not sure this is related to the fact that you didn't have an API baseline... > BTW, to set properly the API baseline for 3.7 I-builds: > 1) install a 3.6.0 build somewhere on your disk > 2) create a new baseline (let say 3.6.0) with the location pointing to the > eclipse directory of the 3.6.0 installed build > 3) select the created baseline as default > > But I stronger advice to restart from a brand new workspace and load JDT/Core > project (org.eclipse.jdt.core) and all test projects I mentioned in comment 22 > + org.eclipse.test.performance > + org.eclipse.test.performance.win32 (if you're running tests on a windows box) > > Then everything should work properly. Thanks for your help. Unfortunately I'm at a loss for the moment on how to make the tests work again. I was originally working off of HEAD and I think the problem probably started when I reverted the projects to what was in the repo to check that applying my patch on a clean copy of the latest code worked. To fix this I first tried updating eclipse to the latest nightly build and checking out HEAD of everything into a new workspace. Then I tried using the v_B32 versions (along with the latest tagged version of everything else) against both I20010111-0800 and I20110114-1330 integration builds. For each of these tests I set the API Baseline to my "Build id: 20100917-0705" helios install and attempted to run RunFormatterTests via Run As > Junit Plug-In Test (on clean code without my changes applied). I always get the exception below now. java.lang.IllegalArgumentException: Bundle "org.eclipse.jdt.core.tests.model" not found. Possible causes include missing dependencies, too restrictive version ranges, or a non-matching required execution environment. at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.getClassLoader(RemotePluginTestRunner.java:77) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.getTestClassLoader(RemotePluginTestRunner.java:71) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.loadClass(RemoteTestRunner.java:693) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.loadClasses(RemoteTestRunner.java:429) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:452) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62) at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.run(CoreTestApplication.java:23) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:324) at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:587) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:198) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:344) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:324) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:622) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:577) at org.eclipse.equinox.launcher.Main.run(Main.java:1410) at org.eclipse.equinox.launcher.Main.main(Main.java:1386) I'll attach my new patch since I'm fairly confident it won't cause regressions. I'll still need to resolve this problem though, both to verify that the tests pass and to expose the new option in the UI. The only thing I can think of to try is to use HEAD again once there's a new nightly build up. The patch is simpler now, it now just does this: // Add pending space if necessary if (this.pendingSpace) { - addInsertEdit(currentTokenStartPosition, " "); //$NON-NLS-1$ + if (this.formatter.preferences.comment_preserve_trailing_line_comment_indentation) { + addInsertEdit(currentTokenStartPosition, new String(this.lastLineComment.leadingSpaces)); + } else { + addInsertEdit(currentTokenStartPosition, " "); //$NON-NLS-1$ + } } Along with the code for supporting the new preference. I think should be pretty safe, but once I get my environment working again I can tell you for sure.
Created attachment 186960 [details] patch that hopefully passes all the tests
(In reply to comment #27) > Created attachment 186960 [details] > patch that hopefully passes all the tests Was able to verify FormatterMassiveRegressionTests pass with this patch on my work machine that has a working environment. Tried to run AutomatedSuite in JDT/UI but got this: java.lang.UnsatisfiedLinkError: Cannot load 64-bit SWT libraries on 32-bit JVM presumably because I'm running on Mac and the Java 1.5 implementation is 32 bit there. Is there a way to run these tests on Mac or should I install an ubuntu VM to test this?
I'll check the UI tests. I'll also review the patch today.
(In reply to comment #29) > I'll check the UI tests. > I'll also review the patch today. Ok, thanks Olivier. btw, the UI tests run for me once I change the project to use the 1.6 jvm, but they ran out of permgen before they finished. Running them again now with more permgen. Will look into adding the new preference to the UI soon. Thanks again for looking into this patch.
Created attachment 187048 [details] patch to add new preference to the ui this patch just contains changes for ui. when launching this, the preview code on the comments tab didn't match what appears in the code, so I suspect there's a different place that needs to get updated, but the new checkbox shows up and functions.
On the first patch, the default value for the new option must be false as this is the current setting. This is actually what the code does. I'll post a new patch for this soon.
Created attachment 187113 [details] Proposed fix + regression tests Updated patch. I'll rerun all tests with it.
(In reply to comment #33) > Created attachment 187113 [details] > Proposed fix + regression tests > > Updated patch. I'll rerun all tests with it. The patch looks good and the observed behavior while running massive regression tests looks good too. Hence, I'm OK with this proposal :-) Olivier, I think that the preference initialization needs also to be done in the setJavaConventionsSettings() method of DefaultCodeFormatterOptions. Other than that it's a +1 for me
Created attachment 187195 [details] Proposed fix + regression tests Updated patch with constant renaming and according to Frédéric's comment.
Fixed and released for 3.7M5. Thanks for your time and patches, Ray.
(In reply to comment #35) > Created attachment 187195 [details] > Proposed fix + regression tests > > Updated patch with constant renaming and according to Frédéric's comment. Great new! Thanks guys for all your help with this.
(In reply to comment #37) > Great new! Thanks guys for all your help with this. I thank you for the effort you put into this bug. I particularly appreciated your comment 20. Hope you can further contribute patch to the JDT/Core project.
Verified for 3.7M5 using build I20110124-1800.
Verified again for 3.7 RC0 using Build id: I20110428-0848
