Hello sebb, Some notes inline below from an external eye.
Regards On Tue, Mar 1, 2016 at 11:08 AM, sebb <[email protected]> wrote: > On 1 March 2016 at 06:11, Philippe Mouawad <[email protected]> > wrote: > > On Monday, February 29, 2016, sebb <[email protected]> wrote: > > > >> On 29 February 2016 at 20:57, <[email protected] <javascript:;>> > wrote: > >> > Author: pmouawad > >> > Date: Mon Feb 29 20:57:32 2016 > >> > New Revision: 1732944 > >> > > >> > URL: http://svn.apache.org/viewvc?rev=1732944&view=rev > >> > Log: > >> > Bug 58986 - Report/Dashboard reuses the same output directory > >> > Bugzilla Id: 58986 > >> > > >> > Modified: > >> > jmeter/trunk/bin/jmeter.properties > >> > jmeter/trunk/src/core/org/apache/jmeter/JMeter.java > >> > > >> > jmeter/trunk/src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java > >> > jmeter/trunk/xdocs/changes.xml > >> > jmeter/trunk/xdocs/usermanual/generating-dashboard.xml > >> > > >> > Modified: jmeter/trunk/bin/jmeter.properties > >> > URL: > >> > http://svn.apache.org/viewvc/jmeter/trunk/bin/jmeter.properties?rev=1732944&r1=1732943&r2=1732944&view=diff > >> > > >> > ============================================================================== > >> > --- jmeter/trunk/bin/jmeter.properties (original) > >> > +++ jmeter/trunk/bin/jmeter.properties Mon Feb 29 20:57:32 2016 > >> > @@ -1262,6 +1262,7 @@ jmeter.reportgenerator.exporter.html.cla > >> > > >> > #jmeter.reportgenerator.exporter.html.property.template_dir=report-template > >> > > >> > # Sets the destination directory for generated html pages. > >> > +# This will override the value set through -o command line option > >> > > #jmeter.reportgenerator.exporter.html.property.output_dir=report-output > >> > > >> > # Indicates which graph series are filtered (regular expression) > >> > > >> > Modified: jmeter/trunk/src/core/org/apache/jmeter/JMeter.java > >> > URL: > >> > http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/JMeter.java?rev=1732944&r1=1732943&r2=1732944&view=diff > >> > > >> > ============================================================================== > >> > --- jmeter/trunk/src/core/org/apache/jmeter/JMeter.java (original) > >> > +++ jmeter/trunk/src/core/org/apache/jmeter/JMeter.java Mon Feb 29 > >> 20:57:32 2016 > >> > @@ -110,6 +110,9 @@ public class JMeter implements JMeterPlu > >> > public static final String HTTP_PROXY_USER = "http.proxyUser"; // > >> $NON-NLS-1$ > >> > > >> > public static final String JMETER_NON_GUI = "JMeter.NonGui"; // > >> $NON-NLS-1$ > >> > + > >> > + public static final String JMETER_REPORT_OUTPUT_DIR_PROPERTY = > >> > + "jmeter.reportgenerator.outputdir"; //$NON-NLS-1$ > >> > > >> > // If the -t flag is to "LAST", then the last loaded file (if > any) > >> is used > >> > private static final String USE_LAST_JMX = "LAST"; > >> > @@ -134,6 +137,7 @@ public class JMeter implements JMeterPlu > >> > private static final int VERSION_OPT = 'v';// $NON-NLS-1$ > >> > private static final int REPORT_GENERATING_OPT = 'g';// > $NON-NLS-1$ > >> > private static final int REPORT_AT_END_OPT = 'e';// > $NON-NLS-1$ > >> > + private static final int REPORT_OUTPUT_FOLDER_OPT = 'o';// > >> $NON-NLS-1$ > >> > > >> > private static final int SYSTEM_PROPERTY = 'D';// $NON-NLS-1$ > >> > private static final int JMETER_GLOBAL_PROP = 'G';// $NON-NLS-1$ > >> > @@ -217,7 +221,10 @@ public class JMeter implements JMeterPlu > >> > "generate report dashboard only"), > >> > new CLOptionDescriptor("reportatendofloadtests", > >> > CLOptionDescriptor.ARGUMENT_DISALLOWED, > >> REPORT_AT_END_OPT, > >> > - "generate report dashboard after load test") > >> > + "generate report dashboard after load test"), > >> > + new CLOptionDescriptor("reportoutputfolder", > >> > + CLOptionDescriptor.ARGUMENT_REQUIRED, > >> REPORT_OUTPUT_FOLDER_OPT, > >> > + "output folder for report dashboard"), > >> > }; > >> > > >> > public JMeter() { > >> > @@ -400,6 +407,31 @@ public class JMeter implements JMeterPlu > >> > startGui(testFile); > >> > startOptionalServers(); > >> > } else { > >> > + CLOption reportOutputFolderOpt = parser > >> > + > .getArgumentById(REPORT_OUTPUT_FOLDER_OPT); > >> > + if(reportOutputFolderOpt != null) { > >> > + String reportOutputFolder = > >> parser.getArgumentById(REPORT_OUTPUT_FOLDER_OPT).getArgument(); > >> > + File reportOutputFolderAsFile = new > >> File(reportOutputFolder); > >> > + // We check folder does not exist or it is > empty > >> > + if(!reportOutputFolderAsFile.exists() || > >> > + // folder exists but is empty > >> > + > (reportOutputFolderAsFile.isDirectory() > >> && reportOutputFolderAsFile.listFiles().length == 0)) { > >> > + if(!reportOutputFolderAsFile.exists()) { > >> > + // Report folder does not exist, we > >> check we can create it > >> > + > if(!reportOutputFolderAsFile.mkdirs()) { > >> > + throw new > >> IllegalArgumentException("Cannot create output report to:'" > >> > + > >> +reportOutputFolderAsFile.getAbsolutePath()+"' as I was not able to > create > >> it"); > >> > + } > >> > + } > >> > + log.info("Setting property > >> '"+JMETER_REPORT_OUTPUT_DIR_PROPERTY+"' > >> to:'"+reportOutputFolderAsFile.getAbsolutePath()+"'"); > >> > + > >> JMeterUtils.setProperty(JMETER_REPORT_OUTPUT_DIR_PROPERTY, > >> > + > >> reportOutputFolderAsFile.getAbsolutePath()); > >> > + } else { > >> > + throw new > IllegalArgumentException("Cannot > >> output report to:'" > >> > + > >> +reportOutputFolderAsFile.getAbsolutePath()+"' as it would overwrite > >> existing non empty folder"); > >> > + } > >> > + } > >> > + > >> > >> This looks really awkward. > >> The processing really belongs in the class/package that uses it. > > > > for now only html exporter uses it. > > But that may change > > > >> > >> It would be much cleaner just to set the property to the option value > >> and have the class validate it. > >> Otherwise the JMeter class needs to know too much about what's valid. > > > > > > It just does a checking on existence and content, that's it > > So why does it not do the same for the property that it overrides? > it should probably > Those checks are done in the reporter code, and should remain there. > If you do so, the checks will be made too late. So the test (or the computation for generation) will run and nothing will be written. Not sure it is better. > >> > >> Also it seems wrong to validate/create the directory if it is not > >> going to be used. > > > > > > It is a matter of opinion. > > > > It works > > It does not always work properly - the option parameter is checked > even if it is not going to be used, It will be checked if it's in command line. > so can cause unnecessary test > failures or create spurious directories > > > so as somebody wrote "you will have to live with it" > > This is new code, so changes cannot cause compatibility issues. > > > or feel free to code it. > > I will fix it. > > > > > > > >> > CLOption testReportOpt = parser > >> > .getArgumentById(REPORT_GENERATING_OPT); > >> > > >> > > >> > Modified: > >> > jmeter/trunk/src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java > >> > URL: > >> > http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java?rev=1732944&r1=1732943&r2=1732944&view=diff > >> > > >> > ============================================================================== > >> > --- > >> > jmeter/trunk/src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java > >> (original) > >> > +++ > >> > jmeter/trunk/src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java > >> Mon Feb 29 20:57:32 2016 > >> > @@ -28,6 +28,7 @@ import java.util.regex.PatternSyntaxExce > >> > import org.apache.commons.io.FileUtils; > >> > import org.apache.commons.lang3.StringUtils; > >> > import org.apache.commons.lang3.Validate; > >> > +import org.apache.jmeter.JMeter; > >> > import org.apache.jmeter.report.config.ConfigurationException; > >> > import org.apache.jmeter.report.config.ExporterConfiguration; > >> > import org.apache.jmeter.report.config.GraphConfiguration; > >> > @@ -85,7 +86,8 @@ public class HtmlTemplateExporter extend > >> > > >> > // Output directory > >> > private static final String OUTPUT_DIR = "output_dir"; > >> > - private static final File OUTPUT_DIR_DEFAULT = new > >> File("report-output"); > >> > + // Default output folder name > >> > + private static final String OUTPUT_DIR_NAME_DEFAULT = > >> "report-output"; > >> > > >> > private void addToContext(String key, Object value, DataContext > >> context) { > >> > if (value instanceof String) { > >> > @@ -347,7 +349,7 @@ public class HtmlTemplateExporter extend > >> > > >> > // Get output directory property value > >> > File outputDir = getPropertyFromConfig(exportCfg, OUTPUT_DIR, > >> > - OUTPUT_DIR_DEFAULT, File.class); > >> > + getReportOutputFolder(), File.class); > >> > LOG.info("Will generate dashboard in folder:" + outputDir); > >> > > >> > // Add the flag defining whether only sample series are > >> filtered to the > >> > @@ -469,4 +471,16 @@ public class HtmlTemplateExporter extend > >> > LOG.debug("End of template processing"); > >> > > >> > } > >> > + > >> > + /** > >> > + * @return {@link File} the folder where to output HTML report > >> > + */ > >> > + private File getReportOutputFolder() { > >> > + String globallyDefinedOutputDir = > >> JMeterUtils.getProperty(JMeter.JMETER_REPORT_OUTPUT_DIR_PROPERTY); > >> > + if(!StringUtils.isEmpty(globallyDefinedOutputDir)) { > >> > + return new File(globallyDefinedOutputDir); > >> > + } else { > >> > + return new File(JMeterUtils.getJMeterBinDir(), > >> OUTPUT_DIR_NAME_DEFAULT); > >> > + } > >> > + } > >> > } > >> > > >> > Modified: jmeter/trunk/xdocs/changes.xml > >> > URL: > >> > http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1732944&r1=1732943&r2=1732944&view=diff > >> > > >> > ============================================================================== > >> > --- jmeter/trunk/xdocs/changes.xml (original) > >> > +++ jmeter/trunk/xdocs/changes.xml Mon Feb 29 20:57:32 2016 > >> > @@ -344,6 +344,7 @@ Summary > >> > <li><bug>58931</bug>New Report/Dashboard : Getting font errors > >> under Firefox and Chrome (not Safari)</li> > >> > <li><bug>58932</bug>Report / Dashboard: Document clearly and log > >> what report are not generated when saveservice options are not correct. > >> Developed by Florent Sabbe (f dot sabbe at ubik-ingenierie.com) and > >> contributed by Ubik-Ingenierie</li> > >> > <li><bug>59055</bug>JMeter report generator : When generation is > >> not launched from jmeter/bin folder report-template is not found</li> > >> > + <li><bug>58986</bug>Report/Dashboard reuses the same output > >> directory</li> > >> > </ul> > >> > > >> > <!-- =================== Thanks =================== --> > >> > > >> > Modified: jmeter/trunk/xdocs/usermanual/generating-dashboard.xml > >> > URL: > >> > http://svn.apache.org/viewvc/jmeter/trunk/xdocs/usermanual/generating-dashboard.xml?rev=1732944&r1=1732943&r2=1732944&view=diff > >> > > >> > ============================================================================== > >> > --- jmeter/trunk/xdocs/usermanual/generating-dashboard.xml (original) > >> > +++ jmeter/trunk/xdocs/usermanual/generating-dashboard.xml Mon Feb 29 > >> 20:57:32 2016 > >> > @@ -444,6 +444,7 @@ jmeter.reportgenerator.apdex_statisfied_ > >> > jmeter.reportgenerator.apdex_tolerated_threshold=3000 > >> > > >> > # Sets the destination directory for generated html pages, it is > better > >> to change it for every generation > >> > +# This will override the value set through -o command line option > >> > # > >> > jmeter.reportgenerator.exporter.html.property.output_dir=/tmp/test-report > >> > > >> > # Indicates which graph series are filtered (regular expression) > >> > @@ -481,14 +482,14 @@ jmeter.reportgenerator.exporter.html.fil > >> > <subsection name="§-num;.3.1 Generation from an > >> existing sample log file" anchor="report_only"> > >> > <p> > >> > Use the following command : > >> > - <source>jmeter -n -g <log file> > >> -Jjmeter.reportgenerator.exporter.html.property.output_dir=<Path to > >> output folder></source> > >> > + <source>jmeter -n -g <log file> -o > >> <Path to output folder></source> > >> > </p> > >> > </subsection> > >> > > >> > <subsection name="§-num;.3.2 Generation after > load > >> test" anchor="report_after_load_test"> > >> > <p> > >> > Use the following command : > >> > - <source>jmeter -n -t <test JMX file> -e > >> -l <test log file> > >> -Jjmeter.reportgenerator.exporter.html.property.output_dir=<Path to > >> output folder></source> > >> > + <source>jmeter -n -t <test JMX file> -e > >> -l <test log file> -o <Path to output folder></source> > >> > </p> > >> > </subsection> > >> > </subsection> > >> > > >> > > >> > > > > > > -- > > Cordialement. > > Philippe Mouawad. > -- Regards Ubik Load Pack <http://ubikloadpack.com> Team Follow us on Twitter <http://twitter.com/ubikloadpack> Cordialement L'équipe Ubik Load Pack <http://ubikloadpack.com> Suivez-nous sur Twitter <http://twitter.com/ubikloadpack>
