Patch to stop counting missing tests on internal functions

[Rik-3]

10/8/09

All,

Currently when a user runs 'make check' there is a report which shows a
large number of files that don't have tests written for them and then
invites the user to contribute tests.  On looking through the list,
however, there are about 60 functions which are for internal use only as
designated by the prefix and suffix "__".   An example is __contourc__.cc

My proposed patch to fntests.m would remove these from the report of
functions missing tests. (This is *only* about reporting.  If tests for
internal functions have been written they would still continue to be
run.)  First, if these internal functions need coverage they should get
it from tests written by the function which calls them.  In the example
given, there is already a test in contourc.m which presumably would
catch an error in the internal function.  Secondly, if a user does have
a few spare moments to write a test for a function we will get more
value for the coding effort if they concentrate on functions that people
might use all the time rather than internal ones.

Thoughts?  If there are no strong objections I'll apply the patch in a
few days.

Cheers,
Rik

diff -r f8e2e9fdaa8f test/fntests.m
--- a/test/fntests.m Thu Oct 08 16:43:05 2009 +0200
+++ b/test/fntests.m Thu Oct 08 20:56:04 2009 -0700
@@ -145,7 +145,11 @@
  dsk += sk;
  files_with_tests(end+1) = f;
       else
- files_with_no_tests(end+1) = f;
+ ## Don't count internal functions which are, or should be,
+        ## covered by tests in the functions which call them
+ if (! (length (nm) > 3 && strcmp (nm(1:2), "__")) )
+  files_with_no_tests(end+1) = f;
+ endif
       endif
     endif
   endfor

[John W. Eaton-3]

On  9-Oct-2009, Rik wrote:

| Currently when a user runs 'make check' there is a report which shows a
| large number of files that don't have tests written for them and then
| invites the user to contribute tests.  On looking through the list,
| however, there are about 60 functions which are for internal use only as
| designated by the prefix and suffix "__".   An example is __contourc__.cc
|
| My proposed patch to fntests.m would remove these from the report of
| functions missing tests. (This is *only* about reporting.  If tests for
| internal functions have been written they would still continue to be
| run.)  First, if these internal functions need coverage they should get
| it from tests written by the function which calls them.  In the example
| given, there is already a test in contourc.m which presumably would
| catch an error in the internal function.  Secondly, if a user does have
| a few spare moments to write a test for a function we will get more
| value for the coding effort if they concentrate on functions that people
| might use all the time rather than internal ones.
|
| Thoughts?  If there are no strong objections I'll apply the patch in a
| few days.

I'm not sure that we should stop testing all functions named this way.
The naming scheme was adopted because Octave did not have private
functions or subfunctions, so the functions with names of the form
__X__ which really should be private functions or subfunctions should
be converted.  Then they won't be tested because you can't call
private functions or subfunctions directly, and we could ensure that
the test script does not search private subdirectories.  Then we would
be left with functions that are "internal" in the sense that they do
something special that is not normally useful for programming, but are
occasionally used for debugging or some other purpose.  I'm thinking
of functions like __dump_symtab_info__.

jwe

[John W. Eaton-3]

On 13-Oct-2009, Jaroslav Hajek wrote:

| I would also like to see more private functions become really private;

I would also like to see this happen, but it will mean some changes in
the build system, and as I'm currently working on converting the
Octave build system to use automake (I should have something to show
soon) I'd like to delay any such change until after the automake
changes are done.

| one problem I see is that a number of __helper__ functions are
| compiled codes, which are all bundled in one directory in the current
| Octave's binary files organization.
| Regarding the test log, it is surely not realistic to expect every
| source file to have tests; the list in fntests.log includes assistant
| sources, such as oct-locbuf.cc, for which no test is really
| meaningful, or even auto-generated sources, such as mx-cm-dm.cc.
| Maybe C++ files that are expected to have tests some day can be marked
| as such, and only those files listed.

How about only noting the C++ source files that have DEFUNs?

jwe

[Jaroslav Hajek-2]

On Tue, Oct 13, 2009 at 11:56 AM, John W. Eaton <jwe@...> wrote:

