bugGNU Octave - Bugs: bug #41215, Request for a "pkg test"...

 
 

bug #41215: Request for a "pkg test" feature

Submitter:  Julien Bect <jbect>
Submitted:  Mon 13 Jan 2014 09:15:49 AM UTC
   
 
Category:  Octave Function Severity:  1 - Wish
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  philipnienhuis
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 16 Feb 2020 09:39:07 PM UTC, comment #38: 

Thanks Mike.

Could you also please review and if possible push the related patch in bug #41298? (see comment #21 there, patch v.4)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 16 Feb 2020 07:41:13 PM UTC, comment #37: 

I made some further doc and style changes, cleaned up the syntax a bit, and did some minimal testing to make sure I didn't break anything. I pushed this on the default branch, should be part of Octave 6

https://hg.savannah.gnu.org/hgweb/octave/rev/fbed279b7074

Mike Miller <mtmiller>
Group Member
Sun 16 Feb 2020 06:28:37 PM UTC, comment #36: 

Will this be getting into the 6X release, or a future version?

John Donoghue <lostbard>
Group Member
Wed 08 Jan 2020 12:29:41 AM UTC, comment #35: 

Testing with patch from Bug #41298 included, and appears to work fine for me

John Donoghue <lostbard>
Group Member
Sun 05 Jan 2020 12:56:14 PM UTC, comment #34: 

forgot uploading the patch. Done.

