Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bug59945_DefaultGroovyEngineToJSR223ElementsV2 #223

Closed
wants to merge 4 commits into from
Closed

Bug59945_DefaultGroovyEngineToJSR223ElementsV2 #223

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fe20ff
Bug59945_DefaultGroovyEngineToJSR223ElementsV2
ra0077 Aug 4, 2016
8c8c4d0
Hi,
ra0077 Aug 5, 2016
3d4a165
Take account of advices
ra0077 Aug 5, 2016
69e27bd
Remove log message as discuted in PR and like "CSS/JQuery Extractor"
ra0077 Sep 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/core/org/apache/jmeter/util/JSR223TestElement.java
View file
Open in desktop
Expand Up @@ -66,6 +66,8 @@ public static ScriptEngineManager getInstance() {
}

private static final long serialVersionUID = 233L;

private static final Logger log = LoggingManager.getLoggerForClass();

/** If not empty then script in ScriptText will be compiled and cached */
private String cacheKey = "";
Expand All @@ -86,11 +88,16 @@ public JSR223TestElement() {
}

protected ScriptEngine getScriptEngine() throws ScriptException {
final String lang = getScriptLanguage();
String lang = getScriptLanguage();

if (lang.isEmpty()) {
lang = defaultScriptLanguage;
log.warn("Script language has not been chosen on the UI, the script will be interpreted as a groovy script");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really intend to warn on each and every sampler execution?
This looks awfully broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vladimir,

getScriptEngine method is called for each sampler execution and I am OK with your it's not the best solution for performance.
But you can dynamically change the script language during the execution of the script.
It's why I have made this.
If you have a better solution, please let me know
The other solution will be to don't allow to dynamically modify/change the script language during the execution. But I am not sure it's a good solution.

Antonio

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not the best solution for performance

JMeter is meant for performance testing. Why do you break the main use case?

  1. The warn message does not enable user to identify which particular sampler is bad, so it will be impossible to fix large JMeter script. That kind of message is useless.
  2. The message should be printed at most once for each sampler for each test execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first idea was to put Groovy as default script language in the GUI but this solution have a lot of impact (like all the modifications of a default value with compatibility)

The best solution will be to add a "Check script" button to verify the script and advice users of their mistakes and best practices to apply to their script (if you know how to implement it, please share it)

For the moment if the user forget to define a script language, he needs to check the errors in log (by GUI or in the file) and I think it's not user friendly

Call each time getScriptEngine method is already not a good solution for performance (need to check if the JIT don't inline it) because I am not sure that modify dynamically the script language during the execution of the script is very useful feature used by someone (but if you have a use case, please share it)

  1. I am OK with you, I can modify the warn message
  2. For the moment script language can be modified at each iteration of the test. It means that sometime the script language could be good and sometimes no. If the message is printed only once, it will be incomplete and false.
    If we don't modify the behavior of JMeter (allow to dynamically change the script language), the better solution I have is to print a better message. If you have a better solution, please let me know.

}

ScriptEngine scriptEngine = getInstance().getEngineByName(lang);
if (scriptEngine == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scriptEngine will be never null if groovy become the default language. This (if and exception below) must be changes.
Perhaps put here the test (if lang is undefine then log warn "interpret as groovy script")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Milamber,

I have keep the exception because I am not sure that getInstance().getEngineByName("groovy") will always return something.
For example if the groovy jar are missing

Antonio

throw new ScriptException("Cannot find engine named: '"+lang+"', ensure you set language field in JSR223 Test Element:"+getName());
throw new ScriptException("Cannot find engine named: '"+lang+"', ensure you set language field in JSR223 Test Element: "+getName());
}

return scriptEngine;
Expand Down
2 changes: 2 additions & 0 deletions src/core/org/apache/jmeter/util/ScriptingTestElement.java
View file
Open in desktop
Expand Up @@ -36,6 +36,8 @@ public abstract class ScriptingTestElement extends AbstractTestElement {
private String script = ""; // script (if file not provided)

protected String scriptLanguage = ""; // BSF/JSR223 language to use

protected String defaultScriptLanguage = "groovy"; // if no language is chosen in GUI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be final static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

//-- For TestBean implementations only

public ScriptingTestElement() {
Expand Down