[GitHub] [jmeter] vlsi commented on a diff in pull request #5761: Add new properties for test plan external configuration serialize, functional tearDown

2023-05-19 Thread via GitHub 


vlsi commented on code in PR #5761:
URL: https://github.com/apache/jmeter/pull/5761#discussion_r1198915348


##
src/core/src/main/java/org/apache/jmeter/testelement/TestPlan.java:
##
@@ -82,12 +85,23 @@ public void prepareForPreCompile()
 }
 
 /**
- * Fetches the functional mode property
- *
+ * Fetches the functional mode property
+ * Could be change for no-GUI test with jmeter property: {@code 
PROP_FUNCTIONAL_MODE}
  * @return functional mode
  */
 public boolean isFunctionalMode() {
-return getPropertyAsBoolean(FUNCTIONAL_MODE);
+boolean functionalModeDefault = getPropertyAsBoolean(FUNCTIONAL_MODE);
+log.debug("functionalModeDefault=" + functionalModeDefault);
+boolean functionalModeReturn = functionalModeDefault;
+if (isNonGui()) {
+String propFunctionalModeProperty = 
JMeterUtils.getProperty(PROP_FUNCTIONAL_MODE);
+if (propFunctionalModeProperty != null) {
+functionalModeReturn = 
JMeterUtils.getPropDefault(PROP_FUNCTIONAL_MODE, functionalModeDefault);
+log.info("Change with property " + PROP_FUNCTIONAL_MODE + ", 
value=" + functionalModeReturn);

Review Comment:
   If you go with `jmeter.test_plan.tearDown_on_shutdown=true` (remove ` 
value`), then it would be understandable, and the users could search for that 
exact pattern everywhere. WDYT?



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

To unsubscribe, e-mail: dev-unsubscr...@jmeter.apache.org

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

[GitHub] [jmeter] vlsi commented on a diff in pull request #5761: Add new properties for test plan external configuration serialize, functional tearDown

2023-05-03 Thread via GitHub 


vlsi commented on code in PR #5761:
URL: https://github.com/apache/jmeter/pull/5761#discussion_r1183761662


##
src/core/src/main/java/org/apache/jmeter/testelement/TestPlan.java:
##
@@ -82,12 +85,23 @@ public void prepareForPreCompile()
 }
 
 /**
- * Fetches the functional mode property
- *
+ * Fetches the functional mode property
+ * Could be change for no-GUI test with jmeter property: {@code 
PROP_FUNCTIONAL_MODE}
  * @return functional mode
  */
 public boolean isFunctionalMode() {
-return getPropertyAsBoolean(FUNCTIONAL_MODE);
+boolean functionalModeDefault = getPropertyAsBoolean(FUNCTIONAL_MODE);
+log.debug("functionalModeDefault=" + functionalModeDefault);
+boolean functionalModeReturn = functionalModeDefault;
+if (isNonGui()) {

Review Comment:
   Why the override is for `non-gui-mode` only?
   I think we should not add mode-dependent settings as it would be hard to 
tell why the behaviour is different.



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

To unsubscribe, e-mail: dev-unsubscr...@jmeter.apache.org

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

[GitHub] [jmeter] vlsi commented on a diff in pull request #5761: Add new properties for test plan external configuration serialize, functional tearDown

2023-05-03 Thread via GitHub 


vlsi commented on code in PR #5761:
URL: https://github.com/apache/jmeter/pull/5761#discussion_r1183758521


##
src/core/src/main/java/org/apache/jmeter/testelement/TestPlan.java:
##
@@ -83,11 +86,23 @@ public void prepareForPreCompile()
 
 /**
  * Fetches the functional mode property
- *

Review Comment:
   `` is wrong here.
   The sentence should end with a period. See 
https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#descriptions
   
   See
   
   >  The Javadoc tool copies this first sentence to the appropriate member, 
class/interface or package summary
   



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

To unsubscribe, e-mail: dev-unsubscr...@jmeter.apache.org

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

[GitHub] [jmeter] vlsi commented on a diff in pull request #5761: Add new properties for test plan external configuration serialize, functional tearDown

2023-04-28 Thread via GitHub 


vlsi commented on code in PR #5761:
URL: https://github.com/apache/jmeter/pull/5761#discussion_r1180333204


##
src/core/src/main/java/org/apache/jmeter/testelement/TestPlan.java:
##
@@ -82,12 +85,23 @@ public void prepareForPreCompile()
 }
 
 /**
- * Fetches the functional mode property
- *
+ * Fetches the functional mode property
+ * Could be change for no-GUI test with jmeter property: {@code 
PROP_FUNCTIONAL_MODE}
  * @return functional mode
  */
 public boolean isFunctionalMode() {
-return getPropertyAsBoolean(FUNCTIONAL_MODE);
+boolean functionalModeDefault = getPropertyAsBoolean(FUNCTIONAL_MODE);
+log.debug("functionalModeDefault=" + functionalModeDefault);
+boolean functionalModeReturn = functionalModeDefault;
+if (isNonGui()) {
+String propFunctionalModeProperty = 
JMeterUtils.getProperty(PROP_FUNCTIONAL_MODE);
+if (propFunctionalModeProperty != null) {
+functionalModeReturn = 
JMeterUtils.getPropDefault(PROP_FUNCTIONAL_MODE, functionalModeDefault);
+log.info("Change with property " + PROP_FUNCTIONAL_MODE + ", 
value=" + functionalModeReturn);

Review Comment:
   I mean the wording tells nothing to the end user. What should the end user 
do with the log message like `Change with property 
jmeter.test_plan.functional_mode, value=true`? Is it good or bad? What does it 
mean?
   
   If you think it is important to tell users that the actual property was 
adjusted, then something like `Overriding the value of "functional mode" for 
component "..." to "..." since JMeter property "..." was set to "..."`.



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

To unsubscribe, e-mail: dev-unsubscr...@jmeter.apache.org

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

[GitHub] [jmeter] vlsi commented on a diff in pull request #5761: Add new properties for test plan external configuration serialize, functional tearDown

2023-04-28 Thread via GitHub 


vlsi commented on code in PR #5761:
URL: https://github.com/apache/jmeter/pull/5761#discussion_r1180256462


##
src/core/src/main/java/org/apache/jmeter/testelement/TestPlan.java:
##
@@ -82,12 +85,23 @@ public void prepareForPreCompile()
 }
 
 /**
- * Fetches the functional mode property
- *
+ * Fetches the functional mode property
+ * Could be change for no-GUI test with jmeter property: {@code 
PROP_FUNCTIONAL_MODE}
  * @return functional mode
  */
 public boolean isFunctionalMode() {
-return getPropertyAsBoolean(FUNCTIONAL_MODE);
+boolean functionalModeDefault = getPropertyAsBoolean(FUNCTIONAL_MODE);
+log.debug("functionalModeDefault=" + functionalModeDefault);
+boolean functionalModeReturn = functionalModeDefault;
+if (isNonGui()) {
+String propFunctionalModeProperty = 
JMeterUtils.getProperty(PROP_FUNCTIONAL_MODE);
+if (propFunctionalModeProperty != null) {
+functionalModeReturn = 
JMeterUtils.getPropDefault(PROP_FUNCTIONAL_MODE, functionalModeDefault);
+log.info("Change with property " + PROP_FUNCTIONAL_MODE + ", 
value=" + functionalModeReturn);

Review Comment:
   This will be visible to the end users.  What does `Change with property` 
mean?



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

To unsubscribe, e-mail: dev-unsubscr...@jmeter.apache.org

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

[GitHub] [jmeter] vlsi commented on a diff in pull request #5761: Add new properties for test plan external configuration serialize, functional tearDown

2023-04-28 Thread via GitHub 


vlsi commented on code in PR #5761:
URL: https://github.com/apache/jmeter/pull/5761#discussion_r1180255791


##
src/core/src/main/java/org/apache/jmeter/testelement/TestPlan.java:
##
@@ -82,12 +85,23 @@ public void prepareForPreCompile()
 }
 
 /**
- * Fetches the functional mode property
- *
+ * Fetches the functional mode property
+ * Could be change for no-GUI test with jmeter property: {@code 
PROP_FUNCTIONAL_MODE}
  * @return functional mode
  */
 public boolean isFunctionalMode() {
-return getPropertyAsBoolean(FUNCTIONAL_MODE);
+boolean functionalModeDefault = getPropertyAsBoolean(FUNCTIONAL_MODE);
+log.debug("functionalModeDefault=" + functionalModeDefault);

Review Comment:
   Please either remove the logging or use patterns for log formatting
   ```suggestion
   log.debug("functionalModeDefault={}", functionalModeDefault);
   ```



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

To unsubscribe, e-mail: dev-unsubscr...@jmeter.apache.org

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