Bug 57381 - HTTP(S) Test Script Recorder should display an error if Target Controller references a Recording Controller and no Recording Controller exists
Summary: HTTP(S) Test Script Recorder should display an error if Target Controller ref...
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: HTTP (show other bugs)
Version: 2.12
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords: PatchAvailable
Depends on:
Blocks: 25430
  Show dependency tree
 
Reported: 2014-12-21 14:57 UTC by Philippe Mouawad
Modified: 2014-12-27 16:02 UTC (History)
3 users (show)



Attachments
Patch implementing feature (4.63 KB, patch)
2014-12-24 14:30 UTC, UbikLoadPack support
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Mouawad 2014-12-21 14:57:27 UTC
Currently, HTTP(S) Test Script Recorder is very permissive when no recording controller is setup.
Documentation only mentions Recording Controller possibility but implementation tries to find a node in the following order:
- Defined / Saved target
- Recording Controller
- Thread Group
- Workbench

This behaviour is disturbing for many reasons:
- Created samples can sometime appear where you do not expect them to
- They can be created in Workbench which is weird as they will not be really usable

I propose to change this behaviour to:
- Display an error popup on start of proxy if targetNode.getTestElement() is not of type : GenericController or AbstractThreadGroup.

In fact I think it should be only Recording Controller but this may break existing behaviour.

