You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by Philippe Mouawad <ph...@gmail.com> on 2016/03/01 07:11:58 UTC

Re: svn commit: r1732944 - in /jmeter/trunk: bin/jmeter.properties src/core/org/apache/jmeter/JMeter.java src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java xdocs/changes.xml xdocs/usermanual/generating-dashboard.xml

On Monday, February 29, 2016, sebb <se...@gmail.com> wrote:

> On 29 February 2016 at 20:57,  <pmouawad@apache.org <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

>
> 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 so as somebody wrote "you will have to live with it" or feel free
to code 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="&sect-num;.3.1 Generation from an
> existing sample log file" anchor="report_only">
> >                      <p>
> >                          Use the following command :
> > -                        <source>jmeter -n -g &lt;log file&gt;
> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
> output folder&gt;</source>
> > +                        <source>jmeter -n -g &lt;log file&gt; -o
> &lt;Path to output folder&gt;</source>
> >                      </p>
> >                  </subsection>
> >
> >                  <subsection name="&sect-num;.3.2 Generation after load
> test" anchor="report_after_load_test">
> >                      <p>
> >                          Use the following command :
> > -                        <source>jmeter -n -t &lt;test JMX file&gt; -e
> -l &lt;test log file&gt;
> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
> output folder&gt;</source>
> > +                        <source>jmeter -n -t &lt;test JMX file&gt; -e
> -l &lt;test log file&gt; -o &lt;Path to output folder&gt;</source>
> >                      </p>
> >                  </subsection>
> >              </subsection>
> >
> >
>


-- 
Cordialement.
Philippe Mouawad.

Re: svn commit: r1732944 - in /jmeter/trunk: bin/jmeter.properties src/core/org/apache/jmeter/JMeter.java src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java xdocs/changes.xml xdocs/usermanual/generating-dashboard.xml

Posted by sebb <se...@gmail.com>.
On 1 March 2016 at 10:14, UBIK LOAD PACK Support
<su...@ubikloadpack.com> wrote:
> Hello sebb,
>
> Some notes inline below  from an external eye.
>
> Regards
>
> On Tue, Mar 1, 2016 at 11:08 AM, sebb <se...@gmail.com> wrote:
>
>> On 1 March 2016 at 06:11, Philippe Mouawad <ph...@gmail.com>
>> wrote:
>> > On Monday, February 29, 2016, sebb <se...@gmail.com> wrote:
>> >
>> >> On 29 February 2016 at 20:57,  <pmouawad@apache.org <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

And should it also check all the other property names that are used in
a test plan that might cause a late failure?

The JMeter class should not need to know about how properties are
used, apart from the few that it needs to start the test.

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

That's a good point, but there are other ways to solve the problem
that don't require the JMeter class to know anything about the use of
the parameter.
e.g. check when the class is created.

Also the report can still be run independently, so nothing much is lost.

>> >>
>> >> 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="&sect-num;.3.1 Generation from an
>> >> existing sample log file" anchor="report_only">
>> >> >                      <p>
>> >> >                          Use the following command :
>> >> > -                        <source>jmeter -n -g &lt;log file&gt;
>> >> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
>> >> output folder&gt;</source>
>> >> > +                        <source>jmeter -n -g &lt;log file&gt; -o
>> >> &lt;Path to output folder&gt;</source>
>> >> >                      </p>
>> >> >                  </subsection>
>> >> >
>> >> >                  <subsection name="&sect-num;.3.2 Generation after
>> load
>> >> test" anchor="report_after_load_test">
>> >> >                      <p>
>> >> >                          Use the following command :
>> >> > -                        <source>jmeter -n -t &lt;test JMX file&gt; -e
>> >> -l &lt;test log file&gt;
>> >> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
>> >> output folder&gt;</source>
>> >> > +                        <source>jmeter -n -t &lt;test JMX file&gt; -e
>> >> -l &lt;test log file&gt; -o &lt;Path to output folder&gt;</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>

Re: svn commit: r1732944 - in /jmeter/trunk: bin/jmeter.properties src/core/org/apache/jmeter/JMeter.java src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java xdocs/changes.xml xdocs/usermanual/generating-dashboard.xml

Posted by UBIK LOAD PACK Support <su...@ubikloadpack.com>.
Hello sebb,

Some notes inline below  from an external eye.

Regards

On Tue, Mar 1, 2016 at 11:08 AM, sebb <se...@gmail.com> wrote:

> On 1 March 2016 at 06:11, Philippe Mouawad <ph...@gmail.com>
> wrote:
> > On Monday, February 29, 2016, sebb <se...@gmail.com> wrote:
> >
> >> On 29 February 2016 at 20:57,  <pmouawad@apache.org <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="&sect-num;.3.1 Generation from an
> >> existing sample log file" anchor="report_only">
> >> >                      <p>
> >> >                          Use the following command :
> >> > -                        <source>jmeter -n -g &lt;log file&gt;
> >> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
> >> output folder&gt;</source>
> >> > +                        <source>jmeter -n -g &lt;log file&gt; -o
> >> &lt;Path to output folder&gt;</source>
> >> >                      </p>
> >> >                  </subsection>
> >> >
> >> >                  <subsection name="&sect-num;.3.2 Generation after
> load
> >> test" anchor="report_after_load_test">
> >> >                      <p>
> >> >                          Use the following command :
> >> > -                        <source>jmeter -n -t &lt;test JMX file&gt; -e
> >> -l &lt;test log file&gt;
> >> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
> >> output folder&gt;</source>
> >> > +                        <source>jmeter -n -t &lt;test JMX file&gt; -e
> >> -l &lt;test log file&gt; -o &lt;Path to output folder&gt;</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>

Re: svn commit: r1732944 - in /jmeter/trunk: bin/jmeter.properties src/core/org/apache/jmeter/JMeter.java src/core/org/apache/jmeter/report/dashboard/HtmlTemplateExporter.java xdocs/changes.xml xdocs/usermanual/generating-dashboard.xml

Posted by sebb <se...@gmail.com>.
On 1 March 2016 at 06:11, Philippe Mouawad <ph...@gmail.com> wrote:
> On Monday, February 29, 2016, sebb <se...@gmail.com> wrote:
>
>> On 29 February 2016 at 20:57,  <pmouawad@apache.org <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?
Those checks are done in the reporter code, and should remain there.

>>
>> 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, 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="&sect-num;.3.1 Generation from an
>> existing sample log file" anchor="report_only">
>> >                      <p>
>> >                          Use the following command :
>> > -                        <source>jmeter -n -g &lt;log file&gt;
>> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
>> output folder&gt;</source>
>> > +                        <source>jmeter -n -g &lt;log file&gt; -o
>> &lt;Path to output folder&gt;</source>
>> >                      </p>
>> >                  </subsection>
>> >
>> >                  <subsection name="&sect-num;.3.2 Generation after load
>> test" anchor="report_after_load_test">
>> >                      <p>
>> >                          Use the following command :
>> > -                        <source>jmeter -n -t &lt;test JMX file&gt; -e
>> -l &lt;test log file&gt;
>> -Jjmeter.reportgenerator.exporter.html.property.output_dir=&lt;Path to
>> output folder&gt;</source>
>> > +                        <source>jmeter -n -t &lt;test JMX file&gt; -e
>> -l &lt;test log file&gt; -o &lt;Path to output folder&gt;</source>
>> >                      </p>
>> >                  </subsection>
>> >              </subsection>
>> >
>> >
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.