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

[Phil Race]

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
>