harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yang Paulex" <paulex.y...@gmail.com>
Subject Re: [classlib] please review my regression test and patch for HttpURLConnection
Date Thu, 25 May 2006 10:12:52 GMT
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.

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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message