-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add prefix or suffix sampler numbering for HTTP Test Script Recorder #571
Conversation
Could this pull request will be merge ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Thanks for PR.
See my remarks which require changes before considering merge.
Thanks
@@ -55,9 +55,19 @@ | |||
/* | |||
* Optionally number the requests | |||
*/ | |||
private static final boolean NUMBER_REQUESTS = | |||
private final boolean NUMBER_REQUESTS = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if those fields are not static, then they should be camel case
protected static boolean isNumberRequests() { | ||
return NUMBER_REQUESTS; | ||
protected boolean isNumberRequests() { | ||
boolean bNumberRequest = JMeterUtils.getPropDefault("proxy.number.requests", true); // $NON-NLS-1$; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's not a simple getter ? just return NUMBER_REQUEST (to be camelCased)
} | ||
|
||
protected String getNumberValueFormat() { | ||
String sNumberValueFormat = JMeterUtils.getPropDefault("proxy.number.value_format", "%03d"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same note
} | ||
|
||
protected String getNumberMode() { | ||
String sMumberMode = JMeterUtils.getPropDefault("proxy.number.mode", "prefix"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same note
@@ -61,6 +62,8 @@ | |||
private static final int SAMPLER_NAME_NAMING_MODE_PREFIX = 0; // $NON-NLS-1$ | |||
private static final int SAMPLER_NAME_NAMING_MODE_COMPLETE = 1; // $NON-NLS-1$ | |||
|
|||
private static final String SAMPLER_NUMBERING_MODE_NAME_PREFIX = "prefix"; // $NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you didn't use the enum HttpSamplerNumberingMode ?
if (HttpSamplerNumberingMode.PREFIX.getStringMode().equals(sNumberingMode)) { | ||
setHttpSampleNumberingMode(HttpSamplerNumberingMode.PREFIX.getIntValue()); | ||
} | ||
if (HttpSamplerNumberingMode.SUFFIX.getStringMode().equals(sNumberingMode)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if
sNumberingMode = HttpSamplerNumberingMode.PREFIX.getStringMode(); | ||
} | ||
|
||
if (httpSampleNumberingMode == HttpSamplerNumberingMode.SUFFIX.getIntValue()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if
SamplerCreator samplerCreator = samplerCreatorFactory.getDefaultSamplerCreator(); | ||
|
||
iRequestNumber = samplerCreator.getRequestNumber(); | ||
iNumberingStartValue = iRequestNumber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this ?
if(HTTP_SAMPLER_NUMBERING_MODE.equals(combo.getName())) { | ||
model.setHttpSampleNumberingMode(httpSampleNumberingMode.getSelectedIndex()); | ||
if (httpSampleNumberingMode.getSelectedIndex() == 0) { | ||
JMeterUtils.setProperty("proxy.number.mode", "prefix"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this and not using setter/getters ?
…(S) Test Script Recorder with the sampler name numbering mode
@@ -442,6 +483,12 @@ public void actionPerformed(ActionEvent action) { | |||
} | |||
} else if (command.equals(ACTION_RESTART)) { | |||
model.stopProxy(); | |||
// what is the last number for numbering samplers, ex : 96 | |||
int iRequestNumber = model.getRequestNumberFromSamplerCreator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the i
at the beginning of iRequestNumber
is used to indicate an integer, I would remove it and name the variable requestNumber
. That is the way variables are named in JMeter most of the time. Most developers around JMeter will probably use an IDE, which shows the type of the variable anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use prefix i for Integer or s for String for local variables but i could change to requestNumber
@@ -6720,13 +6719,34 @@ Both Chrome and Internet Explorer use the same trust store for certificates. | |||
By default it also removes <code>If-Modified-Since</code> and <code>If-None-Match</code> headers. | |||
These are used to determine if the browser cache items are up to date; | |||
when recording one normally wants to download all the content. | |||
To change which additional headers are removed, define the JMeter property <code>proxy.headers.remove</code> | |||
To change which additional headers are removed, define the JMeter property <code>proxy.heade'rs.remove</code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be a strange name for a property. Is proxy.heade'rs
really correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't change this line, i get it from a previous version before the modification, but you are rigth, the correct proxy.headers.remove
as a comma-separated list of headers. | ||
</property> | ||
<property name="Add Assertions" required="Yes">Add a blank assertion to each sampler?</property> | ||
<property name="Regex Matching" required="Yes">Use Regex Matching when replacing variables? If checked replacement will use word boundaries, i.e. it will only replace word matching values of variable, not part of a word. A word boundary follows Perl5 definition and is equivalent to <code>\b</code>. More information below in the paragraph about "<code>User Defined Variable replacement</code>".</property> | ||
<property name="Prefix/Transaction name" required="No">Add a prefix to sampler name during recording (Prefix mode). Or replace sampler name by user chosen name (Transaction name)</property> | ||
<property name="Create new transaction after request (ms)">Inactivity time between two requests needed to consider them in two separate groups.</property> | ||
<property name="Numbering Sampler Choice" required="Yes">Select the numbering mode for sampler name<br/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a mismatch in spaces at the front. I think the property
tags should match up.
<property name="Create new transaction after request (ms)">Inactivity time between two requests needed to consider them in two separate groups.</property> | ||
<property name="Numbering Sampler Choice" required="Yes">Select the numbering mode for sampler name<br/> | ||
<ul> | ||
<li><code>Without numbering</code> no numbering, ex : '/a.png'</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ex : ... is probably a French abbreviation (it means excercise in English). In English you can use e.g., or for example. Also note, that there is no space before a colon in English typography. And regarding the markup of /a.png
, I would use a code
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i will use e.g.
int httpSampleNameMode = request.getHttpSampleNameMode(); | ||
if (!HTTPConstants.CONNECT.equals(request.getMethod()) && isNumberRequests()) { | ||
if(StringUtils.isNotEmpty(prefix)) { | ||
// with a prefix name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these parts be extracted in smaller functions/methods? You could use names to tell the reader what the function is doing instead of using a comment and at the same time the code gets less (at least here :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add a comment because "prefix" have 2 significations :
- prefix name
- prefix numbering
This is prefix name.
HttpRequestHdr request) { | ||
String prefix = request.getPrefix(); | ||
protected void computeSamplerName(HTTPSamplerBase sampler, HttpRequestHdr request) { | ||
String prefix = request.getPrefix(); // ppp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of ppp ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ppp is "ppp"refix name" i use this string to test (look at the screen shot for the description of the modification and the test).
I could rename ppp with pn for Prefix Name.
nnn is "nnn"ame i use this string to test the naming request.
@@ -86,30 +75,49 @@ public AbstractSamplerCreator() { | |||
/** | |||
* @return int request number | |||
*/ | |||
protected static int getRequestNumber() { | |||
public int getRequestNumber() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change it to be non-static? (I am not sure, I like the static, but I would like to here your reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method need to be public not protected (same package)
This method return value from a static value but i don't see why this method need to be static.
Other methods are not static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the static comes from the fact, that the result does not depend on the instance, but rather the class.
bin/jmeter.properties
Outdated
@@ -604,8 +604,15 @@ upgrade_properties=/bin/upgrade.properties | |||
# it assumes that the user has clicked a new URL | |||
#proxy.pause=5000 | |||
|
|||
# Add numeric suffix to Sampler names (default true) | |||
# Add numeric to Sampler names (default true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some placeholder has to be used (in my opinion) instead of suffix or use number instead of numeric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove the suffix (add to the JMeter 5.2) because is not only suffix.
I don't change the comment "numeric". This comment exist for a long time.
I can change to numbering.
Have you had a chance to look at #595? |
@@ -86,30 +75,49 @@ public AbstractSamplerCreator() { | |||
/** | |||
* @return int request number | |||
*/ | |||
protected static int getRequestNumber() { | |||
public int getRequestNumber() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the static comes from the fact, that the result does not depend on the instance, but rather the class.
|
||
public int getRequestNumberFromSamplerCreator() { | ||
int iRequestNumber = 1; | ||
SamplerCreator samplerCreator = samplerCreatorFactory.getDefaultSamplerCreator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this assumption may not always be correct, as the DefaultSamplerCreator
is probably the only factory, that would really work here.
On the other hand that seems to be the same with the formatting scheme changes. Hm...
Part of a bigger PR. Contributed by Vincent Daburon (vdaburon at gmail.com) Relates to #571
Thanks for your contribution on this feature. I hope you like the implemented feature, even if not all of your code has made it into the codebase. It would be great, if you could check, whether a current nightly works for you. |
Part of a bigger PR. Contributed by Vincent Daburon (vdaburon at gmail.com) Relates to apache#571
Names of newly created Samplers by the DefaultSamplerCreator can now be styled with a freestyle format, that understands the controls for MessageFormat. Apart from the placeholders {0}, {1} and {2}, that are hard to guess, for which part they are used, more memorizable placeholders #{name}, #{path} and #{counter} can be used. The counter for the generated samplers can be set to a user specified number. Based on a patch by Vincent Daburon Bugzilla Id: 64696 Closes apache#571 Closes apache#595
Part of a bigger PR. Contributed by Vincent Daburon (vdaburon at gmail.com) Relates to apache#571
Names of newly created Samplers by the DefaultSamplerCreator can now be styled with a freestyle format, that understands the controls for MessageFormat. Apart from the placeholders {0}, {1} and {2}, that are hard to guess, for which part they are used, more memorizable placeholders #{name}, #{path} and #{counter} can be used. The counter for the generated samplers can be set to a user specified number. Based on a patch by Vincent Daburon Bugzilla Id: 64696 Closes apache#571 Closes apache#595
Description
Change GUI for HTTP(s)Test Script Recorder add choice list for sampler numbering in prefix or suffix
Add choice list in with
Prefix number (start) (Default)
Suffix number (at the end)
Without numbering
Add text field to set the next number start at 1
Add text field to set the number format (String format) String format syntax default %03d
Motivation and Context
I preferred the nummbering of the samplers like the version of JMeter 3.3 and there was no choice of selection of the suffix or prefix number
Quite often I had to leave JMeter to have a sampler numbering that starts at 1.
Now you can set the next number.
How Has This Been Tested?
Select all different possibilities for naming samplers (Prefix name, Transaction name, prefix numbering, suffix numbering, no numbering, change next value, change numbering format)
Screenshots (if appropriate):
Types of changes
Checklist: