On the other hand, it makes sense that similar construction would be equals() for objects intended to be compared and the CM subclasses have always tried to be comparable. But, since CM is abstract you can't really test its constructors directly. With these fixes we should end up having all subclasses compare as equals() for similar construction so we are still making good on the implied promise that they are testing here for all constructable objects.
Should we consider that CM might promise proper equals() tests for subclasses? For one, we can't really fully promise that since subclasses may differ for reasons other than what CM can test, so its implementation of equals() can't be authoritative in general. It is true that we used to provide a conditionally appropriate test for just the parts that CM was responsible for, but we never documented that. In practice that potential promise doesn't matter for our internal classes, it only matters for external classes that aren't being updated here.
With regards to Phil's question about whether these changes will affect application code - since you can't instantiate CM directly, and since we deal with these conditions properly in all sub-classes this boils down to whether applications use their own subclass of CM, and I don't have a lot of data for that. Phil?
...jim
On 1/30/17 10:37 PM, Jayathirth D V wrote:
Hi, Regarding Jim's query of #2 JCK test case: How do they accomplish this when the CM class is abstract? Do they create a relatively empty subclass and instantiate that? Yes they have simple subclass(MyColorModel) of ColorModel and they instantiate that with same "bits" and try to compare. MyColorModel cm = new MyColorModel(4); ColorModel x = new MyColorModel(4) if (!cm.equals(x)) { return Status.failed("Should return true on two instances instantiated by the same constructor"); } Thanks, Jay -----Original Message----- From: Jayathirth D V Sent: Tuesday, January 31, 2017 11:53 AM To: Philip Race; Jim Graham; 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods Hi Phil & Jim, Thanks for your inputs. I have verified with previous iteration of the changes and both #1 and #4 fail from the time we have introduced the change in how we calculate "nBits" in ColorModel. - nBits = bits.clone(); + /* + * We need significant bits value only for the length + * of number of components, so we truncate remaining part. + * It also helps in hashCode calculation since bits[] can contain + * different values after the length of number of components between + * two ColorModels. + */ + nBits = Arrays.copyOf(bits, numComponents); #2 and #3 would fail from the time we have removed equals() and hashCode() methods from ColorModel, to follow identity-as-equals verification. Collectively all of them are failing right now as we have both the changes at http://cr.openjdk.java.net/~jdv/7107905/webrev.15/ . Thanks, Jay -----Original Message----- From: Phil Race Sent: Tuesday, January 31, 2017 5:27 AM To: Jim Graham; Jayathirth D V; 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods 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
