date:20181120

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

2018-11-20 Thread FSchumacher

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

2018-11-20 Thread FSchumacher

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

2018-11-20 Thread FSchumacher

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 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 pull request #432: Bug 62870 / Templates : Add ability to provide par...

2018-11-20 Thread FSchumacher

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 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 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 pull request #432: Bug 62870 / Templates : Add ability to provide par...

2018-11-20 Thread FSchumacher

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

2018-11-20 Thread FSchumacher

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

2018-11-20 Thread FSchumacher

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 @@
   
 
 
-  .*toolbar\.live\.com.*
   (?i).*\.(bmp|css|js|gif|ico|jpe?g|png|swf|eot|otf|ttf|mp4|woff|woff2)
-  update\.microsoft\.com.*
-  toolbarqueries\.google\..*
-  clients.*\.google.*
-  api\.bing\.com.*
+  www\.download\.windowsupdate\.com.*
--- 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...

2018-11-20 Thread FSchumacher

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


---

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

2018-11-20 Thread FSchumacher

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

2018-11-20 Thread FSchumacher

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

2018-11-20 Thread pmouawad

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

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter pull request #432: Bug 62870 / Templates : Add ability to provide par...

[GitHub] jmeter issue #432: Bug 62870 / Templates : Add ability to provide parameters

11 matches

Site Navigation

Mail list logo

Footer information