You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Mikhail Loenko <ml...@gmail.com> on 2006/05/23 10:54:21 UTC
[classlib] please review my regression test and patch for HttpURLConnection
I've created regression test and patch for
http://issues.apache.org/jira/browse/HARMONY-482
I had to make some changes in the luni's build.xml,
that are intended to be fixed once we all agree with
proposed test suite layout.
Please review the changes.
Thanks,
Mikhail
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Tim Ellison <t....@gmail.com>.
Mikhail Loenko wrote:
> Hi Paulex,
>
> 2006/5/25, Yang Paulex <pa...@gmail.com>:
>> Mikhail,
>>
>> Sorry to response so late, seems both IBM's smtp server and gmail is not
>> stable so that I have to send this mail several times from today before
>> yesterday (please pardon me if the mailing list get several duplicate
>> mails
>> after hundreds of hours! ).
>>
>> Basically, I'm afraid I cannot agree with you on this patch.
>>
>> 1. I have no strong objection on the patch for HttpURLConnection,
>> but...If I
>> understand correctly, Harmony-482 is about the internal contract between
>> java.net.HttpURLConnection.setRequestMethod() and
>> o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream(),
>>
>
> Not exactly. The second problem is that the code makes assumption that
> two String constant-pool constants in two different classes will be the
> same
> object. That is an assumption about VM that is met in J9 and RI.
>
> Other VMs might not have this kind of optimization and the code will not
> work
> on those VMs. I've fixed the code to not depend on that assumption.
The JVM spec requires that String literals are interned using
String.intern, so you are safe to make that assumption.
> Then I had to develop a test that would
> 1. passes on RI
> 2. fails on unfixed Harmony
> 3. passes on fixed Harmony
>
> The code you suggested below passes on unfixed Harmony, so
> it is not a good regression test.
>
>> and the internal contract may be broken only if some of the codes
>> refactored
>> (no chance for user application), say, modification for the
>> setRequestMethod() like this:
>>
>> - this.method = methodTokens[i];
>> + this.method = method;
>>
>> But this can be easily monitored by test codes below:
>>
>> public void testGetOutputStream() throws Exception {
>> HttpURLConnection c = (HttpURLConnection) new
>> URL("http://127.0.0.1:"<http://127.0.0.1/>
>> + port).openConnection();
>> c.setDoOutput(true);
>> c.setRequestMethod(new String("GET"));
>> c.getOutputStream();
>>
>> ...//other for POST/PUT
>> }
>>
>> This test is enough to enforced the internal contract, it will shout
>> if some
>> refactory breaks the internal contract. So IMHO, I think the patch for
>> HttpURLConnection is a little over-designed.
>>
>> 2. Even the patch is necessary, I think the introduction of injected
>> java.net.HttpURLConnectionAccessor is debatable. I suggest to avoid the
>> injected helper class as long as possible, because it introduces
>> complexity
>> to understand/manage and is sometimes fragile because it may highly
>> coupled
>> with implementation details. For this specific case, a mocked subclass of
>> o.a.h....HttpURLConnection with overridden getRequestMethod can be used
>> instead.
>
> I partially agree. If you suggest a test that would be easier to
> understand,
> I'll be happy to replace the current one.
IMHO subclassing like this in tests is fine, and where we can't (i.e. to
get access to package private elements) then injecting helpers into the
bootclasspath is a good practice.
Regards,
Tim
>> 2006/5/23, Mikhail Loenko <ml...@gmail.com>:
>> >
>> > I've created regression test and patch for
>> > http://issues.apache.org/jira/browse/HARMONY-482
>> >
>> > I had to make some changes in the luni's build.xml,
>> > that are intended to be fixed once we all agree with
>> > proposed test suite layout.
>> >
>> > Please review the changes.
>> >
>> > Thanks,
>> > Mikhail
>> >
>> > ---------------------------------------------------------------------
>> > Terms of use : http://incubator.apache.org/harmony/mailing.html
>> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
>> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>> >
>> >
>>
>>
>> --
>> Paulex Yang
>> China Software Development Labotary
>> IBM
>>
>>
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
--
Tim Ellison (t.p.ellison@gmail.com)
IBM Java technology centre, UK.
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Mikhail Loenko <ml...@gmail.com>.
2006/5/25, Yang Paulex <pa...@gmail.com>:
> 2006/5/25, Yang Paulex <pa...@gmail.com>:
> >
> > Hi, Mikhail,
> >
> > 2006/5/25, Mikhail Loenko <ml...@gmail.com>:
> >
> > > Hi Paulex,
> > >
> > > 2006/5/25, Yang Paulex <pa...@gmail.com>:
> > > > Mikhail,
> > > >
> > > > Sorry to response so late, seems both IBM's smtp server and gmail is
> > > not
> > > > stable so that I have to send this mail several times from today
> > > before
> > > > yesterday (please pardon me if the mailing list get several duplicate
> > > mails
> > > > after hundreds of hours! ).
> > > >
> > > > Basically, I'm afraid I cannot agree with you on this patch.
> > > >
> > > > 1. I have no strong objection on the patch for HttpURLConnection,
> > > but...If I
> > > > understand correctly, Harmony-482 is about the internal contract
> > > between
> > > > java.net.HttpURLConnection.setRequestMethod () and
> > > >
> > > o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream
> > > (),
> > >
> > > Not exactly. The second problem is that the code makes assumption that
> > > two String constant-pool constants in two different classes will be the
> > > same
> > > object. That is an assumption about VM that is met in J9 and RI.
> > >
> > > Other VMs might not have this kind of optimization and the code will not
> > > work
> > > on those VMs. I've fixed the code to not depend on that assumption.
> >
> >
> > I accepts this reason, it will be better if the original codes use
> > predefined constants instead of hard coded string.
> >
>
> I'm Sorry, I made a mistake here, the String's spec and Java Language Spec
> has required the literal String as well as constant with String value must
> be interned, please refer to String.intern()'s JavaDoc and JLS 3.10.5. So
> the original code can work with any Java VM/Compiler.
Good catch! I'll simplify the test
Thanks,
Mikhail
>
>
> Then I had to develop a test that would
> > > 1. passes on RI
> > > 2. fails on unfixed Harmony
> > > 3. passes on fixed Harmony
> > >
> > > The code you suggested below passes on unfixed Harmony, so
> > > it is not a good regression test.
> >
> >
> > The codes below is not regressiont test, it's just a test to enforce the
> > original internal contract between setRequestMethod() and
> > getOutputStream().
> >
> > > and the internal contract may be broken only if some of the codes
> > > refactored
> > > > (no chance for user application), say, modification for the
> > > > setRequestMethod() like this:
> > > >
> > > > - this.method = methodTokens[i];
> > > > + this.method = method;
> > > >
> > > > But this can be easily monitored by test codes below:
> > > >
> > > > public void testGetOutputStream() throws Exception {
> > > > HttpURLConnection c = (HttpURLConnection) new
> > > > URL("http://127.0.0.1:"<http://127.0.0.1/>
> > > > + port).openConnection();
> > > > c.setDoOutput(true);
> > > > c.setRequestMethod(new String("GET"));
> > > > c.getOutputStream();
> > > >
> > > > ...//other for POST/PUT
> > > > }
> > > >
> > > > This test is enough to enforced the internal contract, it will shout
> > > if some
> > > > refactory breaks the internal contract. So IMHO, I think the patch for
> > > > HttpURLConnection is a little over-designed.
> > > >
> > > > 2. Even the patch is necessary, I think the introduction of injected
> > > > java.net.HttpURLConnectionAccessor is debatable. I suggest to avoid
> > > the
> > > > injected helper class as long as possible, because it introduces
> > > complexity
> > > > to understand/manage and is sometimes fragile because it may highly
> > > coupled
> > > > with implementation details. For this specific case, a mocked subclass
> > > of
> > > > o.a.h....HttpURLConnection with overridden getRequestMethod can be
> > > used
> > > > instead.
> > >
> > > I partially agree. If you suggest a test that would be easier to
> > > understand,
> > > I'll be happy to replace the current one.
> >
> >
> > How about this one below?
> >
> >
> > public void testGetOutputStream() throws Exception {
> > // Regression for HARMONY-482
> > HttpURLConnection c = new MockHttpURLConnection(new URL("
> > http://127.0.0.1"), port);
> > c.setDoOutput(true);
> >
> > c.setRequestMethod(new String("POST"));
> > c.getOutputStream();
> >
> >
> > c.setRequestMethod(new String("GET"));
> > c.getOutputStream();
> >
> > c.setRequestMethod(new String("PUT"));
> > c.getOutputStream();
> > }
> >
> > private static class MockHttpURLConnection extends
> > org.apache.harmony.luni.internal.net.www.protocol.http.HttpURLConnection{
> > protected MockHttpURLConnection(URL url, int port) {
> > super(url, port);
> > }
> > //override this method to set method directly without validation
> > public void setRequestMethod(String method){
> > this.method = method;
> > }
> > }
> >
> > Thanks,
> > > Mikhail
> > >
> > > >
> > > > Your comments?
> > > >
> > > >
> > > > 2006/5/23, Mikhail Loenko <ml...@gmail.com>:
> > > > >
> > > > > I've created regression test and patch for
> > > > > http://issues.apache.org/jira/browse/HARMONY-482
> > > > >
> > > > > I had to make some changes in the luni's build.xml,
> > > > > that are intended to be fixed once we all agree with
> > > > > proposed test suite layout.
> > > > >
> > > > > Please review the changes.
> > > > >
> > > > > Thanks,
> > > > > Mikhail
> > > > >
> > > > >
> > > ---------------------------------------------------------------------
> > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > > > For additional commands, e-mail:
> > > harmony-dev-help@incubator.apache.org
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Paulex Yang
> > > > China Software Development Labotary
> > > > IBM
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > >
> > >
> >
> >
> > --
> >
> > Paulex Yang
> > China Software Development Labotary
> > IBM
> >
>
>
>
> --
> Paulex Yang
> China Software Development Labotary
> IBM
>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Yang Paulex <pa...@gmail.com>.
2006/5/25, Yang Paulex <pa...@gmail.com>:
>
> Hi, Mikhail,
>
> 2006/5/25, Mikhail Loenko <ml...@gmail.com>:
>
> > Hi Paulex,
> >
> > 2006/5/25, Yang Paulex <pa...@gmail.com>:
> > > Mikhail,
> > >
> > > Sorry to response so late, seems both IBM's smtp server and gmail is
> > not
> > > stable so that I have to send this mail several times from today
> > before
> > > yesterday (please pardon me if the mailing list get several duplicate
> > mails
> > > after hundreds of hours! ).
> > >
> > > Basically, I'm afraid I cannot agree with you on this patch.
> > >
> > > 1. I have no strong objection on the patch for HttpURLConnection,
> > but...If I
> > > understand correctly, Harmony-482 is about the internal contract
> > between
> > > java.net.HttpURLConnection.setRequestMethod () and
> > >
> > o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream
> > (),
> >
> > Not exactly. The second problem is that the code makes assumption that
> > two String constant-pool constants in two different classes will be the
> > same
> > object. That is an assumption about VM that is met in J9 and RI.
> >
> > Other VMs might not have this kind of optimization and the code will not
> > work
> > on those VMs. I've fixed the code to not depend on that assumption.
>
>
> I accepts this reason, it will be better if the original codes use
> predefined constants instead of hard coded string.
>
I'm Sorry, I made a mistake here, the String's spec and Java Language Spec
has required the literal String as well as constant with String value must
be interned, please refer to String.intern()'s JavaDoc and JLS 3.10.5. So
the original code can work with any Java VM/Compiler.
Then I had to develop a test that would
> > 1. passes on RI
> > 2. fails on unfixed Harmony
> > 3. passes on fixed Harmony
> >
> > The code you suggested below passes on unfixed Harmony, so
> > it is not a good regression test.
>
>
> The codes below is not regressiont test, it's just a test to enforce the
> original internal contract between setRequestMethod() and
> getOutputStream().
>
> > and the internal contract may be broken only if some of the codes
> > refactored
> > > (no chance for user application), say, modification for the
> > > setRequestMethod() like this:
> > >
> > > - this.method = methodTokens[i];
> > > + this.method = method;
> > >
> > > But this can be easily monitored by test codes below:
> > >
> > > public void testGetOutputStream() throws Exception {
> > > HttpURLConnection c = (HttpURLConnection) new
> > > URL("http://127.0.0.1:"<http://127.0.0.1/>
> > > + port).openConnection();
> > > c.setDoOutput(true);
> > > c.setRequestMethod(new String("GET"));
> > > c.getOutputStream();
> > >
> > > ...//other for POST/PUT
> > > }
> > >
> > > This test is enough to enforced the internal contract, it will shout
> > if some
> > > refactory breaks the internal contract. So IMHO, I think the patch for
> > > HttpURLConnection is a little over-designed.
> > >
> > > 2. Even the patch is necessary, I think the introduction of injected
> > > java.net.HttpURLConnectionAccessor is debatable. I suggest to avoid
> > the
> > > injected helper class as long as possible, because it introduces
> > complexity
> > > to understand/manage and is sometimes fragile because it may highly
> > coupled
> > > with implementation details. For this specific case, a mocked subclass
> > of
> > > o.a.h....HttpURLConnection with overridden getRequestMethod can be
> > used
> > > instead.
> >
> > I partially agree. If you suggest a test that would be easier to
> > understand,
> > I'll be happy to replace the current one.
>
>
> How about this one below?
>
>
> public void testGetOutputStream() throws Exception {
> // Regression for HARMONY-482
> HttpURLConnection c = new MockHttpURLConnection(new URL("
> http://127.0.0.1"), port);
> c.setDoOutput(true);
>
> c.setRequestMethod(new String("POST"));
> c.getOutputStream();
>
>
> c.setRequestMethod(new String("GET"));
> c.getOutputStream();
>
> c.setRequestMethod(new String("PUT"));
> c.getOutputStream();
> }
>
> private static class MockHttpURLConnection extends
> org.apache.harmony.luni.internal.net.www.protocol.http.HttpURLConnection{
> protected MockHttpURLConnection(URL url, int port) {
> super(url, port);
> }
> //override this method to set method directly without validation
> public void setRequestMethod(String method){
> this.method = method;
> }
> }
>
> Thanks,
> > Mikhail
> >
> > >
> > > Your comments?
> > >
> > >
> > > 2006/5/23, Mikhail Loenko <ml...@gmail.com>:
> > > >
> > > > I've created regression test and patch for
> > > > http://issues.apache.org/jira/browse/HARMONY-482
> > > >
> > > > I had to make some changes in the luni's build.xml,
> > > > that are intended to be fixed once we all agree with
> > > > proposed test suite layout.
> > > >
> > > > Please review the changes.
> > > >
> > > > Thanks,
> > > > Mikhail
> > > >
> > > >
> > ---------------------------------------------------------------------
> > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > > For additional commands, e-mail:
> > harmony-dev-help@incubator.apache.org
> > > >
> > > >
> > >
> > >
> > > --
> > > Paulex Yang
> > > China Software Development Labotary
> > > IBM
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
>
>
> --
>
> Paulex Yang
> China Software Development Labotary
> IBM
>
--
Paulex Yang
China Software Development Labotary
IBM
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Yang Paulex <pa...@gmail.com>.
Hi, Mikhail,
2006/5/25, Mikhail Loenko <ml...@gmail.com>:
>
> Hi Paulex,
>
> 2006/5/25, Yang Paulex <pa...@gmail.com>:
> > Mikhail,
> >
> > Sorry to response so late, seems both IBM's smtp server and gmail is not
> > stable so that I have to send this mail several times from today before
> > yesterday (please pardon me if the mailing list get several duplicate
> mails
> > after hundreds of hours! ).
> >
> > Basically, I'm afraid I cannot agree with you on this patch.
> >
> > 1. I have no strong objection on the patch for HttpURLConnection,
> but...If I
> > understand correctly, Harmony-482 is about the internal contract between
> > java.net.HttpURLConnection.setRequestMethod() and
> > o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream
> (),
>
> Not exactly. The second problem is that the code makes assumption that
> two String constant-pool constants in two different classes will be the
> same
> object. That is an assumption about VM that is met in J9 and RI.
>
> Other VMs might not have this kind of optimization and the code will not
> work
> on those VMs. I've fixed the code to not depend on that assumption.
I accepts this reason, it will be better if the original codes use
predefined constants instead of hard coded string.
Then I had to develop a test that would
> 1. passes on RI
> 2. fails on unfixed Harmony
> 3. passes on fixed Harmony
>
> The code you suggested below passes on unfixed Harmony, so
> it is not a good regression test.
The codes below is not regressiont test, it's just a test to enforce the
original internal contract between setRequestMethod() and
getOutputStream().
> and the internal contract may be broken only if some of the codes
> refactored
> > (no chance for user application), say, modification for the
> > setRequestMethod() like this:
> >
> > - this.method = methodTokens[i];
> > + this.method = method;
> >
> > But this can be easily monitored by test codes below:
> >
> > public void testGetOutputStream() throws Exception {
> > HttpURLConnection c = (HttpURLConnection) new
> > URL("http://127.0.0.1:"<http://127.0.0.1/>
> > + port).openConnection();
> > c.setDoOutput(true);
> > c.setRequestMethod(new String("GET"));
> > c.getOutputStream();
> >
> > ...//other for POST/PUT
> > }
> >
> > This test is enough to enforced the internal contract, it will shout if
> some
> > refactory breaks the internal contract. So IMHO, I think the patch for
> > HttpURLConnection is a little over-designed.
> >
> > 2. Even the patch is necessary, I think the introduction of injected
> > java.net.HttpURLConnectionAccessor is debatable. I suggest to avoid the
> > injected helper class as long as possible, because it introduces
> complexity
> > to understand/manage and is sometimes fragile because it may highly
> coupled
> > with implementation details. For this specific case, a mocked subclass
> of
> > o.a.h....HttpURLConnection with overridden getRequestMethod can be used
> > instead.
>
> I partially agree. If you suggest a test that would be easier to
> understand,
> I'll be happy to replace the current one.
How about this one below?
public void testGetOutputStream() throws Exception {
// Regression for HARMONY-482
HttpURLConnection c = new MockHttpURLConnection(new URL("
http://127.0.0.1"), port);
c.setDoOutput(true);
c.setRequestMethod(new String("POST"));
c.getOutputStream();
c.setRequestMethod(new String("GET"));
c.getOutputStream();
c.setRequestMethod(new String("PUT"));
c.getOutputStream();
}
private static class MockHttpURLConnection extends
org.apache.harmony.luni.internal.net.www.protocol.http.HttpURLConnection{
protected MockHttpURLConnection(URL url, int port) {
super(url, port);
}
//override this method to set method directly without validation
public void setRequestMethod(String method){
this.method = method;
}
}
Thanks,
> Mikhail
>
> >
> > Your comments?
> >
> >
> > 2006/5/23, Mikhail Loenko <ml...@gmail.com>:
> > >
> > > I've created regression test and patch for
> > > http://issues.apache.org/jira/browse/HARMONY-482
> > >
> > > I had to make some changes in the luni's build.xml,
> > > that are intended to be fixed once we all agree with
> > > proposed test suite layout.
> > >
> > > Please review the changes.
> > >
> > > Thanks,
> > > Mikhail
> > >
> > > ---------------------------------------------------------------------
> > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > >
> > >
> >
> >
> > --
> > Paulex Yang
> > China Software Development Labotary
> > IBM
> >
> >
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
--
Paulex Yang
China Software Development Labotary
IBM
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Mikhail Loenko <ml...@gmail.com>.
Hi Paulex,
2006/5/25, Yang Paulex <pa...@gmail.com>:
> Mikhail,
>
> Sorry to response so late, seems both IBM's smtp server and gmail is not
> stable so that I have to send this mail several times from today before
> yesterday (please pardon me if the mailing list get several duplicate mails
> after hundreds of hours! ).
>
> Basically, I'm afraid I cannot agree with you on this patch.
>
> 1. I have no strong objection on the patch for HttpURLConnection, but...If I
> understand correctly, Harmony-482 is about the internal contract between
> java.net.HttpURLConnection.setRequestMethod() and
> o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream(),
Not exactly. The second problem is that the code makes assumption that
two String constant-pool constants in two different classes will be the same
object. That is an assumption about VM that is met in J9 and RI.
Other VMs might not have this kind of optimization and the code will not work
on those VMs. I've fixed the code to not depend on that assumption.
Then I had to develop a test that would
1. passes on RI
2. fails on unfixed Harmony
3. passes on fixed Harmony
The code you suggested below passes on unfixed Harmony, so
it is not a good regression test.
> and the internal contract may be broken only if some of the codes refactored
> (no chance for user application), say, modification for the
> setRequestMethod() like this:
>
> - this.method = methodTokens[i];
> + this.method = method;
>
> But this can be easily monitored by test codes below:
>
> public void testGetOutputStream() throws Exception {
> HttpURLConnection c = (HttpURLConnection) new
> URL("http://127.0.0.1:"<http://127.0.0.1/>
> + port).openConnection();
> c.setDoOutput(true);
> c.setRequestMethod(new String("GET"));
> c.getOutputStream();
>
> ...//other for POST/PUT
> }
>
> This test is enough to enforced the internal contract, it will shout if some
> refactory breaks the internal contract. So IMHO, I think the patch for
> HttpURLConnection is a little over-designed.
>
> 2. Even the patch is necessary, I think the introduction of injected
> java.net.HttpURLConnectionAccessor is debatable. I suggest to avoid the
> injected helper class as long as possible, because it introduces complexity
> to understand/manage and is sometimes fragile because it may highly coupled
> with implementation details. For this specific case, a mocked subclass of
> o.a.h....HttpURLConnection with overridden getRequestMethod can be used
> instead.
I partially agree. If you suggest a test that would be easier to understand,
I'll be happy to replace the current one.
Thanks,
Mikhail
>
> Your comments?
>
>
> 2006/5/23, Mikhail Loenko <ml...@gmail.com>:
> >
> > I've created regression test and patch for
> > http://issues.apache.org/jira/browse/HARMONY-482
> >
> > I had to make some changes in the luni's build.xml,
> > that are intended to be fixed once we all agree with
> > proposed test suite layout.
> >
> > Please review the changes.
> >
> > Thanks,
> > Mikhail
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
>
>
> --
> Paulex Yang
> China Software Development Labotary
> IBM
>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Yang Paulex <pa...@gmail.com>.
Mikhail,
Sorry to response so late, seems both IBM's smtp server and gmail is not
stable so that I have to send this mail several times from today before
yesterday (please pardon me if the mailing list get several duplicate mails
after hundreds of hours! ).
Basically, I'm afraid I cannot agree with you on this patch.
1. I have no strong objection on the patch for HttpURLConnection, but...If I
understand correctly, Harmony-482 is about the internal contract between
java.net.HttpURLConnection.setRequestMethod() and
o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream(),
and the internal contract may be broken only if some of the codes refactored
(no chance for user application), say, modification for the
setRequestMethod() like this:
- this.method = methodTokens[i];
+ this.method = method;
But this can be easily monitored by test codes below:
public void testGetOutputStream() throws Exception {
HttpURLConnection c = (HttpURLConnection) new
URL("http://127.0.0.1:"<http://127.0.0.1/>
+ port).openConnection();
c.setDoOutput(true);
c.setRequestMethod(new String("GET"));
c.getOutputStream();
...//other for POST/PUT
}
This test is enough to enforced the internal contract, it will shout if some
refactory breaks the internal contract. So IMHO, I think the patch for
HttpURLConnection is a little over-designed.
2. Even the patch is necessary, I think the introduction of injected
java.net.HttpURLConnectionAccessor is debatable. I suggest to avoid the
injected helper class as long as possible, because it introduces complexity
to understand/manage and is sometimes fragile because it may highly coupled
with implementation details. For this specific case, a mocked subclass of
o.a.h....HttpURLConnection with overridden getRequestMethod can be used
instead.
Your comments?
2006/5/23, Mikhail Loenko <ml...@gmail.com>:
>
> I've created regression test and patch for
> http://issues.apache.org/jira/browse/HARMONY-482
>
> I had to make some changes in the luni's build.xml,
> that are intended to be fixed once we all agree with
> proposed test suite layout.
>
> Please review the changes.
>
> Thanks,
> Mikhail
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
--
Paulex Yang
China Software Development Labotary
IBM
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Paulex Yang <pa...@gmail.com>.
Mikhail,
I'm afraid I cannot agree with you on this patch,
If I understand correctly, Harmony-482 is about the internal contract
between java.net.HttpURLConnection.setRequestMethod() and
o.a.h.l.internal.net.www.protocol.http.HttpURLConnection.getOutputStream(),
and the internal contract may be broken only if some of the codes
refactored (no chance for user application), say, modification for the
setRequestMethod() like this:
- this.method = methodTokens[i];
+ this.method = method;
But this can be easily monitored by test codes below:
public void testGetOutputStream() throws Exception {
HttpURLConnection c = (HttpURLConnection) new
URL("http://127.0.0.1:"
+ port).openConnection();
c.setDoOutput(true);
c.setRequestMethod(new String("GET"));
c.getOutputStream();
...//other for POST/PUT
}
This test is enough to enforced the internal contract, it will shout if
some refactory breaks the internal contract. So IMHO, I think the patch
for HttpURLConnection is over-designed.
However, I even have no strong objection on it. What makes me more
confused is the introduction of injected
java.net.HttpURLConnectionAccessor, I suggest to avoid the injected
helper class as long as possible, because it introduces complexity to
understand/manage and is sometimes fragile because it may highly coupled
with implementation details, for this specific case, a mocked subclass
of o.a.h....HttpURLConnection with overridden getRequestMethod can
catch the fragility of the internal contract.
Mikhail Loenko wrote:
> I've created regression test and patch for
> http://issues.apache.org/jira/browse/HARMONY-482
>
> I had to make some changes in the luni's build.xml,
> that are intended to be fixed once we all agree with
> proposed test suite layout.
>
> Please review the changes.
>
> Thanks,
> Mikhail
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
--
Paulex Yang
China Software Development Lab
IBM
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Mikhail Loenko <ml...@gmail.com>.
Hi Mark
2006/5/23, Mikhail Loenko <ml...@gmail.com>:
> Thanks, Mark!
>
> Could you please also review my recent changes in crypto's build.xml?
>
> Thanks,
> Mikhail
>
> 2006/5/23, Mark Hindess <ma...@googlemail.com>:
> >
> > On 23 May 2006 at 15:54, "Mikhail Loenko" <ml...@gmail.com> wrote:
> > > I've created regression test and patch for
> > > http://issues.apache.org/jira/browse/HARMONY-482
> > >
> > > I had to make some changes in the luni's build.xml,
> > > that are intended to be fixed once we all agree with
> > > proposed test suite layout.
> >
> > Once upon a time, the logging build.xml had similar changes. It
> > still does except it has since been renamed test-internal rather
> > than injected. I prefer the way that this is handled in the logging
> > build.xml - specifically:
> >
> > * a separate variable for test.injected path
> > * a separate directory - not subdirectory - for compiled classes
> > * using <bootclasspath> rather than jvmarg
the latter one does not work well with
-Dtest.jre.home="C:\Program Files\Java\jrockit-jdk1.5.0-windows-ia32"
it crashes VM
Thanks,
Mikhail
> >
> > However we do it, these two build.xml files should be consistent.
> > (Despite a couple of failed JIRAs, I still hope that one day we will
> > simplifying the build.xml files and having them use similar mechanisms
> > makes that *much* easier.)
> >
> > Regards,
> > Mark.
> >
> >
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] please review my regression test and patch for HttpURLConnection
Posted by Mikhail Loenko <ml...@gmail.com>.
Thanks, Mark!
Could you please also review my recent changes in crypto's build.xml?
Thanks,
Mikhail
2006/5/23, Mark Hindess <ma...@googlemail.com>:
>
> On 23 May 2006 at 15:54, "Mikhail Loenko" <ml...@gmail.com> wrote:
> > I've created regression test and patch for
> > http://issues.apache.org/jira/browse/HARMONY-482
> >
> > I had to make some changes in the luni's build.xml,
> > that are intended to be fixed once we all agree with
> > proposed test suite layout.
>
> Once upon a time, the logging build.xml had similar changes. It
> still does except it has since been renamed test-internal rather
> than injected. I prefer the way that this is handled in the logging
> build.xml - specifically:
>
> * a separate variable for test.injected path
> * a separate directory - not subdirectory - for compiled classes
> * using <bootclasspath> rather than jvmarg
>
> However we do it, these two build.xml files should be consistent.
> (Despite a couple of failed JIRAs, I still hope that one day we will
> simplifying the build.xml files and having them use similar mechanisms
> makes that *much* easier.)
>
> Regards,
> Mark.
>
>
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org