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 <[email protected]>: > Hi > > 2016-03-15 12:32 GMT+01:00 sebb <[email protected]>: > >> On 15 March 2016 at 11:15, Antonio Gomes Rodrigues <[email protected]> >> 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 <[email protected]>: >> > >> >> On 15 March 2016 at 09:30, ra0077 <[email protected]> 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 < >> >> [email protected]>: >> >> > >> >> > > 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 [email protected] or file a JIRA >> >> ticket >> >> > with INFRA. >> >> > --- >> >> >> > >
