Community
Participate
Working Groups
Build Identifier: A small contribution to test classes Reproducible: Always
Created attachment 176176 [details] Proposed test class with six test cases Improvable - i got an build error executing ant goal "ant clover" when i changed the expected exceptions to NullPointerExceptions instead of RuntimeException, while i had no issue into Eclipse ...
Will be reviewed shortly.
(In reply to comment #1) 'clover' is not a target that can be run without a clean test run before it. Have you tried 'ant clean clean-integration test clover' for example, to ensure that everything starts cleanly. There is also 'clean-ivy' that will clear out the ivy-cache before the build, and as a last resort 'git clean -fdx' (at the root of the git repository) will remove everything not tracked in the git repo itself. 'ant test clover' will then build the jars, the tests, run the tests and then run them with clover (if you have clover installed and a licence to run it).
Review results: 1) renamed class to ConfigurationAdminConfigurationInfoTests (notice plural -- it is a convention, that is all. 2) renamed RANDOM_PID to CONFIG_INFO_TEST_PID. 3) new ConfigurationAdminConfigurationInfo objects are placed in ConfigurationInfo local variables, not ConfigurationAdminConfigurationInfo variables, because we should be testing the behaviour at the interface level. (And none of the tests exploit implementation-specific access.) 4) renamed local variables of 4) to configurationInfo (convention, again -- also Eclipse will generate it for you under the right circumstances). Please take the revised version (attached by me) and modify it to answer the following issue: The use of mockAdmin is not always realistic. In the case of testGetPropertiesWithEmptyHashtable() (not the right name, I think) getConfiguration() doesn't ever return an empty configuration (see the Felix implementation and the StubConfigurationAdmin class in org.eclipse.virgo.kernel). In the case of testFailingGetProperties() you correctly simulate an IOException. This is OK. In the case of testFailingPropertiesWithNullConfiguration() you simulate a ConfigurationAdmin that returns null on getConfiguration() but this is more a test of ConfigurationAdmin and not ConfigurationInfo -- therefore I don't think it ought to be tested (here). Non-null ConfigurationAdmin implementations are documented not ever to return null -- when the pid is not known they return an empty configuration. My recommendation is that you abandon the second exception-expecting test -- I don't think it tests anything worthwhile -- and that you use the StubConfigurationAdmin class in testGetPropertiesWithEmptyHastable() and the assertion should still succeed. Also, we should get to the bottom of the NullPointerException problem you saw. If the expected= clause doesn't work, then trap the exception yourself in the unit test method. If you re-submit it I can flag it as IP. Thanks for the work.
Created attachment 176458 [details] Revised contribution. Here is my slightly altered version (notice the new name).
Please ignore my first comments about testGetPropertiesWithEmptyHastable() -- I confused the two methods.
Thank you for your review Steve. I agree with you on the testFailingGetPropertiesWithNullConfiguration test case, i will remove it. I will re-submit a new corrected version, and on the matter of my NullPointerException handling i don't know exactly, i think it's only a problem on my developping plateform. Could you try it, by changing the expected flag to NullPointerException.class instead of RuntimeExceptions.
Olivier, I have tried replacing expected RuntimeException with expected NullPointerException and get the same symptoms as you reported (I think): Testcase: testFailingGetPropertiesWithNullConfigurationAdmin(org.eclipse.virgo.kernel.config.internal.ConfigurationAdminConfigurationInfoTests): Caused an ERROR Unexpected exception, expected<java.lang.NullPointerException> but was<java.lang.RuntimeException> java.lang.Exception: Unexpected exception, expected<java.lang.NullPointerException> but was<java.lang.RuntimeException> Caused by: java.lang.RuntimeException: java.lang.NullPointerException: null at org.eclipse.virgo.kernel.config.internal.ConfigurationAdminConfigurationInfo.getProperties(ConfigurationAdminConfigurationInfo.java:46) at org.eclipse.virgo.kernel.config.internal.ConfigurationAdminConfigurationInfoTests.testFailingGetPropertiesWithNullConfigurationAdmin(ConfigurationAdminConfigurationInfoTests.java:69) which implies that the standard JUnitTestRunner (called by Ant) is, for some reason, wrapping the NullPointerException in a RuntimeException. I'm not entirely sure why this should be -- if it isn't caused by our aspects being woven in (which is entirely possible) -- it could be a bug in the standard JUnit Test Runner. Eclipse runs with its own JUnit Runner, so won't necessarily see this. What to do -- I think that in this case, testing for a NullPointerException is not really nice. We don't want our code ever to do this deliberately (we treat a NPE as a bug, in whatever context it arises). So the solution is to find out how this might legitimately arise in our non-test code and fix the bug! That is, ensure that it doesn't happen. Over to you -- you're in this area -- what do you suggest we should change in our base code so that NPEs never escape? Hint: A small patch to ConfigurationAdminConfigurationInfo might be appropriate.
Created attachment 176497 [details] Updated class after review Here is an updated version : - without the second test case ; - and using StubConfigurationAdminInfo I'll study later about ConfigurationAdminInfo's ways of improvement. Regards
Created attachment 176548 [details] Final amended tests As a result of this submission I have updated the ConfigurationStub which was not working according to the spec, and that meant that more realistic tests could be performed here. The null ConfigurationAdmin is prohibited by a @NonNull annotation which I have added to the first parameter of the ConfigurationAdminConfigurationInfo constructor. This is now tested (by expected=FatalAssertionException) instead of expecting a null getProperties() (which is disallowed behaviour in any case). I have added a separate test for getProperties() with an empty configuration. Hope you like the changes. As soon as you make the necessary assertions I can put them up.
These changes are ok for me. And i can assure you that this code is 100% by me (and you) and that i have the right to contribute this code to Virgo. thank you Steve.
Comment on attachment 176497 [details] Updated class after review SHA: 6aed63fd535198d320a2be88e63e6388258e5210 Author: Olivier Girardot <ssaboum@gmail.com> Date: Fri Aug 13 2010 13:45:45 GMT+0100 (BST) Committer: Steve Powell <spowell@vmware.com> Commit Date: Fri Aug 13 2010 13:56:34 GMT+0100 (BST) Subject: Modified ConfigurationAdminConfigurationInfo to cope with null Dictionary; Added @NonNull to ConfgurationAdmin argument on C…A…C…I… constructor; Modified StubConfiguration to return null initially; Added tests for ConfigurationAdminConfigurationInfo. Makes it into the kernel. Well done.
Waiting for all builds to go through clean before close.
All clean.