Thoughts ?
Comment 1 Philippe Mouawad 2014-12-21 15:02:20 UTC
(In reply to Philippe Mouawad from comment #0)
> Currently, HTTP(S) Test Script Recorder is very permissive when no recording
> controller is setup.
> Documentation only mentions Recording Controller possibility but
> implementation tries to find a node in the following order:
> - Defined / Saved target
> - Recording Controller
> - Thread Group
> - Workbench
> 
> This behaviour is disturbing for many reasons:
> - Created samples can sometime appear where you do not expect them to
> - They can be created in Workbench which is weird as they will not be really
> usable
> 
> I propose to change this behaviour to:
> - Display an error popup on start of proxy if targetNode.getTestElement() is
> not of type : GenericController or AbstractThreadGroup.
> 
> In fact I think it should be only Recording Controller but this may break
> existing behaviour.
> 
> Thoughts ?

I propose to change this behaviour to:
- Display an error popup on start of proxy if "Target Controller" is configured to "Recording Controller" and if no "Recording Controller" exists

Thoughts
Comment 2 Milamber 2014-12-21 15:16:32 UTC

+1
The popup is a good idea.
Comment 3 Dzmitry Kashlach 2014-12-21 19:41:23 UTC
Hi JMeter team,

May be, another IMHO good variant is to remove "Use recording controller" from drop-down list e.g. to show user only nodes, which are really present in test-tree(Thread Groups, Controllers, whatever else)?
Comment 4 Sebb 2014-12-22 15:20:05 UTC
(In reply to Dzmitry Kashlach from comment #3)
> Hi JMeter team,
> 
> May be, another IMHO good variant is to remove "Use recording controller"
> from drop-down list e.g. to show user only nodes, which are really present
> in test-tree(Thread Groups, Controllers, whatever else)?

+1
Comment 5 UbikLoadPack support 2014-12-22 15:47:45 UTC
-1
This method is really helpful specially if you use Recording template.
Checking popup seems enough.

Regards
Comment 6 UbikLoadPack support 2014-12-22 15:48:02 UTC
(In reply to Milamber from comment #2)
> 
> +1
> The popup is a good idea.

+1 for this one
Comment 7 UbikLoadPack support 2014-12-22 15:54:45 UTC
(In reply to Dzmitry Kashlach from comment #3)
> Hi JMeter team,
> 
> May be, another IMHO good variant is to remove "Use recording controller"
> from drop-down list e.g. to show user only nodes, which are really present
> in test-tree(Thread Groups, Controllers, whatever else)?

Hi, another argument why removing it is not a good idea IMHO is bug 57382.
If you remove it then when reloading you will need to setup correctly target before using Test Plan, while with "Use recording controller", nothing is needed if you have a Recording controller.
Comment 8 Sebb 2014-12-22 16:01:13 UTC
(In reply to UbikLoadPack support from comment #5)
> -1
> This method is really helpful specially if you use Recording template.
> Checking popup seems enough.
> 
> Regards

Cannot the the recording template can be fixed so that it just works?
Comment 9 Sebb 2014-12-22 16:02:48 UTC
(In reply to UbikLoadPack support from comment #7)
> (In reply to Dzmitry Kashlach from comment #3)
> > Hi JMeter team,
> > 
> > May be, another IMHO good variant is to remove "Use recording controller"
> > from drop-down list e.g. to show user only nodes, which are really present
> > in test-tree(Thread Groups, Controllers, whatever else)?
> 
> Hi, another argument why removing it is not a good idea IMHO is bug 57382.
> If you remove it then when reloading you will need to setup correctly target
> before using Test Plan, while with "Use recording controller", nothing is
> needed if you have a Recording controller.

Not sure I see how Bug 57832 affects this.
Comment 10 UbikLoadPack support 2014-12-22 16:07:36 UTC
(In reply to Sebb from comment #9)
> (In reply to UbikLoadPack support from comment #7)
> > (In reply to Dzmitry Kashlach from comment #3)
> > > Hi JMeter team,
> > > 
> > > May be, another IMHO good variant is to remove "Use recording controller"
> > > from drop-down list e.g. to show user only nodes, which are really present
> > > in test-tree(Thread Groups, Controllers, whatever else)?
> > 
> > Hi, another argument why removing it is not a good idea IMHO is bug 57382.
> > If you remove it then when reloading you will need to setup correctly target
> > before using Test Plan, while with "Use recording controller", nothing is
> > needed if you have a Recording controller.
> 
> Not sure I see how Bug 57832 affects this.

Well if you remove "Use Recording Controller", then it means you rely on user setting the target controller explicitely right ?
So if he does, he saves his plans and reloads it later, due to bug the setting is forgotten and it now records not where user wanted it to record but as per the result of ProxyControl#findTargetControllerNode

So you have lost productivity with this change.

Regarding "Cannot the the recording template can be fixed so that it just works?" ? No you need to fix 57832 before.

Regards
Comment 11 Sebb 2014-12-22 16:20:32 UTC
(In reply to UbikLoadPack support from comment #10)
> (In reply to Sebb from comment #9)
> > (In reply to UbikLoadPack support from comment #7)
> > > (In reply to Dzmitry Kashlach from comment #3)
> > > > Hi JMeter team,
> > > > 
> > > > May be, another IMHO good variant is to remove "Use recording controller"
> > > > from drop-down list e.g. to show user only nodes, which are really present
> > > > in test-tree(Thread Groups, Controllers, whatever else)?
> > > 
> > > Hi, another argument why removing it is not a good idea IMHO is bug 57382.
> > > If you remove it then when reloading you will need to setup correctly target
> > > before using Test Plan, while with "Use recording controller", nothing is
> > > needed if you have a Recording controller.
> > 
> > Not sure I see how Bug 57832 affects this.
> 
> Well if you remove "Use Recording Controller", then it means you rely on
> user setting the target controller explicitely right ?

Yes.

> So if he does, he saves his plans and reloads it later, due to bug the
> setting is forgotten and it now records not where user wanted it to record
> but as per the result of ProxyControl#findTargetControllerNode
> 
> So you have lost productivity with this change.

Not if bug 57832 has been fixed, surely?
 
> Regarding "Cannot the the recording template can be fixed so that it just
> works?" ? No you need to fix 57832 before.

Fine, let's fix 57832.
 
> Regards
Comment 12 UbikLoadPack support 2014-12-22 16:22:24 UTC
(In reply to Sebb from comment #11)
> (In reply to UbikLoadPack support from comment #10)
> > (In reply to Sebb from comment #9)
> > > (In reply to UbikLoadPack support from comment #7)
> > > > (In reply to Dzmitry Kashlach from comment #3)
> > > > > Hi JMeter team,
> > > > > 
> > > > > May be, another IMHO good variant is to remove "Use recording controller"
> > > > > from drop-down list e.g. to show user only nodes, which are really present
> > > > > in test-tree(Thread Groups, Controllers, whatever else)?
> > > > 
> > > > Hi, another argument why removing it is not a good idea IMHO is bug 57382.
> > > > If you remove it then when reloading you will need to setup correctly target
> > > > before using Test Plan, while with "Use recording controller", nothing is
> > > > needed if you have a Recording controller.
> > > 
> > > Not sure I see how Bug 57832 affects this.
> > 
> > Well if you remove "Use Recording Controller", then it means you rely on
> > user setting the target controller explicitely right ?
> 
> Yes.
> 
> > So if he does, he saves his plans and reloads it later, due to bug the
> > setting is forgotten and it now records not where user wanted it to record
> > but as per the result of ProxyControl#findTargetControllerNode
> > 
> > So you have lost productivity with this change.
> 
> Not if bug 57832 has been fixed, surely?
>  
> > Regarding "Cannot the the recording template can be fixed so that it just
> > works?" ? No you need to fix 57832 before.
> 
> Fine, let's fix 57832.
>  
> > Regards


Ok, note that as users we would prefer "Use Recording Controller" to stay.
Comment 13 Sebb 2014-12-22 16:37:25 UTC
(In reply to UbikLoadPack support from comment #12)

> Ok, note that as users we would prefer "Use Recording Controller" to stay.

Why is it still necessary (assuming 57832 is fixed)?

It seems wrong to me to have a drop-down entry which refers to a non-existent controller.
Comment 14 UbikLoadPack support 2014-12-22 16:55:32 UTC
(In reply to Sebb from comment #13)
> (In reply to UbikLoadPack support from comment #12)
> 
> > Ok, note that as users we would prefer "Use Recording Controller" to stay.
> 
> Why is it still necessary (assuming 57832 is fixed)?
> 
> It seems wrong to me to have a drop-down entry which refers to a
> non-existent controller.

It is more of a conceptual Target saying Use Recording controller located somewhere in test plan. By implementing the popup check it would allow to leave this feature retaining backward compatibility.
Comment 15 Sebb 2014-12-22 17:12:41 UTC
(In reply to UbikLoadPack support from comment #14)
> (In reply to Sebb from comment #13)
> > (In reply to UbikLoadPack support from comment #12)
> > 
> > > Ok, note that as users we would prefer "Use Recording Controller" to stay.
> > 
> > Why is it still necessary (assuming 57832 is fixed)?
> > 
> > It seems wrong to me to have a drop-down entry which refers to a
> > non-existent controller.
> 
> It is more of a conceptual Target saying Use Recording controller located
> somewhere in test plan. By implementing the popup check it would allow to
> leave this feature retaining backward compatibility.

If I understand the pop-up proposal correctly, it would prevent the user from running the test plan. So it would not retain compatibility.

However maybe the intention is for the pop-up to be a Warning (rather than an Error).

But I'm not sure how useful it is to retain the existing behaviour, as it is rather unexpected.
Comment 16 UbikLoadPack support 2014-12-24 14:30:15 UTC
Created attachment 32330 [details]
Patch implementing feature
Comment 17 Philippe Mouawad 2014-12-27 15:22:34 UTC
Date: Sat Dec 27 15:22:01 2014
New Revision: 1648056

URL: http://svn.apache.org/r1648056
Log:
Bug 57381 - HTTP(S) Test Script Recorder should display an error if Target Controller references a Recording Controller and no Recording Controller exists
Bugzilla Id: 57381

Modified:
    jmeter/trunk/src/components/org/apache/jmeter/control/gui/TreeNodeWrapper.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/proxy/ProxyControl.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/proxy/gui/ProxyControlGui.java
    jmeter/trunk/xdocs/changes.xml
Comment 18 Philippe Mouawad 2014-12-27 15:22:44 UTC
Thanks for patch
Comment 19 Philippe Mouawad 2014-12-27 16:02:38 UTC
Date: Sat Dec 27 16:01:49 2014
New Revision: 1648060

URL: http://svn.apache.org/r1648060
Log:
Bug 57381 - HTTP(S) Test Script Recorder should display an error if Target Controller references a Recording Controller and no Recording Controller exists
Oups forgot i18n
Bugzilla Id: 57381

Modified:
    jmeter/trunk/src/core/org/apache/jmeter/resources/messages.properties
    jmeter/trunk/src/core/org/apache/jmeter/resources/messages_fr.properties