bugGNU Octave - Bugs: bug #41298, pkg install: automatically extract...

 
 

bug #41298: pkg install: automatically extract %!tests and install both extracted and fixed test files

Submitted by:  Mike Miller <mtmiller>
Submitted on:  Wed 22 Jan 2014 06:14:57 AM UTC  
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Patch Submitted Assigned to:  Philip Nienhuis <philipnienhuis>
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 14 Apr 2020 08:02:11 PM UTC, comment #37: 

Ping .....

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 16 Feb 2020 10:28:03 PM UTC, comment #36: 

I'll try to find some time to review and test this patch as well in the next few days.

Mike Miller <mtmiller>
Project Administrator
Sat 11 Jan 2020 01:25:21 PM UTC, comment #35: 

Anything else needed to get installed %!tests into octave 6 ?

John Donoghue <lostbard>
Project Member
Mon 06 Jan 2020 05:12:48 PM UTC, comment #34: 
John Donoghue <lostbard>
Project Member
Mon 06 Jan 2020 04:16:16 PM UTC, comment #33: 

Yeah - I have some updates for mxe - just hadn't pushed them yet

John Donoghue <lostbard>
Project Member
Mon 06 Jan 2020 04:07:15 PM UTC, comment #32: 

@JohnD:
Thanks again for testing.
I think some action in mxe-octave is required to get the test files in the arch-dependent subdir. Apparently pkg.m is bypassed when building the binary packages? Or is an installed and up-to-date Linux Octave installation required to do help here?

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 05 Jan 2020 06:44:04 PM UTC, comment #31: 

Looks good to me

I installed io from forge both as a local and global package and then ran the tests

dev dicom works (.cpp test files)

forge zeromq works (.cc test files)

forge instrument-control (no main src test files)

dev statistics - no src files

forge blstl - no tests

arduin - no src, depends on instrument control

All these appear to install ok and run tests ok

A few now have double tests since they were being installed from the makefile - Ive started putting a check in the makefile to only install on older versions of octave on the pkgs Ive been maintaining

John Donoghue <lostbard>
Project Member
Sun 05 Jan 2020 12:58:35 PM UTC, comment #30: 

Together with octave-bug-41215-v4.cset in bug #41215 I think the 'pkg test' feature should be complete now.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 05 Jan 2020 12:32:53 PM UTC, comment #29: 

Yep, that's a practical outcome of the new 'pkg tst' feature :-)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 05 Jan 2020 12:23:40 AM UTC, comment #28: 

I guess those tests can avoid flashing plots by setting visible off to the figure before plotting.

Juan Pablo Carbajal <juanpi>
Project Member
Sat 04 Jan 2020 09:22:35 PM UTC, comment #27: 

... and looking closer, it is plotShape.m in geometry that indeed has some funny tests:

%!shared s
%! s = {[0.1 1; 1 0]};

%!test
%! plotShape (s); close;
%!test
%! plotShape (s,'tol', 1e-4);close;
%!test
%! plotShape (s,'color', 'm', 'tol', 1e-4);close;
%!test
%! plotShape (s,'color', 'm', 'linewidth', 2, 'tol', 1e-4);close;
%!test
%! plotShape (s,'color', 'm', 'tol', 1e-4, 'linewidth', 2);close;

Well, sorry for the noise, I (vitually) retract my last postings.

In effect this means that my latest patch here should be ready for review.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 04 Jan 2020 09:17:33 PM UTC, comment #26: 

Hmmm, maybe I'm confused.

"pkg test geometry" shows lot of graphics figures flashing by, that's why I figured "Ah, so the demos get run as well". IOW I assumed I overlooked a few things.

But what you say about __run_test_suite__.m makes sense. So it may well be that some geometry functions merely have tests with visible rather than invisible figures.

Anyway geometry's inst/ subdir has quite a few demos, to wit:

