Bug 418920

Comment 1 Srikanth Sankaran 2013-10-08 09:38:26 EDT

Shankha, please work with Jay as he has the context.

Comment 2 Walter Harley 2013-10-09 01:43:45 EDT

A little more context:

These tests were written because during APT development, there were a lot of cases where it wasn't clear what the correct behavior *should* be - the APT spec was rather vague.  So our criterion was we should try to do the same thing that javac did; and the tests were intended to detect differences between javac and Eclipse.

If there is a test that is now failing for both sides of that divide, it indicates a spec difference, and the test is probably in error.

Comment 3 shankha banerjee 2013-11-08 01:10:13 EST

Created attachment 237293 [details]
Patch

There are two problems related to this bug:

(1) testTypesWithSystemCompiler and testTypesWithEclipseCompiler
are due to the same problem. In JRE8, the class HashMap#HashIterator
is not generic. I believe it was previously generic (JRE7 ?)

Tests should be rewritten to avoid using non-standard classes.
-------------------------------------------------------------------------

The tests have been re written to avoid non standard classes.


(2) The other bug is due to as Jay mentioned how the javadoc is differently
formatted.

So the tests should be rewritten to convert all tabs to spaces, strip out all
non-significant white spaces before comparing.

--------------------------------------------------------------------------

I have not re written any of the tests. I have made changes to make eclipse 
compiler mimic behavior of Sun compiler.

The tabs ('\t') were being converted to white spaces. That has been changed and 
now the tabs are treated as tabs.


org.eclipse.jdt.compiler.apt.tests/lib/apttestprocessors.jar

has to be re generated. 

Tests results are all clean.

Thanks

Comment 4 shankha banerjee 2013-11-08 01:32:39 EST

Created attachment 237296 [details]
Patch

Forgot to pull. Have uploaded a new patch.

Thanks

Comment 5 Srikanth Sankaran 2013-11-08 05:46:56 EST

(In reply to shankha banerjee from comment #4)

Once all the tests are green , please edit org.eclipse.jdt.core.tests.RunAllJava8Tests and remove Java8ElementsTests.class 
and add a new entry for all APT tests. Thanks!

Comment 6 shankha banerjee 2013-11-08 06:32:24 EST

Created attachment 237307 [details]
Patch

org.eclipse.jdt.core.tests.RunAllJava8Tests is all green.

Have added org.eclipse.jdt.compiler.apt.tests to org.eclipse.jdt.core.tests.RunAllJava8Tests.

Thanks

Comment 7 Jay Arthanareeswaran 2013-11-11 05:40:40 EST

(In reply to shankha banerjee from comment #6)
> Created attachment 237307 [details]
> Patch

Patch looks good. Just a few minor comments:

1. Consider using \t instead of directly using 'tab' in the touched tests, just to keep it consistent with other similar instances. It's enough if this is done for only the code we are touching.
2. Add the JCP declaration to the copyright of Outer.java.
3. Move the apt test suite to getAllTestClasses)() from getConverterTestClasses as it sounds more appropriate.

Comment 8 shankha banerjee 2013-11-11 06:16:43 EST

Created attachment 237359 [details]
Patch

> 1. Consider using \t instead of directly using 'tab' in the touched tests,
> just to keep it consistent with other similar instances. It's enough if this
> is done for only the code we are touching.

Done.

> 2. Add the JCP declaration to the copyright of Outer.java.

Done

> 3. Move the apt test suite to getAllTestClasses)() from
> getConverterTestClasses as it sounds more appropriate.

Done.

Thanks

Comment 9 Jay Arthanareeswaran 2013-11-11 07:02:40 EST

Thanks, Shankha, I have pushed the changes here:

http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=8263b1bf4e0979f16664b3ec7a3f63f9c444d8c1