Bug 230364 - [formatter] Test failure in CleanUpStressTest after changes to comment formatter
Summary: [formatter] Test failure in CleanUpStressTest after changes to comment formatter
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4 RC1   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-06 06:50 EDT by Benno Baumgartner CLA
Modified: 2008-05-13 08:59 EDT (History)
2 users (show)

See Also:
Olivier_Thomann: review+


Attachments
required changes to make the test green again (4.39 KB, patch)
2008-05-06 06:50 EDT, Benno Baumgartner CLA
no flags Details | Diff
Proposed patch (3.12 KB, patch)
2008-05-06 13:32 EDT, Frederic Fusier CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benno Baumgartner CLA 2008-05-06 06:50:41 EDT
Created attachment 98789 [details]
required changes to make the test green again

I20080502-0100

Recent changes in the comment formatter (bug 230184) lead to a failing CleanUpStressTest. We might need to adjust the test.
Comment 1 Benno Baumgartner CLA 2008-05-06 06:51:28 EDT
Needs to be fixed for RC1
Comment 2 Benno Baumgartner CLA 2008-05-06 06:53:51 EDT
Frederic? Can you review? IMHO this looks wrong. The comment alignment was better before.
Comment 3 Frederic Fusier CLA 2008-05-06 11:59:14 EDT
The problem was when splitting line comments, the new comment on the line after started after the first one. E.g:
     // comment needs to be split
was formatted as:
     // comment needs
        // to be split

In this small example, you can see that the second line is longer than the first one, but it would have been really hard for the formatter to figure out that in case of the line max length was just after the word 'needs', the comments would need to be split in three lines:
     // comment needs
        // to be
        // split

So, I changed the rule and now the comment formatter adds one indentation less on the second line while splitting and so format as follows:
     // comment needs
    // to be split

If you definitely think this is worst, then please reassign the bug to me instead of applying the changes on CleanUpStressTests and I'll try to see I could see for another fix for this issue...
Comment 4 Frederic Fusier CLA 2008-05-06 13:01:40 EDT
This was due to a side effect of fix for bug 230184.
Comment 5 Frederic Fusier CLA 2008-05-06 13:18:15 EDT
It seems that my explanation in comment 1 was not really clear... I'll try to make it better!

Set the formatter comment line max length to 40. Set the Java editor to use a non-proportional font (e.g. Courier New), text margin to 40 and display the margin in the editor.

Now, open a java editor on the following simple test case:

public class X { // This comment will need to over the max line length
}

If the formatter preferences was changed from Eclipse built-in, then the test case will be formatted as follow:

public class X { // This comment will
					// need to over the max
					// line length
}

Note that, in bugzilla, tabs are 8 characters (and I cannot see any way to change it) so the pasted formatted output looks strange but with 4 chars per tab it would have looked like:

public class X { // This comment will
                    // need to over the max
                    // line length

}

The problem with this output is that the second line is still over 40 chars. I tried to fix this issue but it seems that my fix was not good enough.

In fact the expected formatted output should be:

public class X { // This comment will
                    // need to over the
                    // max line length
}
Comment 6 Frederic Fusier CLA 2008-05-06 13:32:39 EDT
Created attachment 98890 [details]
Proposed patch

While fixing bug 230184, the initial indentation level was wrongly modified while printing line comments.
Comment 7 Frederic Fusier CLA 2008-05-06 13:35:28 EDT
Olivier, could you please review?

We need to release the patch before today's integration build otherwise the JDT/UI CleanUpStressTests will fail.

Note that this change has no impact on Eclipse 3.0 full-src and Ganymede workspaces tests, I still get respectively 5 and 21 failures (same numbers than last released fix - see bug 230230)
Comment 8 Olivier Thomann CLA 2008-05-06 13:39:17 EDT
+1
Comment 9 Frederic Fusier CLA 2008-05-06 13:44:10 EDT
Released for 3.4RC1 in HEAD stream.
Comment 10 Eric Jodet CLA 2008-05-13 08:59:55 EDT
Verified for 3.4RC1 using build I20080510-2000.