You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by ubikloadpack <gi...@git.apache.org> on 2018/11/19 21:37:10 UTC
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
GitHub user ubikloadpack opened a pull request:
https://github.com/apache/jmeter/pull/432
Bug 62870 / Templates : Add ability to provide parameters
## Description
This is the implementation of a feature in enhancement list:
"Templates : Add ability to provide parameters:"
- https://bz.apache.org/bugzilla/show_bug.cgi?id=62870
It has been implemented by members of UbikLoadPack (https://ubikloadpack.com) team (Thomas, Florent and Philippe) and is donated to Apache JMeter project since we have signed a CCLA few years ago.
## Motivation and Context
This feature will allow providing a kind of wizard for template allowing to specify parameters when creating a new Plan:
https://bz.apache.org/bugzilla/show_bug.cgi?id=62870
## How Has This Been Tested?
The PR contains JUnit tests for part of the code.
And we have performed manual testing.
## Screenshots (if appropriate):
The feature is fully documented in the PR with screenshots as long as existing one (template).
<img width="759" alt="screen shot 2018-11-19 at 22 34 37" src="https://user-images.githubusercontent.com/14978275/48736366-58201500-ec4b-11e8-9f88-ad8b356be891.png">
## Types of changes
- New feature (non-breaking change which adds functionality)
- No Breaking change as parameters are optional
## Checklist:
- [X] My code follows the [code style][style-guide] of this project.
- [X] I have updated the documentation accordingly.
[style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ubikloadpack/jmeter BUG_62870
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/jmeter/pull/432.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #432
----
commit 58d37f82c7fa92f50fd7df8e6ecbb8c38d9c7934
Author: pmouawad <p....@...>
Date: 2018-11-19T21:26:20Z
Bug 62870 / Templates : Add ability to provide parameters
https://bz.apache.org/bugzilla/show_bug.cgi?id=62870
Contributed by https://ubikloadpack.com
----
---
[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/jmeter/pull/432
# [Codecov](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=h1) Report
> Merging [#432](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/jmeter/commit/cc55d68116667087e75b88a270c66cecb4d7e916?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `28.43%`.
[](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## trunk #432 +/- ##
===========================================
- Coverage 58.6% 58.57% -0.04%
- Complexity 10638 10667 +29
===========================================
Files 1196 1198 +2
Lines 76087 76331 +244
Branches 7361 7399 +38
===========================================
+ Hits 44594 44712 +118
- Misses 28986 29106 +120
- Partials 2507 2513 +6
```
| [Impacted Files](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pache/jmeter/gui/action/SelectTemplatesDialog.java](https://codecov.io/gh/apache/jmeter/pull/432/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvZ3VpL2FjdGlvbi9TZWxlY3RUZW1wbGF0ZXNEaWFsb2cuamF2YQ==) | `1.58% <0%> (-1.42%)` | `1 <0> (ø)` | |
| [src/core/org/apache/jmeter/util/TemplateUtil.java](https://codecov.io/gh/apache/jmeter/pull/432/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvdXRpbC9UZW1wbGF0ZVV0aWwuamF2YQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
| [...rg/apache/jmeter/gui/action/template/Template.java](https://codecov.io/gh/apache/jmeter/pull/432/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvZ3VpL2FjdGlvbi90ZW1wbGF0ZS9UZW1wbGF0ZS5qYXZh) | `20.93% <4.28%> (+20.93%)` | `12 <2> (+12)` | :arrow_up: |
| [...he/jmeter/gui/action/template/TemplateManager.java](https://codecov.io/gh/apache/jmeter/pull/432/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvZ3VpL2FjdGlvbi90ZW1wbGF0ZS9UZW1wbGF0ZU1hbmFnZXIuamF2YQ==) | `74.41% <84.21%> (+74.41%)` | `13 <10> (+13)` | :arrow_up: |
| [...meter/gui/action/template/TestTemplateManager.java](https://codecov.io/gh/apache/jmeter/pull/432/diff?src=pr&el=tree#diff-dGVzdC9zcmMvb3JnL2FwYWNoZS9qbWV0ZXIvZ3VpL2FjdGlvbi90ZW1wbGF0ZS9UZXN0VGVtcGxhdGVNYW5hZ2VyLmphdmE=) | `87.8% <87.8%> (ø)` | `4 <4> (?)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=footer). Last update [cc55d68...db95633](https://codecov.io/gh/apache/jmeter/pull/432?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/432
Hi Felix,
You’re right.
Take your time
Sorry, I am always impatient guy :)
Regards
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235114098
--- Diff: bin/templates/recording.jmx ---
@@ -90,32 +101,32 @@
<ProxyControl guiclass="ProxyControlGui" testclass="ProxyControl" testname="HTTP(S) Test Script Recorder" enabled="false">
<stringProp name="ProxyControlGui.port">8888</stringProp>
<collectionProp name="ProxyControlGui.exclude_list">
- <stringProp name="1301401588">.*toolbar\.live\.com.*</stringProp>
<stringProp name="1179605444">(?i).*\.(bmp|css|js|gif|ico|jpe?g|png|swf|eot|otf|ttf|mp4|woff|woff2)</stringProp>
- <stringProp name="1276958334">update\.microsoft\.com.*</stringProp>
- <stringProp name="195066122">toolbarqueries\.google\..*</stringProp>
- <stringProp name="-1570593883">clients.*\.google.*</stringProp>
- <stringProp name="339269285">api\.bing\.com.*</stringProp>
+ <stringProp name="-88591710">www\.download\.windowsupdate\.com.*</stringProp>
--- End diff --
I don't think these changes have anything to do with the feature, would you like to commit these in an extra step?
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r236800696
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -137,67 +160,138 @@ private void checkDirtyAndLoad(final ActionEvent actionEvent)
if (template == null) {
return;
}
+ templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames()); // reload the templates before loading
+
final boolean isTestPlan = template.isTestPlan();
// Check if the user wants to drop any changes
- if (isTestPlan) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.CHECK_DIRTY));
- GuiPackage guiPackage = GuiPackage.getInstance();
- if (guiPackage.isDirty()) {
- // Check if the user wants to create from template
- int response = JOptionPane.showConfirmDialog(GuiPackage.getInstance().getMainFrame(),
- JMeterUtils.getResString("cancel_new_from_template"), // $NON-NLS-1$
- JMeterUtils.getResString("template_load?"), // $NON-NLS-1$
- JOptionPane.YES_NO_CANCEL_OPTION,
- JOptionPane.QUESTION_MESSAGE);
- if(response == JOptionPane.YES_OPTION) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.SAVE));
- }
- if (response == JOptionPane.CLOSED_OPTION || response == JOptionPane.CANCEL_OPTION) {
- return; // Don't clear the plan
- }
- }
+ if (isTestPlan && !checkDirty(actionEvent)) {
+ return;
}
ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.STOP_THREAD));
final File parent = template.getParent();
- final File fileToCopy = parent != null
+ File fileToCopy = parent != null
? new File(parent, template.getFileName())
- : new File(JMeterUtils.getJMeterHome(), template.getFileName());
- Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
- this.setVisible(false);
+ : new File(JMeterUtils.getJMeterHome(), template.getFileName());
+ replaceTemplateParametersAndLoad(actionEvent, template, isTestPlan, fileToCopy);
+ }
+
+ /**
+ * @param actionEvent {@link ActionEvent}
+ * @param template {@link Template} definition
+ * @param isTestPlan If it's a full test plan or a part
+ * @param templateFile Template file to load
+ */
+ void replaceTemplateParametersAndLoad(final ActionEvent actionEvent, final Template template,
+ final boolean isTestPlan, File templateFile) {
+ File temporaryGeneratedFile = null;
+ try {
+ // handle customized templates (the .jmx.fmkr files)
+ if (template.getParameters() != null && !template.getParameters().isEmpty()) {
+ File jmxFile = new File(templateFile.getAbsolutePath());
+ Map<String, String> userParameters = getUserParameters();
+ Configuration templateCfg = TemplateUtil.getTemplateConfig();
+ try {
+ temporaryGeneratedFile = File.createTempFile(template.getName(), ".output");
+ templateFile = temporaryGeneratedFile;
+ TemplateUtil.processTemplate(jmxFile, temporaryGeneratedFile, templateCfg, userParameters);
+ } catch (IOException | TemplateException ex) {
+ log.error("Error generating output file {} from template {}", temporaryGeneratedFile, jmxFile, ex);
+ return;
+ }
+ }
+ Load.loadProjectFile(actionEvent, templateFile, !isTestPlan, false);
+ this.dispose();
+ } finally {
+ if (temporaryGeneratedFile != null && !temporaryGeneratedFile.delete()) {
+ log.warn("Could not delete generated output file {} from template {}", temporaryGeneratedFile, templateFile);
+ }
+ }
+ }
+
+ /**
+ * @param actionEvent {@link ActionEvent}
+ * @return true if plan is not dirty or has been saved
+ */
+ boolean checkDirty(final ActionEvent actionEvent) {
--- End diff --
Any reason for not using `private`?
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235125307
--- Diff: src/core/org/apache/jmeter/gui/action/template/TemplateManager.java ---
@@ -149,19 +109,107 @@ public TemplateManager reset() {
} else {
if (log.isWarnEnabled()) {
log.warn("Ignoring template file:'{}' as it does not exist or is not readable",
- f.getAbsolutePath());
+ file.getAbsolutePath());
}
}
} catch(Exception ex) {
if (log.isWarnEnabled()) {
- log.warn("Ignoring template file:'{}', an error occurred parsing the file", f.getAbsolutePath(),
+ log.warn("Ignoring template file:'{}', an error occurred parsing the file", file.getAbsolutePath(),
ex);
}
}
}
}
return temps;
}
+
+ public final class LoggingErrorHandler implements ErrorHandler {
+ private Logger logger;
+
+ public LoggingErrorHandler(Logger logger) {
+ this.logger = logger;
+ }
+ @Override
+ public void error(SAXParseException ex) throws SAXException {
+ throw ex;
+ }
+
+ @Override
+ public void fatalError(SAXParseException ex) throws SAXException {
+ throw ex;
+ }
+
+ @Override
+ public void warning(SAXParseException ex) throws SAXException {
+ logger.warn("Warning", ex);
+ }
+ }
+
+ public static class DefaultEntityResolver implements EntityResolver {
+ public DefaultEntityResolver() {
+ super();
+ }
+
+ public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException {
+ if(systemId.endsWith("templates.dtd")) {
+ return new InputSource(TemplateManager.class.getResourceAsStream("/org/apache/jmeter/gui/action/template/templates.dtd"));
+ } else {
+ return null;
+ }
+ }
+ }
+
+ public Map<String, Template> parseTemplateFile(File file) throws IOException, SAXException, ParserConfigurationException{
+ DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+ dbf.setValidating(true);
+ dbf.setNamespaceAware(true);
+ dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+ DocumentBuilder bd = dbf.newDocumentBuilder();
+ bd.setEntityResolver(new DefaultEntityResolver());
+ LoggingErrorHandler errorHandler = new LoggingErrorHandler(log);
--- End diff --
We could hand the filename to the error handler, so that it could be used in the error messages.
---
[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/432
Hello,
I'll be merging it today.
Regards
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235121173
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -207,19 +290,35 @@ private void init() { // WARNING: called from ctor so must not be overridden (i.
public void actionPerformed(ActionEvent e) {
final Object source = e.getSource();
if (source == cancelButton) {
- this.setVisible(false);
- return;
+ resetJDialog();
+ this.dispose();
} else if (source == applyTemplateButton) {
- checkDirtyAndLoad(e);
- } else if (source == reloadTemplateButton) {
- templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames());
+ String selectedTemplate = templateList.getText();
+ Template template = TemplateManager.getInstance().getTemplateByName(selectedTemplate);
+ if(template.getParameters() != null && !template.getParameters().isEmpty()) {
--- End diff --
I find negations harder to read. I would prefer to use `params == null || params.isEmpty()` and inverse the code blocks and/or introduce a private method `isEmpty(template.getParameters)`.
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235128465
--- Diff: src/core/org/apache/jmeter/util/TemplateUtil.java ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.jmeter.util;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStreamWriter;
+import java.io.Writer;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+
+import freemarker.template.Configuration;
+import freemarker.template.TemplateException;
+import freemarker.template.TemplateExceptionHandler;
+
+/**
+ * Class used to process freemarkers templates
+ * @since 5.1
+ */
+public final class TemplateUtil {
+
+ private static Configuration templateConfiguration = init();
+
+ private TemplateUtil() {
+ super();
+ }
+
+ private static Configuration init() {
+ Configuration templateConfiguration = new Configuration(Configuration.getVersion());
+ templateConfiguration.setDefaultEncoding(StandardCharsets.UTF_8.name());
+ templateConfiguration.setInterpolationSyntax(Configuration.SQUARE_BRACKET_INTERPOLATION_SYNTAX);
+ templateConfiguration.setTemplateExceptionHandler(TemplateExceptionHandler.RETHROW_HANDLER);
+ return templateConfiguration;
+ }
+
+ /**
+ * Give a basic templateConfiguration
+ * @return a Configuration
+ */
+ public static Configuration getTemplateConfig() {
+ return templateConfiguration;
+ }
+
+ /**
+ * Process a given freemarker template and put its result in a new folder.
+ *
+ * @param template : file that contains the freemarker template to process
--- End diff --
Is the colon needed here? What does the rendered javadoc look like?
---
[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/432
Hello Felix,
I have committed the fixes following your review.
Let me know if you will commit the PR (which I would prefer), otherwise I'll do it.
Thanks
Regards
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235118443
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -137,67 +160,127 @@ private void checkDirtyAndLoad(final ActionEvent actionEvent)
if (template == null) {
return;
}
+ templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames()); // reload the templates before loading
+
final boolean isTestPlan = template.isTestPlan();
// Check if the user wants to drop any changes
- if (isTestPlan) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.CHECK_DIRTY));
- GuiPackage guiPackage = GuiPackage.getInstance();
- if (guiPackage.isDirty()) {
- // Check if the user wants to create from template
- int response = JOptionPane.showConfirmDialog(GuiPackage.getInstance().getMainFrame(),
- JMeterUtils.getResString("cancel_new_from_template"), // $NON-NLS-1$
- JMeterUtils.getResString("template_load?"), // $NON-NLS-1$
- JOptionPane.YES_NO_CANCEL_OPTION,
- JOptionPane.QUESTION_MESSAGE);
- if(response == JOptionPane.YES_OPTION) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.SAVE));
- }
- if (response == JOptionPane.CLOSED_OPTION || response == JOptionPane.CANCEL_OPTION) {
- return; // Don't clear the plan
- }
- }
+ if (isTestPlan && !checkDirty(actionEvent)) {
+ return;
}
ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.STOP_THREAD));
final File parent = template.getParent();
- final File fileToCopy = parent != null
+ File fileToCopy = parent != null
? new File(parent, template.getFileName())
- : new File(JMeterUtils.getJMeterHome(), template.getFileName());
- Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
- this.setVisible(false);
+ : new File(JMeterUtils.getJMeterHome(), template.getFileName());
+ File temporaryGeneratedFile = null;
+ try {
+ // handle customized templates (the .jmx.fmkr files)
+ if (template.getParameters() != null && !template.getParameters().isEmpty()) {
+ File jmxFile = new File(fileToCopy.getAbsolutePath());
+ Map<String, String> userParameters = getUserParameters();
+ Configuration templateCfg = TemplateUtil.getTemplateConfig();
+ try {
+ temporaryGeneratedFile = File.createTempFile(template.getName(), ".output");
+ fileToCopy = temporaryGeneratedFile;
+ TemplateUtil.processTemplate(jmxFile, temporaryGeneratedFile, templateCfg, userParameters);
+ } catch (IOException | TemplateException ex) {
+ log.error("Error generating output file {} from template {}", temporaryGeneratedFile, jmxFile, ex);
+ return;
+ }
+ }
+ Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
+ this.dispose();
+ } finally {
+ if(temporaryGeneratedFile != null && !temporaryGeneratedFile.delete()) {
+ log.warn("Could not delete generated output file {} from template {}", temporaryGeneratedFile, fileToCopy);
+ }
+ }
+ }
+
+ /**
+ * @param actionEvent
+ * @return true if we can continue
+ */
+ boolean checkDirty(final ActionEvent actionEvent) {
+ ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.CHECK_DIRTY));
+ GuiPackage guiPackage = GuiPackage.getInstance();
+ if (guiPackage.isDirty()) {
+ // Check if the user wants to create from template
+ int response = JOptionPane.showConfirmDialog(GuiPackage.getInstance().getMainFrame(),
+ JMeterUtils.getResString("cancel_new_from_template"), // $NON-NLS-1$
+ JMeterUtils.getResString("template_load?"), // $NON-NLS-1$
+ JOptionPane.YES_NO_CANCEL_OPTION,
+ JOptionPane.QUESTION_MESSAGE);
+ if(response == JOptionPane.YES_OPTION) {
+ ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.SAVE));
+ return true;
+ }
+ if (response == JOptionPane.CLOSED_OPTION || response == JOptionPane.CANCEL_OPTION) {
+ return false; // Don't clear the plan
+ }
+ }
+ return true;
+ }
+
+ private Map<String, String> getUserParameters(){
+ Map<String, String> userParameters = new LinkedHashMap<>();
+ for(Entry<String, JLabeledTextField> entry : parametersTextFields.entrySet()) {
--- End diff --
`for` is not a function, so - for me - there is a space missing in between `for` and the parentheses.
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r236801357
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -137,67 +160,138 @@ private void checkDirtyAndLoad(final ActionEvent actionEvent)
if (template == null) {
return;
}
+ templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames()); // reload the templates before loading
+
final boolean isTestPlan = template.isTestPlan();
// Check if the user wants to drop any changes
- if (isTestPlan) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.CHECK_DIRTY));
- GuiPackage guiPackage = GuiPackage.getInstance();
- if (guiPackage.isDirty()) {
- // Check if the user wants to create from template
- int response = JOptionPane.showConfirmDialog(GuiPackage.getInstance().getMainFrame(),
- JMeterUtils.getResString("cancel_new_from_template"), // $NON-NLS-1$
- JMeterUtils.getResString("template_load?"), // $NON-NLS-1$
- JOptionPane.YES_NO_CANCEL_OPTION,
- JOptionPane.QUESTION_MESSAGE);
- if(response == JOptionPane.YES_OPTION) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.SAVE));
- }
- if (response == JOptionPane.CLOSED_OPTION || response == JOptionPane.CANCEL_OPTION) {
- return; // Don't clear the plan
- }
- }
+ if (isTestPlan && !checkDirty(actionEvent)) {
+ return;
}
ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.STOP_THREAD));
final File parent = template.getParent();
- final File fileToCopy = parent != null
+ File fileToCopy = parent != null
? new File(parent, template.getFileName())
- : new File(JMeterUtils.getJMeterHome(), template.getFileName());
- Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
- this.setVisible(false);
+ : new File(JMeterUtils.getJMeterHome(), template.getFileName());
+ replaceTemplateParametersAndLoad(actionEvent, template, isTestPlan, fileToCopy);
+ }
+
+ /**
+ * @param actionEvent {@link ActionEvent}
+ * @param template {@link Template} definition
+ * @param isTestPlan If it's a full test plan or a part
+ * @param templateFile Template file to load
+ */
+ void replaceTemplateParametersAndLoad(final ActionEvent actionEvent, final Template template,
+ final boolean isTestPlan, File templateFile) {
+ File temporaryGeneratedFile = null;
+ try {
+ // handle customized templates (the .jmx.fmkr files)
+ if (template.getParameters() != null && !template.getParameters().isEmpty()) {
+ File jmxFile = new File(templateFile.getAbsolutePath());
+ Map<String, String> userParameters = getUserParameters();
+ Configuration templateCfg = TemplateUtil.getTemplateConfig();
+ try {
+ temporaryGeneratedFile = File.createTempFile(template.getName(), ".output");
+ templateFile = temporaryGeneratedFile;
+ TemplateUtil.processTemplate(jmxFile, temporaryGeneratedFile, templateCfg, userParameters);
+ } catch (IOException | TemplateException ex) {
+ log.error("Error generating output file {} from template {}", temporaryGeneratedFile, jmxFile, ex);
+ return;
+ }
+ }
+ Load.loadProjectFile(actionEvent, templateFile, !isTestPlan, false);
+ this.dispose();
+ } finally {
+ if (temporaryGeneratedFile != null && !temporaryGeneratedFile.delete()) {
+ log.warn("Could not delete generated output file {} from template {}", temporaryGeneratedFile, templateFile);
+ }
+ }
+ }
+
+ /**
+ * @param actionEvent {@link ActionEvent}
+ * @return true if plan is not dirty or has been saved
--- End diff --
Somehow I think a method `checkDirty` would return `true` if plan is *dirty* and has not been saved. Can you think of a better name?
---
[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on the issue:
https://github.com/apache/jmeter/pull/432
This seems to be quite a big patch. Do you think two days are enough to have a good look at it?
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235127891
--- Diff: src/core/org/apache/jmeter/resources/messages.properties ---
@@ -1206,6 +1206,7 @@ teardown_on_shutdown=Run tearDown Thread Groups after shutdown of main threads
template_choose=Select Template
template_create_from=Create
template_field=Template ($i$ where i is capturing group number, starts at 1):
+template_fill_parameters=Fill your parameters \:
--- End diff --
In English typography there is no space before the colon :)
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235125815
--- Diff: src/core/org/apache/jmeter/gui/action/template/TemplateManager.java ---
@@ -149,19 +109,107 @@ public TemplateManager reset() {
} else {
if (log.isWarnEnabled()) {
log.warn("Ignoring template file:'{}' as it does not exist or is not readable",
- f.getAbsolutePath());
+ file.getAbsolutePath());
}
}
} catch(Exception ex) {
if (log.isWarnEnabled()) {
- log.warn("Ignoring template file:'{}', an error occurred parsing the file", f.getAbsolutePath(),
+ log.warn("Ignoring template file:'{}', an error occurred parsing the file", file.getAbsolutePath(),
ex);
}
}
}
}
return temps;
}
+
+ public final class LoggingErrorHandler implements ErrorHandler {
+ private Logger logger;
+
+ public LoggingErrorHandler(Logger logger) {
+ this.logger = logger;
+ }
+ @Override
+ public void error(SAXParseException ex) throws SAXException {
+ throw ex;
+ }
+
+ @Override
+ public void fatalError(SAXParseException ex) throws SAXException {
+ throw ex;
+ }
+
+ @Override
+ public void warning(SAXParseException ex) throws SAXException {
+ logger.warn("Warning", ex);
+ }
+ }
+
+ public static class DefaultEntityResolver implements EntityResolver {
+ public DefaultEntityResolver() {
+ super();
+ }
+
+ public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException {
+ if(systemId.endsWith("templates.dtd")) {
+ return new InputSource(TemplateManager.class.getResourceAsStream("/org/apache/jmeter/gui/action/template/templates.dtd"));
+ } else {
+ return null;
+ }
+ }
+ }
+
+ public Map<String, Template> parseTemplateFile(File file) throws IOException, SAXException, ParserConfigurationException{
+ DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+ dbf.setValidating(true);
+ dbf.setNamespaceAware(true);
+ dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+ DocumentBuilder bd = dbf.newDocumentBuilder();
+ bd.setEntityResolver(new DefaultEntityResolver());
+ LoggingErrorHandler errorHandler = new LoggingErrorHandler(log);
+ bd.setErrorHandler(errorHandler);
+ Document document = bd.parse(file.getAbsolutePath());
+ document.getDocumentElement().normalize();
+ Map<String, Template> templates = new TreeMap<>();
+ NodeList templateNodes = document.getElementsByTagName("template");
+ for (int i = 0; i < templateNodes.getLength(); i++) {
+ Node node = templateNodes.item(i);
+ parseTemplateNode(templates, node);
+ }
+ return templates;
+ }
+
+ /**
+ * @param templates
--- End diff --
either fill in the javadoc, or leave it out completely.
---
[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/432
Hello Team,
The PR looks good to me, tested on Mac OSX.
Anybody willing to commit it ?
I can take in charge , but I would prefer somebody else reviewing it.
If no feedback by tomorrow evening, I'll commit it.
Thanks
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235119001
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -207,19 +290,35 @@ private void init() { // WARNING: called from ctor so must not be overridden (i.
public void actionPerformed(ActionEvent e) {
final Object source = e.getSource();
if (source == cancelButton) {
- this.setVisible(false);
- return;
+ resetJDialog();
+ this.dispose();
} else if (source == applyTemplateButton) {
- checkDirtyAndLoad(e);
- } else if (source == reloadTemplateButton) {
- templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames());
+ String selectedTemplate = templateList.getText();
+ Template template = TemplateManager.getInstance().getTemplateByName(selectedTemplate);
+ if(template.getParameters() != null && !template.getParameters().isEmpty()) {
+ this.setContentPane(configureParametersPanel(template.getParameters()));
+ this.revalidate();
+ }else {
+ checkDirtyAndLoad(e);
+ }
+ } else if (source == reloadTemplateButton || source == previous) {
+ resetJDialog();
+ } else if(source == validateButton) {
--- End diff --
Same as with `for`. Spacepolice is calling :)
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235117129
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -137,67 +160,127 @@ private void checkDirtyAndLoad(final ActionEvent actionEvent)
if (template == null) {
return;
}
+ templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames()); // reload the templates before loading
+
final boolean isTestPlan = template.isTestPlan();
// Check if the user wants to drop any changes
- if (isTestPlan) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.CHECK_DIRTY));
- GuiPackage guiPackage = GuiPackage.getInstance();
- if (guiPackage.isDirty()) {
- // Check if the user wants to create from template
- int response = JOptionPane.showConfirmDialog(GuiPackage.getInstance().getMainFrame(),
- JMeterUtils.getResString("cancel_new_from_template"), // $NON-NLS-1$
- JMeterUtils.getResString("template_load?"), // $NON-NLS-1$
- JOptionPane.YES_NO_CANCEL_OPTION,
- JOptionPane.QUESTION_MESSAGE);
- if(response == JOptionPane.YES_OPTION) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.SAVE));
- }
- if (response == JOptionPane.CLOSED_OPTION || response == JOptionPane.CANCEL_OPTION) {
- return; // Don't clear the plan
- }
- }
+ if (isTestPlan && !checkDirty(actionEvent)) {
+ return;
}
ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.STOP_THREAD));
final File parent = template.getParent();
- final File fileToCopy = parent != null
+ File fileToCopy = parent != null
? new File(parent, template.getFileName())
- : new File(JMeterUtils.getJMeterHome(), template.getFileName());
- Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
- this.setVisible(false);
+ : new File(JMeterUtils.getJMeterHome(), template.getFileName());
+ File temporaryGeneratedFile = null;
--- End diff --
Could this and the following try block be extracted into a method? That would enable us to show the intent of the code block by using the method name.
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235127351
--- Diff: src/core/org/apache/jmeter/gui/action/template/TemplateManager.java ---
@@ -149,19 +109,107 @@ public TemplateManager reset() {
} else {
if (log.isWarnEnabled()) {
log.warn("Ignoring template file:'{}' as it does not exist or is not readable",
- f.getAbsolutePath());
+ file.getAbsolutePath());
}
}
} catch(Exception ex) {
if (log.isWarnEnabled()) {
- log.warn("Ignoring template file:'{}', an error occurred parsing the file", f.getAbsolutePath(),
+ log.warn("Ignoring template file:'{}', an error occurred parsing the file", file.getAbsolutePath(),
ex);
}
}
}
}
return temps;
}
+
+ public final class LoggingErrorHandler implements ErrorHandler {
+ private Logger logger;
+
+ public LoggingErrorHandler(Logger logger) {
+ this.logger = logger;
+ }
+ @Override
+ public void error(SAXParseException ex) throws SAXException {
+ throw ex;
+ }
+
+ @Override
+ public void fatalError(SAXParseException ex) throws SAXException {
+ throw ex;
+ }
+
+ @Override
+ public void warning(SAXParseException ex) throws SAXException {
+ logger.warn("Warning", ex);
+ }
+ }
+
+ public static class DefaultEntityResolver implements EntityResolver {
+ public DefaultEntityResolver() {
+ super();
+ }
+
+ public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException {
+ if(systemId.endsWith("templates.dtd")) {
+ return new InputSource(TemplateManager.class.getResourceAsStream("/org/apache/jmeter/gui/action/template/templates.dtd"));
+ } else {
+ return null;
+ }
+ }
+ }
+
+ public Map<String, Template> parseTemplateFile(File file) throws IOException, SAXException, ParserConfigurationException{
+ DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+ dbf.setValidating(true);
+ dbf.setNamespaceAware(true);
+ dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+ DocumentBuilder bd = dbf.newDocumentBuilder();
+ bd.setEntityResolver(new DefaultEntityResolver());
+ LoggingErrorHandler errorHandler = new LoggingErrorHandler(log);
+ bd.setErrorHandler(errorHandler);
+ Document document = bd.parse(file.getAbsolutePath());
+ document.getDocumentElement().normalize();
+ Map<String, Template> templates = new TreeMap<>();
+ NodeList templateNodes = document.getElementsByTagName("template");
+ for (int i = 0; i < templateNodes.getLength(); i++) {
+ Node node = templateNodes.item(i);
+ parseTemplateNode(templates, node);
+ }
+ return templates;
+ }
+
+ /**
+ * @param templates
+ * @param templateNode
+ */
+ void parseTemplateNode(Map<String, Template> templates, Node templateNode) {
+ if (templateNode.getNodeType() == Node.ELEMENT_NODE) {
+ Template template = new Template();
+ Element element = (Element) templateNode;
+ template.setTestPlan("true".equals(element.getAttribute("isTestPlan")));
+ template.setName(element.getElementsByTagName("name").item(0).getTextContent());
--- End diff --
maybe introduce a helper method, that extracts the text content for the first element found:
`template.setName(textOfFirstTag(element, "name"))`
This seems to be repeated three times.
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/jmeter/pull/432
---
[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...
Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/432#discussion_r235117924
--- Diff: src/core/org/apache/jmeter/gui/action/SelectTemplatesDialog.java ---
@@ -137,67 +160,127 @@ private void checkDirtyAndLoad(final ActionEvent actionEvent)
if (template == null) {
return;
}
+ templateList.setValues(TemplateManager.getInstance().reset().getTemplateNames()); // reload the templates before loading
+
final boolean isTestPlan = template.isTestPlan();
// Check if the user wants to drop any changes
- if (isTestPlan) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.CHECK_DIRTY));
- GuiPackage guiPackage = GuiPackage.getInstance();
- if (guiPackage.isDirty()) {
- // Check if the user wants to create from template
- int response = JOptionPane.showConfirmDialog(GuiPackage.getInstance().getMainFrame(),
- JMeterUtils.getResString("cancel_new_from_template"), // $NON-NLS-1$
- JMeterUtils.getResString("template_load?"), // $NON-NLS-1$
- JOptionPane.YES_NO_CANCEL_OPTION,
- JOptionPane.QUESTION_MESSAGE);
- if(response == JOptionPane.YES_OPTION) {
- ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.SAVE));
- }
- if (response == JOptionPane.CLOSED_OPTION || response == JOptionPane.CANCEL_OPTION) {
- return; // Don't clear the plan
- }
- }
+ if (isTestPlan && !checkDirty(actionEvent)) {
+ return;
}
ActionRouter.getInstance().doActionNow(new ActionEvent(actionEvent.getSource(), actionEvent.getID(), ActionNames.STOP_THREAD));
final File parent = template.getParent();
- final File fileToCopy = parent != null
+ File fileToCopy = parent != null
? new File(parent, template.getFileName())
- : new File(JMeterUtils.getJMeterHome(), template.getFileName());
- Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
- this.setVisible(false);
+ : new File(JMeterUtils.getJMeterHome(), template.getFileName());
+ File temporaryGeneratedFile = null;
+ try {
+ // handle customized templates (the .jmx.fmkr files)
+ if (template.getParameters() != null && !template.getParameters().isEmpty()) {
+ File jmxFile = new File(fileToCopy.getAbsolutePath());
+ Map<String, String> userParameters = getUserParameters();
+ Configuration templateCfg = TemplateUtil.getTemplateConfig();
+ try {
+ temporaryGeneratedFile = File.createTempFile(template.getName(), ".output");
+ fileToCopy = temporaryGeneratedFile;
+ TemplateUtil.processTemplate(jmxFile, temporaryGeneratedFile, templateCfg, userParameters);
+ } catch (IOException | TemplateException ex) {
+ log.error("Error generating output file {} from template {}", temporaryGeneratedFile, jmxFile, ex);
+ return;
+ }
+ }
+ Load.loadProjectFile(actionEvent, fileToCopy, !isTestPlan, false);
+ this.dispose();
+ } finally {
+ if(temporaryGeneratedFile != null && !temporaryGeneratedFile.delete()) {
+ log.warn("Could not delete generated output file {} from template {}", temporaryGeneratedFile, fileToCopy);
+ }
+ }
+ }
+
+ /**
+ * @param actionEvent
+ * @return true if we can continue
--- End diff --
This comment seems odd to me. If it returns `true` I would think, that we found the state to be `dirty`. Why would I want to continue in such a state?
---