jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http: control/CookieManager.java gui/CookiePanel.java
Date Sun, 28 Feb 2016 21:44:46 GMT
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> wrote:
> On Sat, Feb 27, 2016 at 2:52 PM, sebb <sebbaz@gmail.com> 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> 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> 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.

Mime
View raw message