Community
Participate
Working Groups
3.6M1 The fix for bug#276741 has introduced several warnings in the jdt/core test plugin projects. These warnings were suppressed by changing the compiler warnings preferences on a project specific basis. We should actually fix these warnings rather than suppress them since (a) suppression doesn't work for integration builds as releng does not use the project specific preferences and so these warnings show up there. (b) We also don't want to suppress this diagnostic altogther since this could hide genuine defects. Please remember to revert the project specific setting to unsuppress the warning.
Created attachment 145752 [details] Proposed Patch Patch includes the fixed tests in ASTTest and ASTParserTest. Also the change to the JDT/Core project specific setting has been reverted.
Jay, I would introduce assertSame(...) which uses identity check (==). The JUnit assertEquals(...) methods for primitives don't do this, e.g. for long they first create Long objects and compare them with equals(...). That's not the intention of the original tests.
Dani, With reference to bug 276741, comment 6, my understanding was that these tests just verify that the constants haven't changed by comparing them with their original literal value. Besides, even though the original bug was about identity check, for these tests, a simple assertEquals might do the job as well. Let me know if I miss something here. Fred, you have any comments?
>a simple assertEquals might do the job Yes, but it is not the right thing to do and it only "works" because the constant types are primitive types. If the constants were objects then it would be completely wrong. Besides that it creates unnecessary objects during the test.
(In reply to comment #4) > >a simple assertEquals might do the job > Yes, but it is not the right thing to do and it only "works" because the > constant types are primitive types. Well, my understanding was that the suggested assertSame method takes two 'int' (primitives) as parameters, which means that we are talking about the case that is very specific to two 'int' comparison. > If the constants were objects then it would be completely wrong. I agree. However, for this, we would have to use the method junit.framework.Assert.assertSame(Object, Object), which can't be used for these tests anyhow. As for the affected 101 tests, they all are free from warnings and are passing.
I think Dani's point is valid regarding the needless creation of objects. That being said, the proposed patch serves the intended purpose and while not being perfect is fully functional and IMO we could accept it as such. I don't see neither the attendant performance issues with object creations nor the solution's inapplicability to non-primitive types as being material enough to rework the patch. So Dani, unless you feel strongly about it, I would be inclined to release the patch as it is. OTOH, if you think we must rework the patch, we will do so.
If we know a better solution then we should use it, especially in this case where doing it right is not a big deal (less work than the time we already spent with the discussion). Keep in mind that people copy our code as an example and hence it should be state of the art. Please rework the patch.
(In reply to comment #7) > If we know a better solution then we should use it Yes, the important question is : is the alternate a "better enough" solution to warrant rework ? Any patch anybody prepares can be improved in some manner, form, fashion to some degree however infinitesimal if sufficient time is spent on it - Does that justify rework ? > Keep in mind that people copy our code as an > example and hence it should be state of the art. In this case, they will copy perfectly reasonable, fully functional code. > Please rework the patch. Yes. Since it appears that seems to offer the only way forward ...
Here's my two cents on this... I agree with Jay that the only purpose of these tests are to verify that no API constant value has been changed. So, when constants are primitives, an assertEquals is enough to do the job. It's also true that the assertSame(Object, Object) cannot be used for those tests. I also agree with Dani that it may be dangerous to use the assertEquals because one can think that it should be the method to use whatever the constants is. And this is obviously not the case for non-primitive ones. It's also true that the assertEquals creates two objects which is not necessary to verify the primitive constants value... So, IMO, I would suggest an approach reconciling these two points of view: implements a specific method assertSame(String, int, int) in the JDT/Core TestCase class and use it in the corresponding tests...
(In reply to comment #9) > Here's my two cents on this... > I agree with Jay that the only purpose of these tests are to verify that no API > constant value has been changed. So, when constants are primitives, an > assertEquals is enough to do the job. It's also true that the > assertSame(Object, Object) cannot be used for those tests. > I also agree with Dani that it may be dangerous to use the assertEquals because > one can think that it should be the method to use whatever the constants is. > And this is obviously not the case for non-primitive ones. It's also true that > the assertEquals creates two objects which is not necessary to verify the > primitive constants value... > So, IMO, I would suggest an approach reconciling these two points of view: > implements a specific method assertSame(String, int, int) in the JDT/Core > TestCase class and use it in the corresponding tests... What is the intended usage of junit.framework.Assert.assertEquals(int, int) junit.framework.Assert.assertEquals(String, int, int) junit.framework.Assert.assertEquals(short, short) junit.framework.Assert.assertEquals(String, short, short) junit.framework.Assert.assertEquals(long, long) junit.framework.Assert.assertEquals(String long, long) etc ... ? It seems every usage of these methods is dangerous on this count. There are many many many such usages of these methods in JDT/core tests outside of the current changes - Do we want to rework them too ?
(In reply to comment #10) > (In reply to comment #9) > There are many many many such usages of these methods in JDT/core tests > outside of the current changes - Do we want to rework them too ? There are 4,497 references to junit.framework.Assert.assertEquals(String, int, int) from JDT/Core tests. All of these create two Integer's as this method call decays to static public void assertEquals(String message, int expected, int actual) { assertEquals(message, new Integer(expected), new Integer(actual)); } FYI. I didn't bother running search references to other similar methods.
>It seems every usage of these methods is dangerous on this count. No they aren't. Normally when using JUnit (and also in normal code) you want to test object equality (and in this case primitives) using 'equals'. However, when you want to test a constant you should test whether the constant values are the same (identity check). Therefore JUnit offers all the 'assertEquals' methods but also offers an 'assertSame'. Note that the reason for the complicated JUnit assertEquals(int, int) implementation is to cover (un-) boxing. This means that you could get a green test even if the values don't have the same type. >There are 4,497 references to junit.framework.Assert.assertEquals(String, int, >int) from JDT/Core tests. I would assume that the intention of most of them is indeed to test equality and not identity and hence it's the right thing to do here even if it means additional object creation.
(In reply to comment #11) > (In reply to comment #10) I know there are 'some' references to assertEquals(String, int, int) in JDT/Core tests! Save 2 objects creation was not the original goal of the patch, it was comparing the int and instead of comparing Integer as assertEquals(String, inti, int) does... So, that was why I suggested the implementation of the specific method assertSame(String, int, int). Then, and only after having addressed the equality comparison initial issue, I also added that this specific implementation would have the 'small' advantage to save 2 objects (Integer) creation... These 2 cents were definitely not a suggestion to rewrite all the existing calls to assertEquals(String, int, int)!
(In reply to comment #12) [...] > >There are 4,497 references to junit.framework.Assert.assertEquals(String, int, > >int) from JDT/Core tests. > I would assume that the intention of most of them is indeed to test equality > and not identity and hence it's the right thing to do here even if it means > additional object creation. You lost me there - Why is equality check of two primitive ints done via additional object creation the right thing to do ?? It has been stated over and over that what is being tested by these tests is that certain API constants of primitive int type retain their values and don't break clients by inadvertantly undergoing change - there is little to be served by saying the approach taken will not work were there to be objects in the picture - this last bit is acknowledged. If the unnecessary object creation is not an issue, IMHO there is even less reason to redo the patch. But we will redo the patch just to break this impasse.
I think the right way to fix these warnings is to disable the check at compile time for the corresponding test projects. There is no need to spend some time on this.
. (In reply to comment #15) > I think the right way to fix these warnings is to disable the check at compile > time for the corresponding test projects. > There is no need to spend some time on this. This was suggested by Frederic in bug 276741, comment 4 and was carried out by me in the fix for bug 276741. But Dani rightly pointed out two reasons (documented in comment#0) why we don't want to do that and suggested an alternate code pattern to solve the warnings issue
>I think the right way to fix these warnings is to disable the check I disagree for the reasons given in comment 0 and because the current the current code is wrong: once the '==' check is inlined it no longer detects a change to the constant unless the test code is recompiled. Now, this happens automatically in your workspace but not in the official build unless you recontribute the test project (if version is the same then the bits are fetched from the p2 repo).
ok, the compile time inlining is a good argument. The purpose of these tests is to check that the value of the constant is unchanged. All these constants are int values. So I don't really see the point of all this discussion. The current patch is good enough for the purpose of the check for the value of the constant. It is a test project.
Created attachment 146103 [details] Proposed fix Jay, please have a look at the patch. I plan to release it for next N-build so that we get rid of the compiler warnings.
Released for 3.6M2. Code check is required for the verification.
Verified for 3.6M2 using I20090915-0100.
