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 22:30:38 GMT
On 28 February 2016 at 21:51, Philippe Mouawad
<philippe.mouawad@gmail.com> wrote:
> Hi sebb,
> Could you make a Patch to see what final code will look like ?

Not until you revert the commit.

> 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.

Please now revert the commit r1732634

> What constants did I rename ? I introduced new ones.

DEFAULT_POLICY => POLICY_FOR_BACKWARD_COMPATIBILITY

> 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

> 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> 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> 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.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message