Bug 8244 - Script to find exporter problems
Summary: Script to find exporter problems
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-14 09:54 UTC by Julian Maurice
Modified: 2018-10-09 20:06 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8244: New script xt/find-undefined-subroutines.pl (13.83 KB, patch)
2012-06-14 10:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8244: New script xt/find-undefined-subroutines.pl (14.46 KB, patch)
2012-06-20 11:25 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8244: New script xt/find-undefined-subroutines.pl (14.58 KB, patch)
2012-06-27 15:11 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8244 follow-up: improvements to find-undefined-subroutines (2.87 KB, patch)
2012-06-27 15:11 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8244 follow-up: improvements to find-undefined-subroutines (2.86 KB, patch)
2012-07-18 13:35 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8244 follow-up: improvements to find-undefined-subroutines (2.92 KB, patch)
2012-08-03 17:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8244: Add PPI to the list of Perl dependencies (751 bytes, patch)
2012-09-14 14:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8244: Add PPI to the list of Perl dependencies (753 bytes, patch)
2012-11-02 13:15 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8244: Add PPI to the list of Perl dependencies (820 bytes, patch)
2013-01-04 13:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8244: Add PPI to the list of Perl dependencies (801 bytes, patch)
2014-04-04 17:55 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2012-06-14 09:54:29 UTC
About one month ago, we (BibLibre) discover a bug in our source code. The problem was: a page that used to work correctly suddenly refuse to work because of 
"Undefined subroutine &get_infos_of at C4/Items.pm (line 1075)" (approximative message) in reserve/request.pl
The piece of code in cause was a "use C4::Members" in C4/Budgets.pm. A modification of C4::Budgets was causing reserve/request.pl to crash... (ouch!)
In fact it introduced a circular dependency.

To avoid this kind of discovery, we decide to write a script that try to detect as many problems as possible. Patch and description follow.
Comment 1 Julian Maurice 2012-06-14 10:18:57 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-06-14 10:35:25 UTC
Note: this require to have $KOHA_CONF to be correctly set ($PERL5LIB is not needed)
Comment 3 Jared Camins-Esakov 2012-06-15 17:31:13 UTC
I am not sure about using requiring arguments to an xt/ test. Would it be possible to have it automatically know that kohaclone is in dirname($0)/.. and then use tempdir for a destination directory? If we did that, then this test could safely be included in Koha?

Also, much more significantly, this script does not work for me. I ran the following command:
> perl xt/find-undefined-subroutines.pl --src-path=/home/jcamins/kohaclone --dest-path=/home/jcamins/kohatmp -v

