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 <[email protected]> wrote: > On 11 November 2011 22:09, <[email protected]> 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.
