bugGNU Octave - Bugs: bug #54718, __run_test_suite__ doesn't...

 
 

bug #54718: __run_test_suite__ doesn't preserve start or working directory

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Mon 24 Sep 2018 02:02:16 PM UTC
   
 
Category:  Test Suite Severity:  2 - Minor
Priority:  3 - Low Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  philipnienhuis Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 28 Sep 2018 06:56:24 AM UTC, comment #23: 

Just to be sure, I can confirm that this has been fixed. Thanks very much!

Philip Nienhuis <philipnienhuis>
Group Member
Thu 27 Sep 2018 08:21:12 PM UTC, comment #22: 

This is now working as expected.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Thu 27 Sep 2018 06:37:40 PM UTC, comment #21: 

OK, then I think this change is what we want instead:

  http://hg.savannah.gnu.org/hgweb/octave/rev/e980aef0e1db

It seems to work for me.  Does it work as you expect now?

John W. Eaton <jwe>
Group administrator
Thu 27 Sep 2018 05:44:22 PM UTC, comment #20: 

Third chime.  I expect the GUI to follow the interpreter, and not the other way around.

Rik <rik5>
Group administrator
Thu 27 Sep 2018 05:40:20 PM UTC, comment #19: 

This is also unexpected for me. If the directories were out of sync and I enable the setting, I would expect that the GUI elements would change to match the current working directory of the interpreter. That's the way it used to work and it made a lot of sense to me.

Mike Miller <mtmiller>
Group Member
Thu 27 Sep 2018 01:19:31 PM UTC, comment #18: 

One more observation and question:

Currently, if you have the setting enabled, it appears that the Octave's working directory is updated any time the file browser switches to a new directory.  AND the file browser follows any change in Octave's working directory.  That seems reasonable to me as long as they are always kept in sync.

However,, if you have the setting disabled and the file browser becomes out of sync with Octave's current working directory and then you enable the setting, which one should change?  Should the file browser be reset to Octave's working directory or should Octave's working directory be modified to match the file browser?  It's not clear to me which should happen.  So maybe it is best to just leave it out of sync until the user changes one of them.

John W. Eaton <jwe>
Group administrator
Thu 27 Sep 2018 12:27:55 PM UTC, comment #17: 

Possibly there is another solution, but I think emitting the signal to update the directory display in the notice_settings function is not the right thing to do.

John W. Eaton <jwe>
Group administrator
Thu 27 Sep 2018 05:08:54 AM UTC, comment #16: 

John, it would be unexpected behavior for me if I change the preferences in order to synchronize both directories and they are still not the same afterwards. Maybe we can call display_directory only when the setting actually was set to true?

Torsten Lilge <ttl>
Group Member
Wed 26 Sep 2018 08:32:48 PM UTC, comment #15: 

Philip: It turned out to be fairly simple, but I wanted to be sure that there wasn't something fundamentally wrong with the communication between the GUI and the interpreter.

John W. Eaton <jwe>
Group administrator
Wed 26 Sep 2018 08:23:45 PM UTC, comment #14: 

Thanks John.
I'll try the fix later this week - our ADSL modem became dysfunctional after maintenance work on power cables near our block :-(  Waiting for a replacement modem.

BTW I never anticipated that this IMO minor bug would attract so many attention and postings.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 26 Sep 2018 02:57:36 PM UTC, comment #13: 

I pushed the following changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/4c721ed27a28

The call to display_directory ultimately emitted a signal to change Octave's directory.  Since that signal is queued and may not be processed until after the chdir function in the script (or series of commands entered at the prompt) was finished, the current directory could be reset to a previous value.

In addition to fixing the reported problem, my patch also changes Octave's behavior.  Previously, if you had sync_octave_directory set to false and the directory displayed in the file browser was different from Octave's current directory, then you changed the sync_octave_directory setting to true, the directory displayed in the file browser would change to match Octave's current directory as soon as you applied the setting.  Now, it will not synchronize until you make some other change, either by changing Octave's current directory or by viewing another directory in the file browser.  That seems like reasonable behavior to me.

John W. Eaton <jwe>
Group administrator
Tue 25 Sep 2018 09:42:38 PM UTC, comment #12: 

One more example, that also shows the requirement that the user setting "sync_octave_directory" needs to be enabled.


