[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or
From:       Jim Graham <james.graham () oracle ! com>
Date:       2017-01-31 22:34:03
Message-ID: 81fdf4d1-3d0e-a780-0e25-743d222ed5fc () oracle ! com
[Download RAW message or body]

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
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]