Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix. 
I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...

                        ...jim
On 1/31/17 2:34 PM, Jim Graham wrote:
For an application to run into this same issue they'd have to expect 
getCompSizes() to return data for components that
don't exist.  It's unlikely they would use that data if they really understand 
the objects.  While that would be odd, I
guess I can see someone might be constructing all of their CM's from an array 
of 4 components regardless of the number
of actual components and we'd be happily remembering the useless extra 
components and returning an array of 4 from
getCompSizes().  As I said, they shouldn't really be reading and interpreting 
those extra components for any image
processing, but I can imagine that they might do something like create a 
variant CM by calling the CompSizes() and
copying them into a new array to construct a new CM with modifications.  They 
might just robotically always copy 4
values without really checking how many are valid.  That's a stretch, and their 
code is weak.  I can conceive of how
this might happen, but I really have no idea how likely it is...

            ...jim

On 1/30/17 3:56 PM, Phil Race wrote:
Sounds like we should at least try to get the tests updated so they only test 
what the spec. says.
Although it does indicate that there is at least a chance that application code 
might also fail due to similar
assumptions.
Does #1 not fail with the previous iteration of this change too ?

-phil.

On 01/30/2017 01:40 PM, Jim Graham wrote:
Hmmm.  Sounds like the test cases were written based on bugs in the 
implementation.  I'm not sure what the best tactic
is here for the short term for getting this in, but many of these changes 
should eventually be considered bugs in the
tests.  Is it acceptable to break API tests like this at the last minute even 
if the tests are at fault?  Phil?

Notes on specific instances below...

On 1/30/17 2:22 AM, Jayathirth D V wrote:
Hi Phil,

    1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test cases: 
4; passed: 3; failed: 1; first test
case failure: ColorModel2001

    This test fails because getComponentSize() returned an array with length 3 
but it expects the length to be 4. In
the test case they have bits per component array     of length 4 like {8, 8, 8, 
8}. But in the test case wherever
they are passing "has Alpha" as "false" we omit the alpha component bit. This 
is because of tighter check     that we
have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . 
"numComponents" will be 3 if hasAlpha is
false.

This is a bug in the test then, especially if the size of our array matches the 
return value of getNumComponents.

    2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases: 3; 
passed: 2; failed: 1; first test case
failure: ColorModel0004

    Here they check for equality between 2 ColorModel objects having same 
values, but it fails because now we are
using identity-as-equals check in ColorModel.

How do they accomplish this when the CM class is abstract?  Do they create a 
relatively empty subclass and instantiate
that?

The documentation for the equals() method does not document the conditions 
under which it returns true, it uses a
vague concept of "equals this ColorModel".  I don't see how they could test 
anything given that documentation.

    3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test cases: 2; 
passed: 1; failed: 1; first test case
failure: ColorModel2006

    Here they check for hashCode equality between 2 ColorModel objects having 
same values, but it fails since we
don't have hashCode check in ColorModel and it     will be different between 2 
Objects.

Same as above, there are no promises documented.

    
4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1: 
Failed. test cases: 2; passed: 1;
failed: 1; first test case failure: testCase1

    Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also 
happening because of same reason as why the
first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK 
test tries to call getComponentSize()
with index 3 which throws ArrayIndexOutOfBoundsException.

Same assessment as #1 above...

Again, while these are my recommendations about the correctness of these tests, 
the question remains whether we want
to introduce a change at this point in the release cycle that will essentially 
invalidate a number of tests that have
been working for several releases already.  I'll leave that tactic issue to 
Phil...

                ...jim