$ grep "%\!demo" *.m
beltProblem.m:%!demo
clipPolygon.m:%!demo
clipPolygon_clipper.m:%!demo
clipPolygon_mrf.m:%!demo
clipPolygon_mrf.m:%!demo
clipPolyline.m:%!demo
cov2ellipse.m:%!demo
curve2polyline.m:%!demo
data2geo.m:%!demo
data2geo.m:%!demo
ellipse2cov.m:%!demo
plotShape.m:%!demo
polygon2patch.m:%!demo
polygon2patch.m:%!demo
polyjoin.m:%!demo
shape2polygon.m:%!demo
shapeArea.m:%!demo # non-convex piece-wise polynomial shape
shapeCentroid.m:%!demo # non-convex bezier shape
shapeCentroid.m:%!demo
simplifyPolygon_geometry.m:%!demo
simplifyPolyline_geometry.m:%!demo
simplifyPolyline_geometry.m:%!demo
transformShape.m:%!demo

philip@DeskPRN MSYS /f/dev/of/hg/geometry-dev/inst

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 04 Jan 2020 07:29:52 PM UTC, comment #25: 

> should extract all lines that begin with "$!" into the -tst files


I meant "should extract all lines that begin with "%!" into the -tst files".

On bug #41215, you said that "the csets here and in bug #41298 also run demos". I haven't tested it yet, but your patch uses "__run_test_suite__", which I don't think runs demos, so now I'm not sure you were right about that to begin with, please clarify.

Mike Miller <mtmiller>
Project Administrator
Sat 04 Jan 2020 07:12:46 PM UTC, comment #24: 

Hold on, can you explain something to me?

If you look in Octave's libinterp/corefcn/ellipj.cc-tst, that file contains both %!test and %!demo blocks. I believe that this bug, about extracting tests, should extract all lines that begin with "$!" into the -tst files, just like Octave.

Now if the file contains both %!test and %!demo blocks, running "pkg test" should normally only run the %!test blocks, not the %!demo blocks. This is the normal way of operating on tests and shouldn't be different in bug #41215, which is why I brought it up there.

1. I think "-tst" files should contain %!demo blocks.

2. I think "pkg test foo" should not run %!demo blocks by default. Maybe with an option, just like "test line" vs "test line verbose". That could be a future enhancement.

Mike Miller <mtmiller>
Project Administrator
Sat 04 Jan 2020 02:42:56 PM UTC, comment #23: 

Over in bug #41215I I mentioned that the current code also extracts demos. Mike thinks that isn't desirable.

It can be a little complicated to only single out %!demo blocks.
Rather than accept all lines starting with "%!" we now have to check for "%!demo ..." and if found skip until we find "%!test" / "%!warning ..." / "%!error ..." / "%!<whatever>...." lines.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 29 Dec 2019 02:27:00 PM UTC, comment #22: 

Using v6, instrument-control now installs as expected.

Also tested dicom (has cpp tests), statistics (src but no source files), zeromq (has cc tests), general (has cc test) - all install ok with associated tests extracted and installed.

John Donoghue <lostbard>
Project Member
Sat 28 Dec 2019 09:28:50 PM UTC, comment #21: 

Ah, generated .cc files ...

Again a new cset version attached (v6). I moved the extract code to almost the very end of the main function of configure_make.m.

