jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: svn commit: r1201070 - in /jmeter/trunk: src/core/org/apache/jmeter/services/FileServer.java test/src/org/apache/jmeter/services/TestFileServer.java xdocs/changes.xml
Date Sat, 12 Nov 2011 11:43:03 GMT
Hello Sebb,
I rolled back and changed approach, you were right I missed some major
differences in the 3 methods.

In my new commits, I had to change a catch Exception in one test because I
made the 3 Fileserver method use the same exception IllegalStateException
for the same error.
I impacted client methods to use this new exception.

I stop work on this for now, not sure we can do more without regressions, I
don't think the 3 methods do the same job.

   - setBasedir and setBase differ in the fact setBasedir accepts null
   parameter (strange for me but it's like this)
   - Regarding setBase, I wonder if it is OK to use setBaseForScript and
   passe a new File(jmxBase, scriptName)? what if client and remote are not in
   the same folder ?


If you have some time can you look at:

   - 47850 <https://issues.apache.org/bugzilla/show_bug.cgi?id=47850>
   - And my last note in
43294<https://issues.apache.org/bugzilla/show_bug.cgi?id=43294>


Thanks
Regards
Philippe
On Fri, Nov 11, 2011 at 11:35 PM, sebb <sebbaz@gmail.com> wrote:

> On 11 November 2011 22:09,  <pmouawad@apache.org> wrote:
> > Author: pmouawad
> > Date: Fri Nov 11 22:09:06 2011
> > New Revision: 1201070
> >
> > URL: http://svn.apache.org/viewvc?rev=1201070&view=rev
> > Log:
> > Bug 52150 - FileServer has 3 confusingly similar methods to set the file
> base
>
> -1
>
> Although the result looks tidier, the behaviour has changed; please revert.
>
> > Modified:
> >    jmeter/trunk/src/core/org/apache/jmeter/services/FileServer.java
> >    jmeter/trunk/test/src/org/apache/jmeter/services/TestFileServer.java
>
> By all means add new tests, but if existing tests have to be changed,
> this is a sign that the update caused the code behaviour to change.
>
> >    jmeter/trunk/xdocs/changes.xml
> >
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/services/FileServer.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/services/FileServer.java?rev=1201070&r1=1201069&r2=1201070&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/services/FileServer.java
> (original)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/services/FileServer.java Fri
> Nov 11 22:09:06 2011
> > @@ -113,16 +113,12 @@ public class FileServer {
> >      * @throws IOException if there is a problem resolving the file name
> >      */
> >     public synchronized void setBasedir(String basedir) throws
> IOException {
> > -        if (filesOpen()) {
> > -            throw new IOException("Files are still open, cannot change
> base directory");
> > -        }
> > -        files.clear();
> >         if (basedir != null) {
> > -            base = new File(basedir);
> > +            File base = new File(basedir);
> >             if (!base.isDirectory()) {
> >                 base = base.getParentFile();
> >             }
> > -            log.info("Set new base='"+base+"'");
> > +            setBase(base);
> >         }
> >     }
> >
> > @@ -139,14 +135,9 @@ public class FileServer {
> >         if (scriptPath == null){
> >             throw new IllegalArgumentException("scriptPath must not be
> null");
> >         }
> > -        if (filesOpen()) {
> > -            throw new IllegalStateException("Files are still open,
> cannot change base directory");
> > -        }
> > -        files.clear();
> > -        // getParentFile() may not work on relative paths
> > +        File parentFolder =
> scriptPath.getAbsoluteFile().getParentFile();
> > +        setBase(parentFolder);
> >         setScriptName(scriptPath.getName());
> > -        base = scriptPath.getAbsoluteFile().getParentFile();
> > -        log.info("Set new base '"+base+"'");
> >     }
> >
> >     /**
> >
> > Modified:
> jmeter/trunk/test/src/org/apache/jmeter/services/TestFileServer.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/test/src/org/apache/jmeter/services/TestFileServer.java?rev=1201070&r1=1201069&r2=1201070&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/test/src/org/apache/jmeter/services/TestFileServer.java
> (original)
> > +++ jmeter/trunk/test/src/org/apache/jmeter/services/TestFileServer.java
> Fri Nov 11 22:09:06 2011
> > @@ -94,13 +94,21 @@ public class TestFileServer extends JMet
> >         assertFalse("Should not have any files open",FS.filesOpen());
> >         assertEquals("a1,b1,c1,d1",FS.readLine(infile));
> >         try {
> > -            FS.setBasedir("x");
> > -            fail("Expected IOException");
> > -        } catch (IOException ignored){
> > +            FS.setBasedir(System.getProperty("java.io.tmpdir"));
> > +            fail("Expected IllegalStateException");
> > +        } catch (IllegalStateException ignored){
> > +               assertEquals("Files are still open, cannot change base
> directory", ignored.getMessage());
> >         }
> >         FS.closeFile(infile);
> > -        FS.setBasedir("y");
> > +        FS.setBasedir(System.getProperty("java.io.temp"));
> >         FS.closeFiles();
> > +
> > +        try {
> > +               FS.setBasedir("x");
> > +            fail("Expected IllegalArgumentException");
> > +        } catch (IllegalArgumentException ignored){
> > +               assertEquals("jmxBase must not be null",
> ignored.getMessage());
> > +        }
> >     }
> >
> >     public void testRelative() throws Exception {
> >
> > Modified: jmeter/trunk/xdocs/changes.xml
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1201070&r1=1201069&r2=1201070&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/xdocs/changes.xml (original)
> > +++ jmeter/trunk/xdocs/changes.xml Fri Nov 11 22:09:06 2011
> > @@ -208,6 +208,7 @@ these occurs, Sampler is marked as faile
> >  <li>Bug 52116 - Allow to add (paste) entries from the clipboard to an
> arguments list</li>
> >  <li>Bug 51091 - New function returning the name of the current "Test
> Plan"</li>
> >  <li>Bug 52160 - Don't display TestBeanGui items which are flagged as
> hidden</li>
> > +<li>Bug 52150 - FileServer has 3 confusingly similar methods to set the
> file base</li>
> >  </ul>
> >
> >  <h2>Non-functional changes</h2>
> >
> >
> >
>



-- 
Cordialement.
Philippe Mouawad.

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