jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: 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 Tue, 01 Mar 2016 06:19:38 GMT
On Tuesday, March 1, 2016, sebb <sebbaz@gmail.com> wrote:

> On 29 February 2016 at 17:17, Philippe Mouawad
> <philippe.mouawad@gmail.com <javascript:;>> wrote:
> > 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:;>
> > <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com <javascript:;>');>>
> wrote:
> >
> >> On 29 February 2016 at 12:27, Philippe Mouawad
> >> <philippe.mouawad@gmail.com <javascript:;>
> >> <javascript:_e(%7B%7D,'cvml','philippe.mouawad@gmail.com <javascript:;>');>>
> 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
>
> You wrote:
>
> > I reviewed the code commited in trunk and sorry, I don't find it clear
> and
> > it does not suit me.
>
> You made no comment about my patch, so I had to assume you had not read it.


you were wrong

>
> >>
> >> > 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.
>
> Public constants cannot be changed.


Do as you like.
it seems you don't want to take into account any of my arguments ...

>
> I agree that using a public constant for these values was a mistake,
> but it's too late now.
>
> The code is working now, and AFAICT the only problem you have is that
> the constant has the wrong name.


it did with my code

>
> I'm sorry, but you will just have to live with it.


that's argumentation.
Because you're the final decider ? ...



>
> >
> >> >    - 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.
>
> But ALL the test elements work the same way.
> The clear() method is what initialises the GUI, and that is in the GUI.

No, check the code

>
> It may well be that they don't fit the MVC model.
> The design has been around for a very long time, probably before the
> model existed.
> I don't think we should rewrite the whole GUI just to fit in with one
> particular model.


let's not change anything , never ever...

>
> > Another option is to make it private.
>
> How many times do I have to say this:


Repeating something wrong do not maje it right.


>
> Public constants cannot be changed.
>
> >
> >
> >> 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 is a meaningless statement.


Just some joke ... No need to be rude

>
> >
> > 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
>
> Using a default does NOT cause a bug.


It does

>
>
>

The problem is using the same constant for the JMX default as for the
> GUI default.
>
> Which I solved by creating a new constant in the GUI class.
>
> However I will happily move the constant to the sampler class if that
> will help make it look more like an MVC model.


As you like.
I am tired of this discussion


>
> > 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.
>
> But there is no need to do this.
>
> What we need to do going forward is ensure that the JMX and GUI
> default constants are clearly documented.


let's document bad design.



>
> Both constants should ideally be private, with a getter for the GUI
> default if necessary.
>
> If the constant is private, of course it can be renamed.
> However its value must remain.
>

As you like


-- 
Cordialement.
Philippe Mouawad.

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