Hi Babak, Yeah, we need to add the String parameter to let us know better about what’s wrong with the assertTrue().
For the new added operations in CxfRsEndpoint, I was think the user won’t access it from out side of CxfRsEndpoint, but we could add the getMethod for it. I will commit a quick fix for these two issues. -- 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:23 PM, Babak Vahdat wrote: > 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 > (http://Nabble.com).
