You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2011/11/11 23:35:06 UTC
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
On 11 November 2011 22:09, <pm...@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>
>
>
>
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
Posted by Philippe Mouawad <ph...@gmail.com>.
Also if you have some time to look at:
- 45132
Thanks
Regards
Philippe
On Sat, Nov 12, 2011 at 12:43 PM, Philippe Mouawad <
philippe.mouawad@gmail.com> wrote:
> 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 <se...@gmail.com> wrote:
>
>> On 11 November 2011 22:09, <pm...@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.
>
>
>
>
--
Cordialement.
Philippe Mouawad.
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
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <se...@gmail.com> wrote:
> On 11 November 2011 22:09, <pm...@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.