jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Antonio Gomes Rodrigues <ra0...@gmail.com>
Subject Re: [GitHub] jmeter pull request: bug59153_ExceptionInCSVDataSetWithEmptyFilena...
Date Tue, 15 Mar 2016 13:25:53 GMT
New PR submited

PR 167

Now I throw I more clear IllegalArgumentException if filename is empty

And a more clear IllegalArgumentException if there is an access problems
(not a file, not readeable, not exist) with filename

I don't have formatting it to allow easy review

Antonio

2016-03-15 13:33 GMT+01:00 Antonio Gomes Rodrigues <ra0077@gmail.com>:

> Hi
>
> 2016-03-15 12:32 GMT+01:00 sebb <sebbaz@gmail.com>:
>
>> On 15 March 2016 at 11:15, Antonio Gomes Rodrigues <ra0077@gmail.com>
>> wrote:
>> > For the moment we have only a FileNotFoundException and this error
>> "ERROR -
>> > jmeter.threads.JMeterThread: Test failed!
>> > java.lang.IllegalArgumentException: Could not read file header line"
>> >
>> > Like you can see is not very usefull
>>
>> However later in the stack trace you will see something like:
>>
>> Caused by: java.io.FileNotFoundException: /workspace/JMeter/bin/xyz
>> (No such file or directory)
>>
>
> It's usefull for you because youre are a developper.
>
> A user which see this error will not necesary understand (no knowledge of
> Java exception, etc.)
>
> And I don't think it's a good idea to show exception to users
>
>
>>
>> > I will work in another file problems (not present, not readable, etc.)
>> in
>> > another PR
>> >
>> > For the moment I have just make this PR to alert user without stacktrace
>> > when he forget to setup the file name.
>> >
>> > Are you ok to have two PR or do you want I try to make only one PR with
>> all
>> > the possible problem?
>>
>> There is only one problem - cannot read the file.
>>
>> > My opinion is to have 2 PR (this one to non setup file name and another
>> to
>> > access problem to the file)
>>
>> Again, I don't see the point of having two separate processes.
>> Just catch the IllegalArgumentException and report it (with the cause if
>> any)
>>
>
> I will try to do it asap
>
> Antonio
>
>>
>> >
>> > Antonio
>> >
>> >
>> >
>> >
>> > Cet e-mail a été envoyé depuis un ordinateur protégé par Avast.
>> > www.avast.com
>> > <
>> https://www.avast.com/fr-fr/lp-safe-emailing?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=OA-2109-A
>> >
>> > <#DDB4FAA8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
>> >
>> > 2016-03-15 11:51 GMT+01:00 sebb <sebbaz@gmail.com>:
>> >
>> >> On 15 March 2016 at 09:30, ra0077 <git@git.apache.org> wrote:
>> >> > Github user ra0077 commented on the pull request:
>> >> >
>> >> >     https://github.com/apache/jmeter/pull/162#issuecomment-196740857
>> >> >
>> >> >     I have swap it and remove formating to be easy to read it
>> >> >
>> >> >     What I do is:
>> >> >     check if the user have put a file name
>> >> >     if not
>> >> >     log it to allow easy debuging
>> >> >     stop the test
>> >> >     if yes
>> >> >     no modification
>> >> >
>> >> >
>> >> >     It allow to easy debug (a clear message in log) and avoid to
>> have an
>> >> >     exception to user
>> >> >
>> >> >
>> >> >
>> >> >     The diff with the trunk:
>> >> >
>> >> >     --- a/src/components/org/apache/jmeter/config/CSVDataSet.java
>> >> >     +++ b/src/components/org/apache/jmeter/config/CSVDataSet.java
>> >> >     @@ -146,6 +146,11 @@ public class CSVDataSet extends
>> >> ConfigTestElement
>> >> >
>> >> >          @Override
>> >> >          public void iterationStart(LoopIterationEvent iterEvent) {
>> >> >     +        String _fileName = getFilename();
>> >> >     +        if (_fileName.isEmpty()) {
>> >> >     +            log.error("No filename setup in CSV Data Set Config:
>> >> >     "+this.getName());
>> >> >     +            throw new JMeterStopThreadException("No filename
>> setup
>> >> in CSV
>> >> >     Data Set Config: "+this.getName());
>> >> >     +        } else {
>> >> >              FileServer server = FileServer.getFileServer();
>> >> >              final JMeterContext context = getThreadContext();
>> >> >              String delim = getDelimiter();
>> >> >     @@ -156,7 +161,6 @@ public class CSVDataSet extends
>> ConfigTestElement
>> >> >                  delim=",";
>> >> >              }
>> >> >              if (vars == null) {
>> >> >     -            String _fileName = getFilename();
>> >> >                  String mode = getShareMode();
>> >> >                  int modeInt =
>> >> CSVDataSetBeanInfo.getShareModeAsInt(mode);
>> >> >                  switch(modeInt){
>> >> >     @@ -212,6 +216,7 @@ public class CSVDataSet extends
>> ConfigTestElement
>> >> >                      threadVars.put(var, EOFVALUE);
>> >> >                  }
>> >> >              }
>> >> >     +        }
>> >> >          }
>> >>
>> >> Thanks, that is more legible
>> >> (though the mail software causes wrapping issues)
>> >>
>> >> That looks OK.
>> >> Though if one wants to avoid the stacktrace appearing in the log it
>> >> might just be simpler to change the log message to drop the stack
>> >> trace for the incorrect filename case as well.
>> >> Does the stacktrace help for that case? I suspect not.
>> >> I think that would be a more useful fix.
>> >>
>> >> >     Antonio
>> >> >
>> >> >     Cet e-mail a été envoyé depuis un ordinateur protégé par Avast.
>> >> >     www.avast.com
>> >> >     <
>> >>
>> https://www.avast.com/fr-fr/lp-safe-emailing?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=OA-2109-A
>> >> >
>> >> >     <#DDB4FAA8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
>> >> >
>> >> >     2016-03-15 9:13 GMT+01:00 Vladimir Sitnikov <
>> >> notifications@github.com>:
>> >> >
>> >> >     > I don't have modify these lines.
>> >> >     >
>> >> >     > @ra0077 <https://github.com/ra0077>, can you please
rework
>> the PR
>> >> without
>> >> >     > reformatting the whole method?
>> >> >     >
>> >> >     > —
>> >> >     > You are receiving this because you were mentioned.
>> >> >     > Reply to this email directly or view it on GitHub:
>> >> >     >
>> https://github.com/apache/jmeter/pull/162#issuecomment-196710977
>> >> >     >
>> >> >
>> >> >
>> >> >
>> >> > ---
>> >> > If your project is set up for it, you can reply to this email and
>> have
>> >> your
>> >> > reply appear on GitHub as well. If your project does not have this
>> >> feature
>> >> > enabled and wishes so, or if the feature is enabled but not working,
>> >> please
>> >> > contact infrastructure at infrastructure@apache.org or file a JIRA
>> >> ticket
>> >> > with INFRA.
>> >> > ---
>> >>
>>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message