__octave_link_gui_preference__ ("filesdockwidget/sync_octave_directory", "true");
chdir ("/tmp");
__octave_link_gui_preference__ ("editor/show_dbg_file", "false");
__octave_link_gui_preference__ ("editor/show_dbg_file", "true");
chdir (get_home_directory ());
pwd ()


Again, save and run as a script file or combine into a one-liner. The 'pwd' will show your home directory, but the current directory after returning to the prompt will be /tmp.

This is slightly different from debug.cc-tst, in that I'm explicitly changing the "show_dbg_file" setting to "true", where debug.cc-tst is restoring it to its previous value, which may have been false. If the user setting is set to false and then restored to false, this bug is also not present.

Mike Miller <mtmiller>
Group Member
Tue 25 Sep 2018 09:25:52 PM UTC, comment #11: 

Yes, I can now reproduce this with a much narrower example. Run the following as a script file or as a one-liner at the prompt:


fn = fullfile (__octave_config_info__ ("octtestsdir"), "libinterp", "corefcn", "debug.cc-tst");
chdir ("/tmp");
test (fn);
chdir (get_home_directory ());
pwd ()


Inside the script or one-liner, the 'pwd' at the end is reported correctly as my home directory. But after returning to the Octave prompt the current directory is set back to /tmp.

Mike Miller <mtmiller>
Group Member
Tue 25 Sep 2018 04:25:31 PM UTC, comment #10: 

My guess is that the unwind_protect code works properly, but that a chdir command queued for the GUI synchronization thing is executed after that.  On my system, the queued command must be running before the unwind_protect code, at least in the cases I've observed.

John W. Eaton <jwe>
Group administrator
Tue 25 Sep 2018 04:17:33 PM UTC, comment #9: 

Weird.  I have "Synchronize Octave working directory with file browser" enabled in the preferences and when I run the code from comment #6, the directory is restored.

John W. Eaton <jwe>
Group administrator
Tue 25 Sep 2018 03:48:55 PM UTC, comment #8: 

I never run with the preference setting for synchronizing the GUI directory with Octave, but that is definitely required.  If I turn on that setting and execute the code from comment #6 then I can produce this bug on the development branch.  A binary search of the files showed that debug.cc-tst is the one causing the problem.   Within that file, it is the call to _octave_link_gui_preference_ that seems to cause the problem.  When I comment those out the test passes and the directory returns to the correct one.


## DO NOT EDIT!  Generated automatically from debug.cc by Make.
%!test
%! if (isguirunning ())
%!   orig_show_dbg = "0";
%!   #orig_show_dbg = __octave_link_gui_preference__ ("editor/show_dbg_file", "0");
%! endif
%! unwind_protect
%!   dbclear all;   # Clear out breakpoints before test
%!   dbstop @ftp/dir;
%!   dbstop @audioplayer/set 70;
%!   dbstop quantile>__quantile__;
%!   dbstop ls;
%!   s = dbstatus;
%!   dbclear all;
%!   assert (s(1).name, "@audioplayer/set>setproperty");
%!   assert (s(2).name, "@ftp/dir");
%!   assert (s(3).name, "ls");
%!   assert (s(4).name, "quantile>__quantile__");
%!   assert (s(2).file(end-10:end), [filesep "@ftp" filesep "dir.m"]);
%! unwind_protect_cleanup
%!   if (isguirunning ())
%!    # __octave_link_gui_preference__ ("editor/show_dbg_file", orig_show_dbg);
%!   endif
%! end_unwind_protect



Rik <rik5>
Group administrator
Mon 24 Sep 2018 11:53:40 PM UTC, comment #7: 

It's worth noting that the '__run_test_suite__' in Octave 4.4 does not 'cd' when it runs tests in function files and in the tst files extracted from liboctave and libinterp. This was changed on the development branch, see https://hg.savannah.gnu.org/hgweb/octave/rev/2be7cf62b9ee.

With that in mind, I tried running the same command as before, with Octave 4.4.1, but with the '__run_test_suite__' from the default branch added to the path. And that completed successfully and restored the working directory properly.

Mike Miller <mtmiller>
Group Member
Mon 24 Sep 2018 11:27:06 PM UTC, comment #6: 

After some more testing, the following is enough to reproduce this for me 100% in the development version:


tdir = __octave_config_info__ ("octtestsdir");
__run_test_suite__ ({fullfile(tdir, "libinterp", "corefcn")}, {})


