harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mikhail Loenko" <mloe...@gmail.com>
Subject Re: [classlib] please review my regression test and patch for HttpURLConnection
Date Thu, 25 May 2006 11:22:09 GMT
2006/5/25, Yang Paulex <paulex.yang@gmail.com>:
> 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.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 <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.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


Mime
View raw message