(file #48134)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 28 Dec 2019 04:56:52 PM UTC, comment #20: 

For statistcs, I was using the repo code which has the linkage fix in it.

For sources other than the base 'src', There are probally a number of issues that would need be looked - and not really part of the scope of this bug anyway.

For the configure_make question, looks like make is is the last block of functionality taht happens in the function (line 121), so my thought was just moving the test extraction to after that, and it would then see any source files generated from make.

John Donoghue <lostbard>
Project Member
Sat 28 Dec 2019 04:06:42 PM UTC, comment #19: 

Hmm bad hunk.
New try (v5)

(file #48130)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 28 Dec 2019 03:50:28 PM UTC, comment #18: 

Thanks for testing.

A test for emptyness would be a first prerequisite, indeed :-) I should have thought of it.

I tried tracking what happens during execution of <pkg>/private/configure_make.m.
Building the .oct files happens in ./src but completely outside the scope of configure_make.m itself; that just launches a shell in which make does its thing, configure_make.m remains unaware that there may be subdirs containing .cc files. After make did its thing there are just .oct files in the temp src/ directory (which is a prerequisite of pkg.m, it expects the binary modules files there.)

As to statistics, statistics-1.4.1 doesn't install here due to an illegal "=" operator in an anonymous function in linkage.m.

Luckily instrument-control generates its own .cc-tst files.
But for other packages where .cc files live in subdirs of src/ some tricks are needed. Maybe iterating over all subdirs except m4/ and autom4te.cache/ would do it? Seems fragile to me nevertheless. Looking around on SF my perception is that such package are rare anyway.
My stance is to just leave the complications of complicated packages to their respective maintainers and not bother here. That is, not at this moment, but maybe for Octave-7.x or perhaps by that time Andrew's packajoozle has taken over.
I just want to help implement a very desirable feature for pkg.m that would work for the vast majority of OF packages, hopefully in time for Octave-6.1.0. It already helped to uncover bugs in the two packages I maintain.

Updated patch with emptyness check attached.

(file #48129)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 28 Dec 2019 01:50:35 PM UTC, comment #17: 

Works on dicom that  has .cpp files with tests, zeromq which has .cc files with tests.

On instrument control, which doesnt have any source files in the main src directory I get:
error: structure has no member 'name'
error: called from
    configure_make at line 99 column 8
    install at line 190 column 7
    pkg at line 480 column 9

-> need to check whether there any  files before iterating the file.name

Note that instrument control has no sources in the base src directory.

statistics also has a source directory, but doesnt have source files in it - it just runs configure so work out what needs be installed, so the empty check would be needed there as well.

In the case of instrument control, it does create a generated .cc file with all the package adds in it (since pkg install doesnt install from sub dirs) so perhaps the test extraction should go after the make  ? That way if anyone has generated sources with tests in them, they would get installed ??

John Donoghue <lostbard>
Project Member
Fri 27 Dec 2019 10:03:55 PM UTC, comment #16: 

OK, I've made a new cset along the suggestions of JohnD and Mike. Please review.

I've simply amended the pkg code such that all files ending in 'tst' will be copied to the arch-dependent directory, and amended test.m such that it processes all files ending in 'tst'.

I also made a new cset for the very related bug #41215 (allow pkg.m to run pkg tests) that is based on _run_tst_suite_.m and that I'll upload in a moment. See discussion there.

(file #48123)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Fri 27 Dec 2019 01:12:13 AM UTC, comment #15: 

IMHO, it would make the most sense to extract and install -tst files from src files into the arch-dependent directory, alongside the corresponding .oct files. This would mirror how Octave installs its own -tst files in subdirectories like 'libinterp/corefcn' under 'etc/tests' to match the original source file layout.

Mike Miller <mtmiller>
Project Administrator
Fri 27 Dec 2019 12:20:27 AM UTC, comment #14: 

Well just getting the tests there is a large improvment, no matter where they go.

As part of 'pkg test', it could call the tests - whether they end up.

John Donoghue <lostbard>
Project Member
Thu 26 Dec 2019 09:10:21 PM UTC, comment #13: 

Getting the "*tst" files into a separate test/ subdir is going to be a messy affair, see L.100-118 in (patched) scripts/pkg/private/configure_make.m.
Of course it can be done but the code isn't going to be pretty.

Alternatively another rule + copy "if" block + subfunction to isolate *tst files can be made in scripts/pkg/private/install.m (L.426 and up).

Just dumping all *tst files into the m-file subdir (arch-independent) is the easiest ATM.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 26 Dec 2019 08:43:43 PM UTC, comment #12: 

Good suggestions, thanks, I'll adapt the patches.

oruntests.m works in only one specified directory and (AFAICS) not its subdirs. Itsoutput is minimal but sufficient (IMO).
__run_test_suite__.m also processes subdirs but gives a lot of other output AND creates an fntests.log file for each run.

So I can either:
Adapt the first patch to simply move the *-tst files into the .m-file subdir, as that'll make it easier to implement a "pkg test"  option that processes the entire package (see bug #41215) using oruntests.m;
-or-:
Move the *-tst files into a "test"subdir and invoke __run_test_suite__.m to run the tests.

Maybe we'll just need to experiment.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 26 Dec 2019 01:12:10 PM UTC, comment #11: 

For comment 7/8, the oruntests change only looks for .cc-tst files as well.

For reference,  _run_test_suite_ runs any file that ends with -tst or .tst

John Donoghue <lostbard>
Project Member
Thu 26 Dec 2019 12:58:39 PM UTC, comment #10: 

For the tests Ive been installing, I usually create a folder in inst called 'test' that contains those files  - which at least separates them from the main code, which at the time seemed a good idea.

But I dont really have an opinion one way or the other.

John Donoghue <lostbard>
Project Member
Thu 26 Dec 2019 12:52:34 PM UTC, comment #9: 

Hmmm, a question is whether we want the .cc-tst files in the arch-independent subdir (.m-files, share/octave/.../<package>/ or in the arch-dependent one (.oct-files, lib/octave/.../<package>/).

In case of arch-independent:

  • (patched) oruntest.m could run all package tests with just one command, rather than calling it separately for each directory (arch-independent and arch-dependent);
  • output of tests for binary modules will be interspersed with tests for .m-file functions;
  • the arch-independent subdir is cluttered with .cc-tst files.

In case of arch-dependent:

  • the .cc-tst files are kept together with the module they belong to;
  • binary modules could be tested separately from .m-files, which might be desirable for better overview.

I have no clear opinion.
ATM my patch in comment #6 is for keeping .cc-tst files in the arch-dependent subdir. I did that because I felt it better to keep tests with the files they belong to; but later on I figured it may be better to be able to test all package functions with just one call to oruntests.m.
Changing this is a mere one-liner.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 26 Dec 2019 12:49:27 PM UTC, comment #8: 

That would be 'isnt quite' working

John Donoghue <lostbard>
Project Member
Thu 26 Dec 2019 12:48:48 PM UTC, comment #7: 

I tried it and the dicom package which  has .cpp files, and it its qute working:

It creates -tst files from .c, .C, .cc, and ,cpp files ....

however in copy_built_files it only install .cc-tst files:

tst = dir (fullfile (src, "*.cc-tst"));

Perhaps that could be *-tst"  to cover any combination of file that may be a test file, or need to spefify a separate filter for each type

John Donoghue <lostbard>
Project Member
Wed 25 Dec 2019 06:39:26 PM UTC, comment #6: 

My pleasure, John.

Attached is an updated patch relative to dev ~23 Dec '19.
I had to adapt some code by Oliver/Lachlan as strcat() wrangles the test output files to something like:

## DO NOT EDIT!%!test
<rest of test code>

rather than

## DO NOT EDIT!  Generated automatically .....
<rest of test code>

What this cset does:

  • extract tests out of *.cc files
  • copies them into similarly named *.cc-tst files
  • copies them over to the archdependent subdir.

Also attached is a cset for oruntests.m that can run tests in *.cc-tst files.
With that fix there's at least some way that tests in .cc-tst files in archdependent package subdirs of installed packages can be executed.
I was looking into the patch in bug #41215 comment #11 (typo in my comment #4 below) but that one gives very clumsy output. I'll comment there a little later.

(file #48116, file #48117)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Wed 25 Dec 2019 05:26:18 PM UTC, comment #5: 

In most of the packages I have been maintaining, I've been adding a makerule that extracts the tests and installs them as part of the package.

This will make it so I dont have to do that which will be nice!

John Donoghue <lostbard>
Project Member
Wed 25 Dec 2019 02:12:14 PM UTC, comment #4: 

Picking up as I find it undesirable that tests in .oct files can't be run after "pkg install ..." and then I accidentally found this bug report.

The current patch (comment #3) creates the .cc-tst files but only in the temp directory used by "pkg install ..." and doesn't copy them over.
I'll try to fix this, I suppose the .cc-tst files will end up in the archdependent directory where I hope the related patch from bug #4115 comment #11 will find them.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Fri 01 Jul 2016 12:21:32 AM UTC, comment #3: 

Oliver, I've made the changes that I suggested.  However there were a few questions in my previous comment.  If you have time, could you please reply to them?

(file #37638)

Lachlan Andrew <lachlan>
Thu 30 Jun 2016 12:41:27 PM UTC, comment #2: 

Oliver, I've had a quick look at the code, and have some initial comments.  I'll try to review it properly soon.

Why do you not overwrite existing tests?  ("if (exist (full_tst_file)), continue, endif")  This may make it hard for someone who is trying to tweak the tests.  Most make processes just overwrite existing files.

I like it when automatically generated files say both the source they were generated from and also the program that did the generation.

In the change to test.m, why do you add ".cc-tst" but not ".tst"?

The commit message should start with a summary line that doesn't start with "* <filename>", but summarises the change and mentions the bug number.  This also applies to the patch for bug #41215.

Some trivial points are:
- the first hunk has indentation of four spaces instead of two.
- When {...} is used to denote a subscript, I think the coding standard says there shouldn't be a space before {.
- Why do you use "not ()" instead of "!"?

Lachlan Andrew <lachlan>
Thu 19 Feb 2015 01:13:30 PM UTC, comment #1: 

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

(file #33131)

Oliver Heimlich <oheim>
Wed 22 Jan 2014 06:14:57 AM UTC, original submission:  

When Forge packages are built and installed using the pkg command, %!tests in source files should be extracted and installed along with the rest of the package. Currently, only the compiled .oct files are copied into the package install location, so any %!tests that are in .c or .cc files are not available at runtime. These tests should be extracted into tst files.

Secondly, any additional tests that are not necessarily associated with a particular class or function could be maintained in .tst files. Taken together, both extracted tst files and maintained fixed .tst scripts should be installed into the package directory so they can be run any time, just like tests in m-files.

This brings the handling of tests in Forge packages more in line with what Octave now does with its own %!tests.

This feature request is based on Carlo's comments in bug #41215, which doesn't strictly depend on this being fixed first, but will certainly benefit from having more tests runnable after a package is installed.

Mike Miller <mtmiller>
Project Administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #48134:  bug_41298_pkg_install_tests.v6.cset added by philipnienhuis (4KiB - application/octet-stream)
file #48130:  bug_41298_pkg_install_tests.v5.cset added by philipnienhuis (4KiB - application/octet-stream)
file #48129:  bug_41298_pkg_install_tests.v4.cset added by philipnienhuis (4KiB - application/octet-stream)
file #48123:  bug_41298_pkg_install_tests.v3.cset added by philipnienhuis (4KiB - application/octet-stream)
file #48116:  bug_41298_extract_tests.cset added by philipnienhuis (4KiB - application/octet-stream)
file #48117:  oruntests.m_cc-tst.cset added by philipnienhuis (868B - application/octet-stream)
file #33131:  octave-bug-41298.patch added by oheim (3KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by juanpi (Posted a comment)
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by apjanke
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by oheim (Updated the item)
  • -email is unavailable- added by svillemot
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-12-28 philipnienhuis Attached File- => Added bug_41298_pkg_install_tests.v6.cset, #48134
    2019-12-28 philipnienhuis Attached File- => Added bug_41298_pkg_install_tests.v5.cset, #48130
    2019-12-28 philipnienhuis Attached File- => Added bug_41298_pkg_install_tests.v4.cset, #48129
    2019-12-27 philipnienhuis Dependencies- => bugs #41215 is dependent
    2019-12-27 philipnienhuis Attached File- => Added bug_41298_pkg_install_tests.v3.cset, #48123
    2019-12-25 philipnienhuis Attached File- => Added bug_41298_extract_tests.cset, #48116
        Attached File- => Added oruntests.m_cc-tst.cset, #48117
        StatusIn Progress => Patch Submitted
    2019-12-25 philipnienhuis StatusPatch Submitted => In Progress
        Assigned toNone => philipnienhuis
    2019-02-26 mtmiller Carbon-CopyRemoved 80942 => -
    2019-02-07 apjanke Carbon-Copy- => Added floss@apjanke.net
    2016-07-01 lachlan Attached File- => Added bug_41298_pkg_install_tests.v1.cset, #37638
    2016-06-29 lachlan StatusNone => Patch Submitted
    2015-02-19 oheim Attached File- => Added octave-bug-41298.patch, #33131
    2014-02-05 svillemot Carbon-Copy- => Added svillemot

    Back to the top


    Powered by Savane 3.5