jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [jmeter] FSchumacher commented on a change in pull request #571: Add prefix or suffix sampler numbering for HTTP Test Script Recorder
Date Sat, 30 May 2020 14:37:40 GMT

FSchumacher commented on a change in pull request #571:
URL: https://github.com/apache/jmeter/pull/571#discussion_r432845144



##########
File path: xdocs/usermanual/component_reference.xml
##########
@@ -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>

Review comment:
       Seems to be a strange name for a property. Is `proxy.heade'rs` really correct?

##########
File path: src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/gui/ProxyControlGui.java
##########
@@ -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();

Review comment:
       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.

##########
File path: xdocs/usermanual/component_reference.xml
##########
@@ -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>
         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/>

Review comment:
       Looks like a mismatch in spaces at the front. I think the `property` tags should match
up.

##########
File path: bin/jmeter.properties
##########
@@ -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)

Review comment:
       Some placeholder has to be used (in my opinion) instead of *suffix* or use *number*
instead of *numeric*.

##########
File path: src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/AbstractSamplerCreator.java
##########
@@ -86,30 +75,49 @@ public AbstractSamplerCreator() {
     /**
      * @return int request number
      */
-    protected static int getRequestNumber() {
+    public int getRequestNumber() {

Review comment:
       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.

##########
File path: xdocs/usermanual/component_reference.xml
##########
@@ -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>
         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/>
+		<ul>
+             <li><code>Without numbering</code> no numbering, ex : '/a.png'</li>

Review comment:
       *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.

##########
File path: src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/DefaultSamplerCreator.java
##########
@@ -283,32 +285,58 @@ public boolean isErrorDetected() {
      * @param sampler {@link HTTPSamplerBase}
      * @param request {@link HttpRequestHdr}
      */
-    protected void computeSamplerName(HTTPSamplerBase sampler,
-            HttpRequestHdr request) {
-        String prefix = request.getPrefix();
+    protected void computeSamplerName(HTTPSamplerBase sampler, HttpRequestHdr request) {
+        String prefix = request.getPrefix(); // ppp

Review comment:
       What is the meaning of *ppp* ?

##########
File path: src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/DefaultSamplerCreator.java
##########
@@ -283,32 +285,58 @@ public boolean isErrorDetected() {
      * @param sampler {@link HTTPSamplerBase}
      * @param request {@link HttpRequestHdr}
      */
-    protected void computeSamplerName(HTTPSamplerBase sampler,
-            HttpRequestHdr request) {
-        String prefix = request.getPrefix();
+    protected void computeSamplerName(HTTPSamplerBase sampler, HttpRequestHdr request) {
+        String prefix = request.getPrefix(); // ppp
         int httpSampleNameMode = request.getHttpSampleNameMode();
         if (!HTTPConstants.CONNECT.equals(request.getMethod()) && isNumberRequests())
{
             if(StringUtils.isNotEmpty(prefix)) {
+            	// with a prefix name

Review comment:
       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 :) )




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message