jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Backward Compatibility manamgement and the case of CookieManager/CookiePanel was Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http: control/CookieManager.java gui/CookiePanel.java
Date Mon, 29 Feb 2016 17:17:15 GMT
Hi Sebb
I hope my new answers will be clear.

Regards
Philippe

On Mon, Feb 29, 2016 at 4:25 PM, sebb <sebbaz@gmail.com
<javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>> wrote:

> On 29 February 2016 at 12:27, Philippe Mouawad
> <philippe.mouawad@gmail.com
> <javascript:_e(%7B%7D,'cvml','philippe.mouawad@gmail.com');>> wrote:
> > Greetings sebb,
> >
> > I reviewed the code commited in trunk and sorry, I don't find it clear
> and
> > it does not suit me.
>
> Have you reviewed the patch?
>

Absolutely. I read it 10 times, the mail + the commit (the reverts one my
code).


>
> Trunk does not have the descriptive comments yet.
>

Ok but I don't think I wrote anything about that

>
> > For me it is an issue that Default policy (
> >
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java#L107
> )
> > and implementation (
> >
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java#L110
> )
> > in CookieManager are not the real defaults and that the GUI holds the
> real
> > defaults (
> >
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java#L90
> ,
> >
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java#L95
> ),
> > for me:
> >
> >    - it makes code "ugly", a field named DEFAULT_POLICY in the model is
> NOT
> >    the default policy ?, same for DEFAULT_IMPLEMENTATION ? really strange
>
> Have you read the patch which clearly documents what the fields mean?
>

Absolutely, and as I wrote, my problem is not the documentation, it is the
code, I think good code should be a kind of "Auto documented". Of course
javadocs and docs are welcome but they don't fix "bad" code.
I see that you deprecate it, but for me it should remain here and be
changed  to reflect the new default.


> >    - it breaks MVC pattern as it's the GUI which overrides the Model
>
> It *IS* the GUI which applies the defaults.
>

This is exactly my problem. It breaks for me the MVC model, as the Model is
not the GUI, so it should be the sampler that decides about the default.

Another option is to make it private.



> This is only exposed here because the GUI default has been changed
> from the JMX default.
>

I am aware of that.

>
> > So -1 for me.
>
> We cannot change the public DEFAULT constant names or values in
> CookieManager without breaking compatibility.
>

I disagree with this note.
My previous patch did not break any backward compatibility except for 3rd
party code using the Java Constants.
As such , as we switch to 3.0 we CAN break this APIs as we WILL document in
changes that we changed this.
Backward compatibility is important, but it can't be the sole MOTTO of
JMeter. I would like this release to drop all the "concessions" to Backward
compatibility that were made (and were good at the time they were made),
but we cannot live indefinitely with them, let 3.0 be the rocket that will
drive us to the Mars of Load Testing tools...


That's also why I proposed to drop for future evolutions the "pattern" that
we used until now which was :
- setProperty(key, value, default_value ) => Which results in default not
being written to JMX. It that had been done since the begining, we would be
able to change the default without any problem, the bug would not have
triggered

By only calling setProperty(key, value) , the default will be written to
file and that's it.
The impact on jmx will be a slight increase in size.
Yes it might break some test cases, but we can fix them.




>
> Please review the patch documentation and see if that answers your
> complaints.
>



