jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@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 Mon, 29 Feb 2016 23:43:37 GMT
On 29 February 2016 at 17:17, Philippe Mouawad
<philippe.mouawad@gmail.com> 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:_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

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.

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

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.

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

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

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.

> Another option is to make it private.

How many times do I have to say this:

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.

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

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

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.

Mime
View raw message