Mailing-List: contact harmony-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: harmony-dev@incubator.apache.org
Received-SPF: pass (asf.osuosl.org: domain of paulex.yang@gmail.com designates
 66.249.82.204 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:message-id:date:from:to:subject:in-reply-to:mime-version:content-type:references;
        b=l71CUHGGyNQowqZeaX1wBPAQf7KR1VcN9/jRwpFzL8vMfFkwaEvtA9bIYiPEHkC1x4usoDOEVaLgS0StMmgllvc9jl9brjSD0OtElusn+BiQ2KQLvhZ2hBAULMi4C8mZPMVFz6E98ZrxBiggrFpIxiQ4dyeVjV+zM4Kbuw3akcw=
Message-ID: <f982cec50605250341h1946f9d8we45552d554552efb@mail.gmail.com>
Date: Thu, 25 May 2006 18:41:55 +0800
From: "Yang Paulex" <paulex.yang@gmail.com>
To: harmony-dev@incubator.apache.org
Subject: Re: [classlib] please review my regression test and patch for
 HttpURLConnection
In-Reply-To: <f982cec50605250312k5e205c5dn286f4301bd5e346@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/alternative;
	boundary="----=_Part_4242_22805424.1148553715238"
References: <906dd82e0605230154j757fc3c5r82a36fdd17cd3297@mail.gmail.com>
	 <f982cec50605250227u78e82650s7fb3de347106147c@mail.gmail.com>
	 <906dd82e0605250242k779a49a2qa47dde5f4d9a23fe@mail.gmail.com>
	 <f982cec50605250312k5e205c5dn286f4301bd5e346@mail.gmail.com>

------=_Part_4242_22805424.1148553715238
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

2006/5/25, Yang Paulex <paulex.yang@gmail.com>:
>
> Hi, Mikhail,
>
> 2006/5/25, Mikhail Loenko <mloenko@gmail.com>:
>
> > Hi Paulex,
> >
> > 2006/5/25, Yang Paulex <paulex.yang@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.getOutputStrea=
m
> > (),
> >
> > 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 no=
t
> > 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 th=
e
> 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 =3D methodTokens[i];
> > > +                this.method =3D method;
> > >
> > > But this can be easily monitored by test codes below:
> > >
> > >   public void testGetOutputStream() throws Exception {
> > >       HttpURLConnection c =3D (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 fo=
r
> > > 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 subclas=
s
> > 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 =3D 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 =3D method;
>         }
>     }
>
> Thanks,
> > Mikhail
> >
> > >
> > > Your comments?
> > >
> > >
> > > 2006/5/23, Mikhail Loenko <mloenko@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.or=
g
> > > > 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
>


--=20
Paulex Yang
China Software Development Labotary
IBM

------=_Part_4242_22805424.1148553715238--