> On  9-Oct-2009, Rik wrote:
>
> | Currently when a user runs 'make check' there is a report which shows a
> | large number of files that don't have tests written for them and then
> | invites the user to contribute tests.  On looking through the list,
> | however, there are about 60 functions which are for internal use only as
> | designated by the prefix and suffix "__".   An example is __contourc__.cc
> |
> | My proposed patch to fntests.m would remove these from the report of
> | functions missing tests. (This is *only* about reporting.  If tests for
> | internal functions have been written they would still continue to be
> | run.)  First, if these internal functions need coverage they should get
> | it from tests written by the function which calls them.  In the example
> | given, there is already a test in contourc.m which presumably would
> | catch an error in the internal function.  Secondly, if a user does have
> | a few spare moments to write a test for a function we will get more
> | value for the coding effort if they concentrate on functions that people
> | might use all the time rather than internal ones.
> |
> | Thoughts?  If there are no strong objections I'll apply the patch in a
> | few days.
>
> I'm not sure that we should stop testing all functions named this way.
> The naming scheme was adopted because Octave did not have private
> functions or subfunctions, so the functions with names of the form
> __X__ which really should be private functions or subfunctions should
> be converted.  Then they won't be tested because you can't call
> private functions or subfunctions directly, and we could ensure that
> the test script does not search private subdirectories.  Then we would
> be left with functions that are "internal" in the sense that they do
> something special that is not normally useful for programming, but are
> occasionally used for debugging or some other purpose.  I'm thinking
> of functions like __dump_symtab_info__.
>
> jwe
>

I would also like to see more private functions become really private;
one problem I see is that a number of __helper__ functions are
compiled codes, which are all bundled in one directory in the current
Octave's binary files organization.
Regarding the test log, it is surely not realistic to expect every
source file to have tests; the list in fntests.log includes assistant
sources, such as oct-locbuf.cc, for which no test is really
meaningful, or even auto-generated sources, such as mx-cm-dm.cc.
Maybe C++ files that are expected to have tests some day can be marked
as such, and only those files listed.

--
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

[Rik-3]

John W. Eaton wrote:

> On 13-Oct-2009, Jaroslav Hajek wrote:
>
> | I would also like to see more private functions become really private;
>
> I would also like to see this happen, but it will mean some changes in
> the build system, and as I'm currently working on converting the
> Octave build system to use automake (I should have something to show
> soon) I'd like to delay any such change until after the automake
> changes are done.
>
> | one problem I see is that a number of __helper__ functions are
> | compiled codes, which are all bundled in one directory in the current
> | Octave's binary files organization.
> | Regarding the test log, it is surely not realistic to expect every
> | source file to have tests; the list in fntests.log includes assistant
> | sources, such as oct-locbuf.cc, for which no test is really
> | meaningful, or even auto-generated sources, such as mx-cm-dm.cc.
> | Maybe C++ files that are expected to have tests some day can be marked
> | as such, and only those files listed.
>
> How about only noting the C++ source files that have DEFUNs?
>  

Maybe we are going about this the wrong way.  The current script
searches for all *.cc and *.m files and then checks whether there there
are any test features (%!test, %!assert, %!error, %!warning) in the
file.  This check is too simplistic and the results are both overly
pessimistic and overly optimistic.  First, they are overly pessimistic
because they seem to show that a lot of files are undocumented, but most
of these files do not contain user functions and instead are helping
code or auto-generated code.  Secondly, they are overly optimistic about
coverage because even a single test for a single function will count the
entire file as being covered.  The file utils.cc has 12 functions in it
and it would be useful to know which ones actually had coverage.

What if we start with the list of functions visible to Octave (using
calls to __builtins__ and __list_functions__) and then cycle through
that list trying to call any test functions?  If there are no tests then
the test function returns something to indicate that and we can mark the
function as needing work.

--Rik
> jwe
>
>  

[John W. Eaton-3]

On 14-Oct-2009, Rik wrote:

| Maybe we are going about this the wrong way.  The current script
| searches for all *.cc and *.m files and then checks whether there there
| are any test features (%!test, %!assert, %!error, %!warning) in the
| file.  This check is too simplistic and the results are both overly
| pessimistic and overly optimistic.  First, they are overly pessimistic
| because they seem to show that a lot of files are undocumented, but most
| of these files do not contain user functions and instead are helping
| code or auto-generated code.  Secondly, they are overly optimistic about
| coverage because even a single test for a single function will count the
| entire file as being covered.  The file utils.cc has 12 functions in it
| and it would be useful to know which ones actually had coverage.
|
| What if we start with the list of functions visible to Octave (using
| calls to __builtins__ and __list_functions__) and then cycle through
| that list trying to call any test functions?  If there are no tests then
| the test function returns something to indicate that and we can mark the
| function as needing work.

Searching for .m files seems reasonable because there is one
user-visible function per file.  But yes, we often have more than one
DEFUN in a single .cc file, so I agree that it would be better to do
tests for each DEFUN instead of for each .cc file.  However, we
currently have no way to tag a test as being for a particular
function, so I don't see how to do what you propose.

jwe