It copied everything into /home/jcamins/kohatmp/home/jcamins/kohaclone, then gave the following error:
cp: cannot create regular file `/home/jcamins/kohatmp/C4/Context.pm': No such file or directory
sh: cannot create /home/jcamins/kohatmp/misc/kohalib.pl: Directory nonexistent
Unknown option: trap
Unable to continue at /usr/bin/prove line 10
Comment 4 Julian Maurice 2012-06-20 11:25:58 UTC Comment hidden (obsolete)
Comment 5 Jared Camins-Esakov 2012-06-25 16:04:47 UTC
It looks good, except for one thing. When I run `prove xt` it does not get caught since the extension is .pl unstead of .t. Perhaps it would be better to call it find-undefined-subroutines.t?
Comment 6 Julian Maurice 2012-06-27 10:10:04 UTC
This script takes much time. I don't think we want to launch it every time with `prove xt`. Although, there is already scripts in xt/ that are not ".t" (yaml_valid.pl for example).

Moreover, this script is not a test script as it does not use Test::More, but build test scripts and run prove on them. Running prove on this script will fail.
Comment 7 Jared Camins-Esakov 2012-06-27 13:48:29 UTC
(In reply to comment #6)
> This script takes much time. I don't think we want to launch it every time
> with `prove xt`. Although, there is already scripts in xt/ that are not ".t"
> (yaml_valid.pl for example).
> 
> Moreover, this script is not a test script as it does not use Test::More,
> but build test scripts and run prove on them. Running prove on this script
> will fail.

Fair enough. Sign off to follow. I will also provide a follow-up to add an argument so that we can tell the script to ignore certain files.
Comment 8 Jared Camins-Esakov 2012-06-27 15:11:00 UTC
Created attachment 10542 [details] [review]
[SIGNED-OFF] Bug 8244: New script xt/find-undefined-subroutines.pl

This script tends to detect if Koha scripts use subroutines that are not
correctly exported by modules. This can happen when we have circular
dependencies between modules.
This script does mainly two things:
 - It rebuilds the hierarchy of Koha Perl modules and replace all the code
   within subroutines by tests (using Test::More). For each subroutine called
   in a subroutine, a test is done on if the called subroutine is defined.
   If it's defined, then it's called (Tested subroutines are only those
   available in Koha Perl modules, not external modules subroutines).
   It does almost the same work with Koha Perl scripts, replacing all the
   code by tests and calling the subroutines if defined.
 - Launch prove on all created Perl scripts.
This results in a summary where failed tests are subroutines that are not
exported, but called without the module name prepended.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 9 Jared Camins-Esakov 2012-06-27 15:11:25 UTC Comment hidden (obsolete)
Comment 10 Jared Camins-Esakov 2012-06-27 15:13:07 UTC
The first patch has been signed off, but my follow-up still requires a sign-off.
Comment 11 Chris Cormack 2012-07-01 01:58:46 UTC
Follow up has an error if you call it without --ignore

Unmatched ) in regex; marked by <-- HERE in m/) <-- HERE / at xt/find-undefined-subroutines.pl line 151.

If you call it with a pattern like 
perl xt/find-undefined-subroutines.pl --ignore *.t

Unknown verb pattern '.t' in regex; marked by <-- HERE in m/(*.t) <-- HERE / at xt/find-undefined-subroutines.pl line 151.

It works if you call it with a pattern like C4

Id like to see the original patch pushed ASAP, so if this follow up could be fixed, or the original pushed and the new patch back to assigned to be fixed that would be excellent.
Comment 12 Julian Maurice 2012-07-18 13:35:52 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2012-08-03 17:20:37 UTC
PPI needs to be added as a dependency for this.
Comment 14 Kyle M Hall 2012-08-03 17:30:49 UTC
Created attachment 11346 [details] [review]
[SIGNED-OFF] Bug 8244 follow-up: improvements to find-undefined-subroutines

* Adds a repeatable --ignore option so that the user can choose to
  ignore certain files.
* Exit with the same success or failure code that the prove command
  returns

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Marcel de Rooy 2012-08-30 09:36:37 UTC
QA Comment:
If the user does not pass any pars, the script should not do anything (or show help..). This is currently not the case. Please correct.
Also please add the optional dependency of PPI to Dependencies.pm.
Comment 16 Jared Camins-Esakov 2012-08-30 10:56:05 UTC
(In reply to comment #15)
> QA Comment:
> If the user does not pass any pars, the script should not do anything (or
> show help..). This is currently not the case. Please correct.
> Also please add the optional dependency of PPI to Dependencies.pm.

It seems to me that there should be an exception for scripts in t/ and xt/, since these are test-related scripts, and the standard when running tests is to run without any arguments. Does that make sense?
Comment 17 Marcel de Rooy 2012-08-30 11:02:32 UTC
(In reply to comment #16)
> It seems to me that there should be an exception for scripts in t/ and xt/,
> since these are test-related scripts, and the standard when running tests is
> to run without any arguments. Does that make sense?

You do. But this seems to me a quite heavy script, creating lots of files in a temp dir. You do not know what it is doing unless you used -v. I would still argue for doing nothing here :)
Comment 18 Jared Camins-Esakov 2012-08-30 11:07:03 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > It seems to me that there should be an exception for scripts in t/ and xt/,
> > since these are test-related scripts, and the standard when running tests is
> > to run without any arguments. Does that make sense?
> 
> You do. But this seems to me a quite heavy script, creating lots of files in
> a temp dir. You do not know what it is doing unless you used -v. I would
> still argue for doing nothing here :)

Ah. Good point.
Comment 19 Julian Maurice 2012-09-14 14:51:15 UTC Comment hidden (obsolete)
Comment 20 Julian Maurice 2012-11-02 13:15:36 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-01-04 13:51:40 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2013-03-08 08:59:42 UTC
QA: Looking at this one now..
Comment 23 Marcel de Rooy 2013-03-08 10:08:33 UTC
QA Comment:
koha-qa is happy.
Optional dependency is added.
In the above comments there was a discussion if the script should start running if passed no parameters. For the record, I just note that it still does. (Not a blocker.)
Available time does not allow me to dive into the details of PPI, TAP, etc. for now. In order to really say something about this code, someone probably should. But that might block this patch for long time.. (Pushing this isolated test script will not hurt anyone :)

I still have three questions:

1) When running the script, I have lots of warnings like "No plan found in TAP output". For instance for admin/import_export_framework.pl.
Could you please clarify? Is this just a false positive?

2) For some other scripts, a reference is made to failed tests. E.g. acqui/basketgroup.pl: Failed tests:  7-9.
As I understand, these should refer to not exported subroutines. How can I find which ones? Are they routines in one of the 'used' modules? Documentation is not very clear. Naming the routines would be very helpful. Note that running the script without parameters will delete the temp dir right away; so I cannot check it.

3) The usage statement says: For more information, open the file with a text editor.. Which file?

I would like to pass qa on this patch. It has been here already too long. It may need some follow-ups to provide more useful information, but that should not block pushing this patch.
But before doing so, I would like to see your answers on the above three questions.
For that reason I set this patch to Failed QA. After receiving your answers, I will update the status again and add a signoff.

Thanks.
Comment 24 M. Tompsett 2014-04-04 17:55:47 UTC
Created attachment 26827 [details] [review]
[SIGNED OFF] Bug 8244: Add PPI to the list of Perl dependencies

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

NOTE: rebased to master only.
Comment 25 Marc Véron 2016-10-04 16:16:01 UTC
Still valid?
Comment 26 Julian Maurice 2018-02-06 13:31:56 UTC
(In reply to Marc Véron from comment #25)
> Still valid?

Probably not. 3 years without activity, and it doesn't work at all anymore.
Changing status to WONTFIX