>
> > Regards
> >
> > Philippe
> >
> > On Mon, Feb 29, 2016 at 12:36 AM, sebb <sebbaz@gmail.com
> <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>> wrote:
> >
> >> OK, I have reverted the commits to CookieManager and CookiePanel.
> >>
> >> I reapplied your commit to CookiePanel.
> >> In the process I found another bug in CookiePanel - not all the
> >> references to the DEFAULT_POLICY were correctly updated.
> >> I also extracted the default implementation as a constant and added
> >> Javadoc to both defaults.
> >>
> >> My suggested patch for CookieManager is as follows.
> >>
> >> Does that make it clear enough what the fields mean?
> >>
> >>
> >> Index: CookieManager.java
> >> ===================================================================
> >> --- CookieManager.java    (revision 1732815)
> >> +++ CookieManager.java    (working copy)
> >> @@ -102,11 +102,28 @@
> >>
> >>      private transient CollectionProperty initialCookies;
> >>
> >> -    // MUST NOT BE CHANGED
> >> -    @SuppressWarnings("deprecation") // cannot be changed
> >> +    /**
> >> +     * Defines the policy that is assumed when the JMX file does not
> >> contain an entry for it
> >> +     * MUST NOT BE CHANGED otherwise JMX files will not be correctly
> >> interpreted
> >> +     * <p>
> >> +     * The default policy for new CookieManager elements is defined by
> >> +     * {@link
> >> org.apache.jmeter.protocol.http.gui.CookiePanel#DEFAULT_POLICY
> >> CookiePanel#DEFAULT_POLICY}
> >> +     *
> >> +     * @deprecated not intended for use outside this class (should
> >> have been private)
> >> +     */
> >> +    @Deprecated
> >>      public static final String DEFAULT_POLICY =
> >> CookieSpecs.BROWSER_COMPATIBILITY;
> >>
> >> -    // MUST NOT BE CHANGED
> >> +    /**
> >> +     * Defines the implementation that is assumed when the JMX file
> >> does not contain an entry for it
> >> +     * MUST NOT BE CHANGED otherwise JMX files will not be correctly
> >> interpreted
> >> +     * <p>
> >> +     * The default implementation for new CookieManager elements is
> >> defined by
> >> +     * {@link
> >> org.apache.jmeter.protocol.http.gui.CookiePanel#DEFAULT_IMPLEMENTATION
> >> CookiePanel#DEFAULT_IMPLEMENTATION}
> >> +     *
> >> +     * @deprecated not intended for use outside this class (should
> >> have been private)
> >> +     */
> >> +    @Deprecated
> >>      public static final String DEFAULT_IMPLEMENTATION =
> >> HC3CookieHandler.class.getName();
> >>
> >>      public CookieManager() {
> >>
> >>
> >>
> >> On 28 February 2016 at 22:43, Philippe Mouawad
> >> <philippe.mouawad@gmail.com
> <javascript:_e(%7B%7D,'cvml','philippe.mouawad@gmail.com');>> wrote:
> >> > On Sunday, February 28, 2016, sebb <sebbaz@gmail.com
> <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>> wrote:
> >> >
> >> >> On 28 February 2016 at 21:51, Philippe Mouawad
> >> >> <philippe.mouawad@gmail.com
> <javascript:_e(%7B%7D,'cvml','philippe.mouawad@gmail.com');>
> <javascript:;>> wrote:
> >> >> > Hi sebb,
> >> >> > Could you make a Patch to see what final code will look like ?
> >> >>
> >> >> Not until you revert the commit.
> >> >
> >> >
> >> > That's funny.
> >> >
> >> > do it if you want, I have no time to lose reverting code that works
> and
> >> is
> >> > readable.
> >> > I don't see under what particular conditions you cannot make a patch
> and
> >> I
> >> > would have to revert my work
> >> >
> >> >
> >> >
> >> >>
> >> >> > Frankly I don't understand why you want me to revert, there is
no
> bug
> >> >> and I
> >> >> > think code is easier to read and understand (without
> documentation).
> >> >>
> >> >> The commit included two separate changes AND it changed constants.
> >> >
> >> >
> >> > No, the aim was to fix and cleanup code which was for me unreadable.
> >> >
> >> > You can disagree but it does not mean you are right
> >> >
> >> >
> >> >>
> >> >> Please now revert the commit r1732634
> >> >
> >> >
> >> >  Feel free to do it.
> >> >
> >> >
> >> >
> >> >> > What constants did I rename ? I introduced new ones.
> >> >>
> >> >> DEFAULT_POLICY => POLICY_FOR_BACKWARD_COMPATIBILITY
> >> >
> >> >
> >> > I then reintroduced Default_policy with the real default policy.
> >> >
> >> >
> >> >> > We can change their values , I cannot imagine Defaults will remain
> as
> >> is
> >> >> > for years.
> >> >>
> >> >> No we cannot, because that changes the way JMXs are interpreted
> >> >
> >> >
> >> > Not acceptable for me.
> >> > I remember we already did it in the past.
> >> > One thing I remember are the defaults of transaction controller,
> >> processing
> >> > time inclusion and generate parent sample.
> >> > At least 2 or 3 changes
> >> >
> >> >
> >> >>
> >> >> > We can change their values, because we created a 3.0 and can
> introduce
> >> >> > breaking changes, knowing that we document a lot all breaking
> changes
> >> (I
> >> >> > don't see many project who care so much about documenting all
the
> >> >> changes).
> >> >> >
> >> >> > Regards
> >> >> > Philippe
> >> >> >
> >> >> >
> >> >> > On Sun, Feb 28, 2016 at 10:44 PM, sebb <sebbaz@gmail.com
> <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>
> >> <javascript:;>>
> >> >> wrote:
> >> >> >
> >> >> >> My objection is two-fold:
> >> >> >>
> >> >> >> Public constants must not be renamed, nor (in general) can
their
> >> >> >> values be changed.
> >> >> >>
> >> >> >> The fix to CookiePanel is a separate bug: i.e. the implementation
> >> must
> >> >> >> be set before the policy.
> >> >> >> It's basically a continuation of the fix I made in r1732595
(I
> forgot
> >> >> >> to check for other instances of the wrong ordering)
> >> >> >> It does not depend on the change to CookieManager; in fact
that
> >> change
> >> >> >> does not affect it at all because DEFAULT_IMPLEMENTATION is
> defined
> >> in
> >> >> >> CookiePanel.
> >> >> >>
> >> >> >> So please revert the commit.
> >> >> >>
> >> >> >> You can then reapply the change to CookiePanel.
> >> >> >>
> >> >> >> I will then update CookieManager to better document the constant.
> >> >> >>
> >> >> >>
> >> >> >> On 27 February 2016 at 13:58, Philippe Mouawad
> >> >> >> <philippe.mouawad@gmail.com
> <javascript:_e(%7B%7D,'cvml','philippe.mouawad@gmail.com');>
> <javascript:;>> wrote:
> >> >> >> > On Sat, Feb 27, 2016 at 2:52 PM, sebb <sebbaz@gmail.com
> <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>
> >> >> <javascript:;>> wrote:
> >> >> >> >
> >> >> >> >> Also the commit fixed a bug and made some unrelated
changes.
> >> >> >> >>
> >> >> >> >> Don't mix separate fixes.
> >> >> >> >>
> >> >> >> >> It's much harder to review and to follow the history
later.
> >> >> >> >>
> >> >> >> >> Please revert and re-apply the change to CookiePanel
> separately.
> >> >> >> >>
> >> >> >> > It is part of the fix.
> >> >> >> >
> >> >> >> >>
> >> >> >> >> I don't agree with the change to CookieManager; that
needs
> further
> >> >> >> >> discussion.
> >> >> >> >>
> >> >> >> >
> >> >> >> > Based  on my last commit few minutes ago, can you open
a new
> >> >> discussion
> >> >> >> (as
> >> >> >> > subject is not clear) to mention what you disagree with
?
> >> >> >> > Thanks
> >> >> >> >
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> On 27 February 2016 at 13:45, sebb <sebbaz@gmail.com
> <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>
> >> <javascript:;>>
> >> >> wrote:
> >> >> >> >> > -1
> >> >> >> >> >
> >> >> >> >> > Must not change public constant names.
> >> >> >> >> >
> >> >> >> >> > If the name is not ideal, fix this by adding
Javadoc, not a
> >> rename.
> >> >> >> >> >
> >> >> >> >> > On 27 February 2016 at 12:53,  <pmouawad@apache.org
> <javascript:_e(%7B%7D,'cvml','pmouawad@apache.org');>
> >> <javascript:;>>
> >> >> wrote:
> >> >> >> >> >> Author: pmouawad
> >> >> >> >> >> Date: Sat Feb 27 12:53:18 2016
> >> >> >> >> >> New Revision: 1732634
> >> >> >> >> >>
> >> >> >> >> >> URL: http://svn.apache.org/viewvc?rev=1732634&view=rev
> >> >> >> >> >> Log:
> >> >> >> >> >> Bug 58756 - CookieManager : Cookie Policy
select box content
> >> must
> >> >> >> >> depend on Cookie implementation
> >> >> >> >> >> A) Fix bug introduced by commit r1732632:
> >> >> >> >> >> 1/ Save with 2.13 a Plan containing CookieManager
that uses
> >> >> defaults
> >> >> >> >> >> 2/ Open it with trunk
> >> >> >> >> >> 3/ It is ok
> >> >> >> >> >> 4/ Close it
> >> >> >> >> >> 5/ Add a new CookieManager => KO the
policy is default
> instead
> >> of
> >> >> >> >> standard
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >> B) Also made constants explicit and removed
dependency on
> >> >> deprecated
> >> >> >> >> class.
> >> >> >> >> >>
> >> >> >> >> >> C) Forced the save of policy and implementation
even if
> there
> >> >> values
> >> >> >> >> are set at defaults to avoid this headache again
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >> Bugzilla Id: 58756
> >> >> >> >> >>
> >> >> >> >> >> Modified:
> >> >> >> >> >>
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> >> >> >>
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> >> >> >>
> >> >> >> >> >> Modified:
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> >> >> >> URL:
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java?rev=1732634&r1=1732633&r2=1732634&view=diff
> >> >> >> >> >>
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> ==============================================================================
> >> >> >> >> >> ---
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> >> >> (original)
> >> >> >> >> >> +++
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> >> >> Sat Feb 27 12:53:18 2016
> >> >> >> >> >> @@ -30,7 +30,6 @@ import java.io.Serializable;
> >> >> >> >> >>  import java.net.URL;
> >> >> >> >> >>  import java.util.ArrayList;
> >> >> >> >> >>
> >> >> >> >> >> -import org.apache.http.client.config.CookieSpecs;
> >> >> >> >> >>  import org.apache.jmeter.config.ConfigTestElement;
> >> >> >> >> >>  import org.apache.jmeter.engine.event.LoopIterationEvent;
> >> >> >> >> >>  import org.apache.jmeter.testelement.TestIterationListener;
> >> >> >> >> >> @@ -102,12 +101,19 @@ public class CookieManager
extends
> Confi
> >> >> >> >> >>
> >> >> >> >> >>      private transient CollectionProperty
initialCookies;
> >> >> >> >> >>
> >> >> >> >> >> -    // MUST NOT BE CHANGED
> >> >> >> >> >> -    @SuppressWarnings("deprecation") //
cannot be changed
> >> >> >> >> >> -    public static final String DEFAULT_POLICY
=
> >> >> >> >> CookieSpecs.BROWSER_COMPATIBILITY;
> >> >> >> >> >> +    // MUST NOT BE CHANGED because as defaults
are not
> saved,
> >> >> >> >> >> +    // when a Test Plan was loaded from
an N-1 version and
> if
> >> >> >> defaults
> >> >> >> >> changed,
> >> >> >> >> >> +    // you end up changing the previously
set policy
> >> >> >> >> >> +    // see issues with Bug 58756
> >> >> >> >> >> +    public static final String
> >> POLICY_FOR_BACKWARD_COMPATIBILITY
> >> >> =
> >> >> >> >> "compatibility";
> >> >> >> >> >>
> >> >> >> >> >> -    // MUST NOT BE CHANGED
> >> >> >> >> >> -    public static final String DEFAULT_IMPLEMENTATION
=
> >> >> >> >> HC3CookieHandler.class.getName();
> >> >> >> >> >> +    // MUST NOT BE CHANGED because as defaults
are not
> saved,
> >> >> >> >> >> +    // when a Test Plan was loaded from
an N-1 version and
> if
> >> >> >> defaults
> >> >> >> >> changed,
> >> >> >> >> >> +    // you end up changing the previously
set policy
> >> >> >> >> >> +    // see issues with Bug 58756
> >> >> >> >> >> +    public static final String
> >> >> >> >> IMPLEMENTATION_FOR_BACKWARD_COMPATIBILITY =
> >> >> >> >> HC3CookieHandler.class.getName();
> >> >> >> >> >> +
> >> >> >> >> >> +    public static final String DEFAULT_IMPLEMENTATION
=
> >> >> >> >> HC4CookieHandler.class.getName();
> >> >> >> >> >>
> >> >> >> >> >>      public CookieManager() {
> >> >> >> >> >>          clearCookies(); // Ensure that
there is always a
> >> >> collection
> >> >> >> >> available
> >> >> >> >> >> @@ -124,11 +130,13 @@ public class CookieManager
extends
> Confi
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>      public String getPolicy() {
> >> >> >> >> >> -        return getPropertyAsString(POLICY,
DEFAULT_POLICY);
> >> >> >> >> >> +        return getPropertyAsString(POLICY,
> >> >> >> >> POLICY_FOR_BACKWARD_COMPATIBILITY);
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>      public void setCookiePolicy(String
policy){
> >> >> >> >> >> -        setProperty(POLICY, policy, DEFAULT_POLICY);
> >> >> >> >> >> +        // we must explicitely save the
policy
> >> >> >> >> >> +        // not use a default implementation
> >> >> >> >> >> +        setProperty(POLICY, policy);
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>      public CollectionProperty getCookies()
{
> >> >> >> >> >> @@ -148,11 +156,13 @@ public class CookieManager
extends
> Confi
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>      public String getImplementation() {
> >> >> >> >> >> -        return getPropertyAsString(IMPLEMENTATION,
> >> >> >> >> DEFAULT_IMPLEMENTATION);
> >> >> >> >> >> +        return getPropertyAsString(IMPLEMENTATION,
> >> >> >> >> IMPLEMENTATION_FOR_BACKWARD_COMPATIBILITY);
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>      public void setImplementation(String
implementation){
> >> >> >> >> >> -        setProperty(IMPLEMENTATION, implementation,
> >> >> >> >> DEFAULT_IMPLEMENTATION);
> >> >> >> >> >> +        // we must explicitely save the
policy
> >> >> >> >> >> +        // not use a default implementation
> >> >> >> >> >> +        setProperty(IMPLEMENTATION, implementation);
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>      /**
> >> >> >> >> >>
> >> >> >> >> >> Modified:
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> >> >> >> URL:
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java?rev=1732634&r1=1732633&r2=1732634&view=diff
> >> >> >> >> >>
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> ==============================================================================
> >> >> >> >> >> ---
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> >> >> (original)
> >> >> >> >> >> +++
> >> >> >> >>
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> >> >> Sat Feb 27 12:53:18 2016
> >> >> >> >> >> @@ -280,9 +280,9 @@ public class CookiePanel
extends Abstrac
> >> >> >> >> >>
> >> >> >> >> >>          tableModel.clearData();
> >> >> >> >> >>          clearEachIteration.setSelected(false);
> >> >> >> >> >> -
> policy.setText(HC4CookieHandler.DEFAULT_POLICY_NAME);
> >> >> >> >> >>
> >> selectHandlerPanel.setSelectedItem(DEFAULT_IMPLEMENTATION
> >> >> >> >> >>
> >> >> .substring(DEFAULT_IMPLEMENTATION.lastIndexOf('.') +
> >> >> >> >> 1));
> >> >> >> >> >> +
> policy.setText(HC4CookieHandler.DEFAULT_POLICY_NAME);
> >> >> >> >> >>          deleteButton.setEnabled(false);
> >> >> >> >> >>          saveButton.setEnabled(false);
> >> >> >> >> >>      }
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >> > --
> >> >> >> > Cordialement.
> >> >> >> > Philippe Mouawad.
> >> >> >>
> >> >> >
> >> >> >
> >> >> >
> >> >> > --
> >> >> > Cordialement.
> >> >> > Philippe Mouawad.
> >> >>
> >> >
> >> >
> >> > --
> >> > Cordialement.
> >> > Philippe Mouawad.
> >>
> >
> >
> >
> > --
> > Cordialement.
> > Philippe Mouawad.
>



-- 
Cordialement.
Philippe Mouawad.




-- 
Cordialement.
Philippe Mouawad.

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