Hi Willem, Thanks for the heads-up & your quick fixes, though I've got two questions:
- Do you see any specific reason why we should NOT change those two assertTrue() statements as suggested, so that instead of getting a stack trace like: https://builds.apache.org/job/Camel.trunk.fulltest/org.apache.camel$camel-core/1614/testReport/junit/org.apache.camel.util.jsse/SSLContextParametersTest/testCipherSuitesFilter/ We would have: junit.framework.AssertionFailedError: 'ABC' does not start with the prefix: 'XYZ'! at junit.framework.Assert.fail(Assert.java:55) at junit.framework.Assert.assertTrue(Assert.java:22) at junit.framework.Assert.assertTrue(Assert.java:31) at junit.framework.TestCase.assertTrue(TestCase.java:201) at org.apache.camel.util.jsse.SSLContextParametersTest.assertStartsWith(SSLContextParametersTest.java:757) at org.apache.camel.util.jsse.SSLContextParametersTest.testCipherSuitesFilter(SSLContextParametersTest.java:551) IMHO this should be considered as a general best practice so that IF an assert fails we get an idea about WHY it failed. - You have not added the missing getters for the fields being NEWLY introduced through CAMEL-6971. Is there any specific CXF convention or rule for doing so? To my understanding in general for every setter there should be a getter. Babak Willem.Jiang wrote > Hi Babak, > > Thanks for the review, I just updated the code with some suggestion of > you. > For the SSLContextParametersTest it is caused by the different JDKs > handles the SSL related setting differently, I just updated the code to > avoid the null collection returned. > > > -- > Willem Jiang > > Red Hat, Inc. > Web: http://www.redhat.com > Blog: http://willemjiang.blogspot.com (http://willemjiang.blogspot.com/) > (English) > http://jnn.iteye.com (http://jnn.javaeye.com/) (Chinese) > Twitter: willemjiang > Weibo: 姜宁willem > > > > > > On Tuesday, November 19, 2013 at 3:59 AM, Babak Vahdat wrote: > >> Hi >> >> Just spotted couple of things on the commit mailing list of today: >> >> http://git-wip-us.apache.org/repos/asf/camel/diff/000581e6 >> >> This commit now makes the following asserts passing: >> >> assertStartsWith((String[]) null, "foo"); >> assertStartsWith(new String[] {}, "foo"); >> assertStartsWith((Collection > <String> > ) null, "foo"); >> assertStartsWith(new ArrayList > <String> > (), "foo"); >> >> Which is actually wrong as that would be wrong to claim that e.g. the >> String "foo" is an element of a given empty collection. Other than that >> this change does not actually fix the failing test as the problem is >> something else, namely there're elements inside the given >> array/collection >> which do NOT start with the given prefix. So maybe a better approach >> would >> be to change the asserts from: >> >> assertTrue(value.startsWith(prefix)); >> >> to: >> >> assertTrue("'" + value + "' does not start with the prefix: '" + prefix >> + "'!", value.startsWith(prefix)); >> >> So we get an idea about what actually is going wrong on ubuntu. See also >> the note inside the checkProtocols() method of this test class. >> >> http://git-wip-us.apache.org/repos/asf/camel/diff/8a080c97 >> >> - The following getters are missing: getProvider() / getSchemaLocation() >> / >> getSchemaLocations() for the properties being newly introduced. >> - Instead of: >> >> @SuppressWarnings("rawtypes") >> JSONProvider jsonProvider = new JSONProvider(); >> >> We could better do: >> >> JSONProvider > jsonProvider = new JSONProvider > (); >> >> - There's no benefit of the following generic List type newly introduced >> by the API: >> >> public void setProviders(List<? extends Object> providers)... >> >> As <?> is as good as <? extends Object> so maybe better do: >> >> public void setProviders(List<?> providers)... >> >> See http://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.4 >> >> ...Every type variable declared as a type parameter has a bound. If no >> bound is declared for a type variable, Object is assumed. >> >> Babak -- View this message in context: http://camel.465427.n5.nabble.com/About-2-commits-of-today-tp5743462p5743486.html Sent from the Camel Development mailing list archive at Nabble.com.
