You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@camel.apache.org by Babak Vahdat <ba...@swissonline.ch> on 2013/11/18 20:59:49 UTC
About 2 commits of today...
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<Object> jsonProvider = new JSONProvider<Object>();
- 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
Re: About 2 commits of today...
Posted by Hadrian Zbarcea <hz...@gmail.com>.
For what is worth, the test was failing with all JDKs not just OpenJDK.
Not sure if hardcoding the cipher suite is the best fix in the long run,
but it does work.
Hadrian
On 11/18/2013 09:56 PM, 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<Object> jsonProvider = new JSONProvider<Object>();
>>
>> - 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
>
>
Re: About 2 commits of today...
Posted by Willem jiang <wi...@gmail.com>.
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).
Re: About 2 commits of today...
Posted by Babak Vahdat <ba...@swissonline.ch>.
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.
Re: About 2 commits of today...
Posted by Willem jiang <wi...@gmail.com>.
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<Object> jsonProvider = new JSONProvider<Object>();
>
> - 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
Re: About 2 commits of today...
Posted by Hadrian Zbarcea <hz...@gmail.com>.
Good catch Babak. Thanks,
Hadrian
On 11/18/2013 02:59 PM, 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<Object> jsonProvider = new JSONProvider<Object>();
>
> - 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
>
>