(file #48167)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 05 Jan 2020 12:54:42 PM UTC, comment #33: 

OK, new cset attached (octave-bug-41215-v4.cset); please review.

Together with bug_41298_pkg_install_tests.v6.cset the 'pkg test' feature should be complete.

Anything else needed for this bug report? I hope not, this is the last day of my Xmas holiday and there's lots of other things I need  to do :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 04 Jan 2020 11:40:49 PM UTC, comment #32: 

Yeah, I was already planning on filing a bug report to use a new better test function than "__run_test_suite__" in "pkg test" once this is complete.

Here's the thing, _run_test_suite_ is a private function and is very specific to Octave's own test suite. It has internal assumptions about the directory structure, about the log file name, about the existence of inline tests vs standalone test scripts.

But it does have some useful features like directory tree recursion, test errors to a log file, returned test status, that no other built in test functions have.

What we need is a new *public* function to run any test suite, not just Octave's test suite, with these same features. Recurse into directory structures. Output the test status with long root directories elided. Send test errors to an optional log file with the name chosen by the caller. Return the number of tests passed and failed.

See bug #44303.

For now, Octave has no suitable alternative to _run_test_suite_, so you might as well use it. I don't think you should change the directory or add any other hacks to work around the fntests.log file name.

If you're interested, I use runtests2 from the testify package (https://github.com/apjanke/octave-testify) to run test suites in my packages. Obviously external for now, so that doesn't help here. This function implements most of these features.

Mike Miller <mtmiller>
Group Member
Sat 04 Jan 2020 10:38:40 PM UTC, comment #31: 

OK, demos aren't run, that got sorted out in bug #41298. Fine.

Another issue with the latest patch is that __run_test_suite__.m always writes an fntests.log file in the working directory.
As the patch stands it'll be overwritten for each new package to be tested.
I thought about cd'íng to the package directory, but then we might get trouble if that directory isn't writable (think of global packages).

Suggestions?
Is there a way to get __run_test_suite__.m to skip producing a fntests.log file?

I'd like to stick with __run_test_suite__.m because it recurses into subdirs and gives a nice overview for each individual package.
oruntests.m first has to be patched before it can be used for tests extracted from .cc files (see patch in bug #41298) but maybe need even more patching.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 04 Jan 2020 10:29:02 PM UTC, comment #30: 

While io just sets up a javaclasspath which might be harmless, other packages may do things that could be more serious and shouldn't linger on. Stale handles, communication channels, hardware connections, etc. come to mind.
But taking into account that it'll be merely developers that will run 'pkg test' this might be less relevant.
It'll be moot anyway if bug #57535 gets fixed soon enough. Indeed, no reason to hold here.

Being able to simply do 'path (oldpath)' is easy, but tracking what needs to be removed from the path is also easy: it's just that part that got prepended to the old path except ".". Figuring out the pertinent package names is a one-liner cellfun. All in all it is just a few lines of code.

[A little OT]
Yep, pkg and friends look ridiculously complicated, but maybe we should be a bit more forgiving. Once you get to think about all the concealed complications, like e.g., nested and shared dependencies, and once you realize that it dates from long before classdef, you soon get to appreciate why it is as it is.

But as it stands, some detailed code is quite naive and could be much more efficient and more concisively written, I give you that. Where I first left it intact as much as I could, I recently started rewriting pieces where I was working, if only to make debug-stepping more manageable.
A question I might raise on the maintainers list is why the package info is kept in single structs in a cell array. That makes code unduly convoluted IMO.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 04 Jan 2020 07:06:41 PM UTC, comment #29: 

Yes, despite "path (orig_path)" not running PKG_DEL currently, I still think this is vastly superior to trying to keep track of directories that you need to run rmpath() on in a loop or a cellfun.

Do you really think it's a serious problem that "pkg test io" will leave the java class path modified? I don't.

As I said, this is a new feature, and I think we should rather implement it as simply and easy to read as possible, and not clutter it with unnecessary code that will be deleted later. These kinds of complications are why I basically don't work on the pkg function at all, it's already ridiculously complicated.

Mike Miller <mtmiller>
Group Member
Sat 04 Jan 2020 02:36:36 PM UTC, comment #28: 

@Mike
Going back to comment #24, you mention to just restore the previous path by saving it along the lines of

oldpath = path;
<internal pkg test stuff>
path (oldpath);

Right?
But if I correctly interpret bug #57535, PKG_DEL in a package directory isn't executed that way; for at least the io package that poses a problem as then it has no chance to clean up the javaclasspath that was set up during pkg initialization.
I already have something that just rmpath-es the extra added pkg directories from the path; after some more testing that can be used until bug #57535 gets fixed.

As to demos, please see bug #41298 to keep this thread on subject.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 04 Jan 2020 05:09:54 AM UTC, comment #27: 

I think running both tests and demos is not desirable for a "pkg test" command.

The "bin" subdirectory seems like a package feature that is not actually widely used in practice, and should maybe be deleted to simplify the pkg command. But if you want to be complete, it would be easy to also save and restore the value of EXEC_PATH.

Mike Miller <mtmiller>
Group Member
Fri 03 Jan 2020 11:39:08 PM UTC, comment #26: 

Oh thanks Mike, that's going to be an easy one then.
Tomorrow I'll try to put up a revised cset.

In the mean time I found that the csets here and in bug #41298 also run demos. Maybe not so bad, that gives a chance to also verify that any demos run w/o errors. Or is it undesirable?

What about EXEC_PATH? Related stanzas for that are commented out with FIXME notes, so I guess there's little to bother there.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 03 Jan 2020 10:55:22 PM UTC, comment #25: 

Filed a bug about PKG_DEL as bug #57535.

That bug does not need to block this one being fixed by restoring the path. It can simply be commented that this is a valid way to restore the load path, even though it currently misses PKG_DEL statements.

Mike Miller <mtmiller>
Group Member
Fri 03 Jan 2020 10:13:34 PM UTC, comment #24: 

Philip - I think you're overthinking it. The only thing you need to do to unload any number of packages is to revert the path. This is super simple:


  orig_path = path ();
  restore_path = onCleanup (@() path (orig_path));

  ## code that loads packages, runs tests, etc...


If this way of restoring the path does not run PKG_DEL files in the directories that are removed, then that seems like another bug report. But let's not make this feature harder than it needs to be.

Mike Miller <mtmiller>
Group Member
Fri 03 Jan 2020 03:29:00 PM UTC, comment #23: 

Hi Mike,
Whether a package is loaded is tracked in (1) the path (authorative IMO) and stored by various pkg.m private functions into a variable "installed_pkgs_lst".
Unfortunately the latter seems to be volatile and regenerated for almost any pkg.m activity. AFAICS it never makes into the pkg.m function proper.

As to what needs to be done, your idea matches mine. When unloading packages also PKG_DEL needs to be called, so my idea is to compare the path before and after, and for all packages that were found to be loaded in the course of 'pkg tst' just rmpath() their directories. Is that what onCleanup() does? (I don't know its details, never used it).
Indeed installed-packages.m doesn't need to be invoked.
Packages might also manipulate the EXEC_PATH; I suppose that could be tackled analogously?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 02 Jan 2020 11:21:20 PM UTC, comment #22: 

As far as I can tell, whether a package is loaded or not is not actually tracked anywhere, other than what's in the current path. Philip, do you agree with that, since you've been hacking on pkg.m most recently?

If that's the case, then I think the easiest solution would be to simply save the path, load the package to be tested, run the tests, and restore the original path with an onCleanup object.

Mike Miller <mtmiller>
Group Member
Sun 29 Dec 2019 11:32:05 PM UTC, comment #21: 

I couldn't help looking at unloading after testing :-)
And there I ran into another pkg.m bug: if you unload some dependency of a package (depender), and then again load the depender, the dependency isn't reloaded. I need to investigate more, but to fix this one challenging issue will be inserting the dependency at the right place in the path (= after the depender). Interdependencies and shared dependencies will make this an interesting job.

However for the issue at hand (pkg test) my proposed strategy + some workaround(s) will probably do the job, some other small pkg.m glitches might have to be smoothed out as well.

All in all pkg.m isn't so bad at all IMO, but there were a few wrinkles left behind that I'm tripping over now :-)

New cset is attached, with a minor fix (an outdated part of an error message that I overlooked).


(file #48137)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 29 Dec 2019 02:50:05 AM UTC, comment #20: 

I didnt even think about the dependancies that would be loaded.

You are right - first things first, and get the current patches merged up.

John Donoghue <lostbard>
Group Member
Sat 28 Dec 2019 10:14:38 PM UTC, comment #19: 

<Loading packages>
Yeah I thought about that early on and looked into it. An issue is that pkg.m automatically also loads all related package dependencies.
pkg.m is smart there but when UNloading a package it does NOT take its dependencies nor its dependers into account.
Maybe the underlying reason is that sorting out shared dependencies was too daunting at the time pkg.m was written. Indeed the code to find & load dependencies for just one package is already quite substantial, but for unloading some package the same or similar code has to be run for all other packages as well. For unloading several packages in one call the code would get even more complicated. As pkg.m stands dependencies are just one-way "linkages", if they were two-way instead things would be a lot simpler.

However for this case it could be easy (in principle) - installed_packages.m returns a cell array with an info struct for each package incl. a "loaded" field. Running it before and after should give the required info. But a snag is that the results can't be fed directly to the "unload" option - there's some processing required.

I'd suggest to first get bug #41298, bug #57489 plus this bug (all related) "fixed" and then I'll look into the unloading stuff.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 28 Dec 2019 01:55:14 PM UTC, comment #18: 

Seems to work for me on dicom and zeromq, running both the inst files (and subdirs) and the arch tests installed from Bug #41298

Only question for me would be that it loads the pkg when running teh tests, but doesnt unload it afterward - should it?

John Donoghue <lostbard>
Group Member
Fri 27 Dec 2019 10:27:31 PM UTC, comment #17: 

Forgot to upload cset. Here it is.

(file #48124)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 27 Dec 2019 10:26:42 PM UTC, comment #16: 

Here's an updated cset.

I hit a bug in private/installed_packages.m along the way and uploaded a fix for that, see bug #57489 (cset in comment #1).

This cset works fine now with the csets from bug #41298 (comment # 16) and bug #57489 (I marked those as dependencies).
For each package in turn it invokes __run_test_suite__.m so also packages with subdirs in inst/ can be tested, along with the tests in the arch-dependent directory.

The only glitch I still see is that for each package, __run_test_suite__.m creates a separate fntests.log file in the current directory.
It can be argued to first cd to the package's arch-independent directory so that fntests.log for each package will be created there. That would be a one-liner along L.656 of (patched) pkg.m.
Any thoughts ?


Philip Nienhuis <philipnienhuis>
Group Member
Wed 25 Dec 2019 06:56:21 PM UTC, comment #15: 

I've been polishing this cset a little, it works OK then, but I found the results a bit disappointing w.r.t. layout and the order in which tests are reported.
I tweaked the cset so that .cc-tst files in the archdependent subdir are also executed (see bug #41298); that part wasn't there yet in the previous cset :-)

I see this:

  • It looks like all tests are thrown together on a heap and reported in random order, even for just one package with binary and m.file functions. What I would rather see is that tests are run package after package, with .m-file and .cc-tst file separated.
  • The full paths of the tested function files / test files are echoed rather than just the function file name, which severely clobbers up the on-screen display.
  • _run_test_suite_.m adds an fntests.log file in the current directory that may not be wanted.


Both of these seem to be due to how _run_test_suite_.m seems to work. I also tried with patched oruntests.m (see bug #41298) but got just a little bit further.

See attached diff fo how far I got.

(file #48118)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 28 Jan 2019 08:07:57 PM UTC, comment #14: 

The patch no longer cleanly applies to default.
It would also be nice (albeit of lower priority) if it would check if the package was loaded before the test and unload it (with all previously unloaded dependencies) after the test has finished.
I also can't find the patch from comment #13.

Markus Mützel <mmuetzel>
Group administrator
Thu 10 Aug 2017 06:20:35 AM UTC, comment #13: 

Thanks Oliver this idea looks good to me!

I found an issue about multiple package names while reviewing this.  I'll attach a patch which also adds docs and a return value.  My patch applies on top of Oliver's.

Colin Macdonald <cbm>
Thu 19 Feb 2015 02:14:01 PM UTC, comment #12: 

Some additional information on this patch and the patch submitted for bug #41298: Together they enable the following new features:

- “pkg test <package>” will run all tests of a particular package. Even tests from .cc sources.
- “pkg test all” will do the same for all installed packages
- “test <functionname>” will also work for OCT-files from packages which have been compiled from .cc sources

Of course, the pkg.m documentation string is to be updated.

Oliver Heimlich <oheim>
Thu 19 Feb 2015 01:13:09 PM UTC, comment #11: 

Please review the attached patch file. Another patch is posted in bug #41298.

(file #33130)

Oliver Heimlich <oheim>
Thu 19 Feb 2015 09:16:37 AM UTC, comment #10: 

Good point, it would be a one liner for few .cc files, but becomes unfeasible if the package has a lot of them. I will prepare two patches then.

Oliver Heimlich <oheim>
Thu 19 Feb 2015 12:44:21 AM UTC, comment #9: 

I agree with all but the first item. There should be a common solution for all packages built in to the pkg command to fix bug #41298. Or do you have a portable one-liner that can easily be stuffed into every Octave Forge package to make tst files?

Mike Miller <mtmiller>
Group Member
Wed 18 Feb 2015 09:26:48 PM UTC, comment #8: 

I propose the following solution:
- The extraction of tests from .cc files is the package maintainers responsibility. The package's Makefile shall contain a build target for .cc-tst files like the Makefiles in octave core. (this solves #41298)
- The “pkg install” shall copy any src/*-tst and src/*.tst files into the package's target directory.
- The requested “pkg test” can be implemented via pkg load + _run_test_suite_
- Bonus target: The test.m script shall lookup <functionname>.cc-tst files, for simple testing of a single installed OCT-file.

I can provide a patch, if you agree with the proposal.

Oliver Heimlich <oheim>
Wed 22 Jan 2014 06:19:08 AM UTC, comment #7: 

Re comment #6, I went ahead and submitted another bug for these two additional suggestions about extracting and installing unit tests from .cc files and fixed .tst scripts. This bug doesn't really depend on that being done first, but it does mean that "pkg test foo" will only test m-files until all tests are available in the package directory.

See bug #41298 for the new bug about extracting and installing tests from "pkg install foo".

Mike Miller <mtmiller>
Group Member
Mon 13 Jan 2014 02:35:46 PM UTC, comment #6: 

Indeed having 'pkg test <package name>' would be useful.

The implementation of this feature should be synchronized with the overhaul of "pkg.m" being carried out by JPi and Carnë.

In addition to calling _run_test_suite_ passing the correct
location of the package installation directory, this would require:

1) extract tests from .cc files and store them in he installed package

2) copy directories with fixed test scripts from the tarball to the install location.

c.

Carlo de Falco <cdf>
Group Member
Mon 13 Jan 2014 02:16:27 PM UTC, comment #5: 


> The major differences between runtests and run_test_suite
> are runtests shows errors in the console, does not create
> a fntests.log file, and it lists all function files that
> are missing unit tests.


Other important differences:

  • _run_test_suite_ display a summary (number of tests, number of  failed tests, etc.) at the end


  • _run_test_suite_ also executes tests which are contained in subdirectories



Julien Bect <jbect>
Mon 13 Jan 2014 02:14:53 PM UTC, comment #4: 


> The function expects the inputs to be passed as
> cell arrays of strings while you are using just
> a string.
>
> You should enclose directory paths in curly braces.


Ok, the following does the job:


pkg load signal
cd ~/octave/signal-1.2.2
__run_test_suite__ ({pwd}, {})


but I agree with Mike that it would be better to be able to do this without having to know where the package is.

Julien Bect <jbect>
Mon 13 Jan 2014 02:05:07 PM UTC, comment #3: 

Yes, both runtests and run_test_suite work given the right paths, but I agree it would be a nice feature to be able to test a package with the pkg command without having to specify paths. The pkg commands already have code for adding the directories of a package to the load path, a new "pkg test" command could make sure the package is loaded and call runtests on each of the directories of the package.

The major differences between runtests and run_test_suite are runtests shows errors in the console, does not create a fntests.log file, and it lists all function files that are missing unit tests.

Mike Miller <mtmiller>
Group Member
Mon 13 Jan 2014 01:49:22 PM UTC, comment #2: 

The function expects the inputs to be passed as cell arrays of strings while you are using just a string.
You should enclose directory paths in curly braces.
c.

Carlo de Falco <cdf>
Group Member
Mon 13 Jan 2014 12:46:34 PM UTC, comment #1: 


> To test all functions in a package I use the internal
> function _run_test_suite_ passing as first argument
> the package install directory.
>
> Does that work better for you?
>
> c.


I have tried but it fails, perhaps am I missing something...

Here is what I do, with Octave 3.8.0:


pkg load signal
cd ~/octave/signal-1.2.2
__run_test_suite__ (pwd)


and I get:


Integrated test scripts:

sq_string cannot be indexed with {



Julien Bect <jbect>
Mon 13 Jan 2014 09:15:49 AM UTC, original submission:  

It would be nice to a "test" option for the pkg command, that would allow to run the test suite of a given package:

pkg test PackageName

similarly to _run_test_suite_ for Octave core.

See the discussion here: http://octave.1599824.n4.nabble.com/Removing-packages-from-Octave-Forge-td4660799i20.html#a4660902

Julien Bect <jbect>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #48167:  octave-bug-41215-v4.cset added by philipnienhuis (3KiB - application/octet-stream)
file #48137:  octave-bug-41215-v3.cset added by philipnienhuis (2KiB - application/octet-stream)
file #48124:  octave-bug-41215-v2.cset added by philipnienhuis (2KiB - application/octet-stream)
file #48118:  bug_41215_v2_PN.cset added by philipnienhuis (1KiB - application/octet-stream)
file #33130:  octave-bug-41215.patch added by oheim (2KiB - text/x-patch)

 

Carbon-Copy List
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by cbm (Posted a comment)
  • -email is unavailable- added by avinoam (Updated the item)
  • -email is unavailable- added by oheim (Posted a comment)
  • -email is unavailable- added by cbm
  • -email is unavailable- added by svillemot
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by jbect (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 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-02-16 mtmiller StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2020-01-05 philipnienhuis Attached File- Added octave-bug-41215-v4.cset, #48167
    2020-01-05 philipnienhuis Assigned toNone philipnienhuis
    2019-12-29 philipnienhuis Attached File- Added octave-bug-41215-v3.cset, #48137
    2019-12-27 philipnienhuis Attached File- Added octave-bug-41215-v2.cset, #48124
    2019-12-27 philipnienhuis Dependencies- Depends on bugs #57489
        Dependencies- Depends on bugs #41298
    2019-12-25 philipnienhuis Attached File- Added bug_41215_v2_PN.cset, #48118
    2019-02-18 mtmiller Carbon-CopyRemoved 80942 -
    2017-08-13 jwe CategoryOctave Package Octave Function
    2016-06-30 avinoam StatusConfirmed Patch Submitted
    2015-02-19 oheim Attached File- Added octave-bug-41215.patch, #33130
    2014-10-18 cbm Carbon-Copy- Added cbm
    2014-02-05 svillemot Carbon-Copy- Added svillemot
    2014-01-13 mtmiller Severity3 - Normal 1 - Wish
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code