With any other directory or set of directories, when '__run_test_suite__' finishes it correctly restores the working directory. If and only if the libinterp/corefcn directory is included in the test run, then the working directory is set to that directory at the end.

If I pass the path(s) to Octave 4.4.1's test directory(ies), even if I am running Octave 5, the working directory is restored correctly.

I put a debug print into the unwind_protect_cleanup block where the working directory is supposed to be restored, and it does execute the block of code, and says it is calling 'cd' with the right argument. But yet, when the function returns, the working directory is not what was passed to 'cd'.

One final clue. If I disable the setting in the Preferences, File Browser dialog that says "Synchronize Octave working directory with file browser", then the working directory is restored to the correct original directory. But again, this only seems to be a problem with the libinterp/corefcn directory.

Can anyone verify any or all of these findings?

Mike Miller <mtmiller>
Group Member
Mon 24 Sep 2018 10:15:12 PM UTC, comment #5: 

Oops, I didn't finish.

2. I tested in octave-cli, and the current directory is correct, it is set to the same as the working directory before calling '__run_test_suite__'.

Possibly due to some interference from the GUI? Race condition? Or a test that only runs when the GUI is active or when the qt toolkit is available?

Mike Miller <mtmiller>
Group Member
Mon 24 Sep 2018 10:11:19 PM UTC, comment #4: 

I can confirm on Debian with 2da65009cc7f.

1. I tested in the GUI, and when '__run_test_suite__' finishes successfully, the current directory is now ${OCTAVE_HOME}/share/octave/5.0.0/etc/tests/libinterp/corefcn.

2. I tested in octave-cli

3. Yes, the GUI shows the local directory changing as the test directories are iterated over. It has always been this way. It's only the workspace panel that does not update when a script runs. I believe there is a user preference to not update directory displayed on the GUI when the current directory changes in the interpreter.

4. I think this is a regression, and possibly due to one of the "fixed" test scripts that has changed or been added since 4.4. I will do some more testing to see if I can verify that or narrow it down further. I can only guess, because I think I saw the correct directory flashing in the GUI toolbar, up until the test suite got to the fixed tests.

Mike Miller <mtmiller>
Group Member
Mon 24 Sep 2018 07:48:10 PM UTC, comment #3: 

100 %.
_run_test_suite_ yields one or more FAILs but those are in privately applied patches (a.o., histcounts, see patch #8801)
A build from Sept. 4 does show this issue.

Yes I saw the unwind_protect block in the first subfunc; but isn't it better to save and restore the working directory in the outer function?
I tried myself (just added "orig_dir = pwd ()" as first command and 
"cd (orig_dir)" as last, but that didn't work and I gave up quickly, as I somewhat agree with Rik.
But now that you jump in I'm re-alerted and a hunch from me is that apparently the unwind_protect block isn't working as hoped for.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 24 Sep 2018 06:51:34 PM UTC, comment #2: 

I don't understand how the original directory is not restored.  In the run_test_dir function in _run_test_suite_.m, an unwind_protect block is used to restore the saved directory.  Is this failure 100% reproducible for you?

I'm not sure what should happen with the file browser.  Maybe it should not update except when Octave displays a prompt.  The behavior does seem different with 4.4.1, but I still see the file browser changing sometimes, for example when running the tests in the gzip.cc-tst file.

John W. Eaton <jwe>
Group administrator
Mon 24 Sep 2018 03:36:48 PM UTC, comment #1: 

_run_test_suite_ isn't really meant for use by anyone but developers, who can handle oddities like this.  I've changed the report to a Feature Request since it would be nice to preserve the current directory if possible.

Rik <rik5>
Group administrator
Mon 24 Sep 2018 02:02:16 PM UTC, original submission:  

Since a few weeks, I see in the File Browser pane in the GUI that __run_test_suite__ switches in turn to each directory holding the functions to be tested.

First (and least), AFAIU Octave isn't supposed to update the working directory in the File Browser pane until after having returned to the prompt. Or do I mix up with the Workspace pane?

Then (actual bug), after finishing all the tests,  the current working directory is that where the last functions were tested [1], rather than the one __run_test_suite__ was started from.

[1] C:/Programs/Octave/OCTAVE~1.0_2/mingw64/share/octave/5.0.0/etc/tests/libinterp/corefcn

Philip Nienhuis <philipnienhuis>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-09-27 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2018-09-24 mtmiller StatusNone Confirmed
    2018-09-24 rik5 Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
        Item GroupNone Feature Request

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code