Description
Julian Maurice
2012-06-14 09:54:29 UTC
Created attachment 10272 [details] [review] 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. -- To use it: xt/find-undefined-subroutines.pl --src-path /path/to/koha --dest-path /another/path [-v] If you launch it on current master, you will find that almost all scripts return that GetMember is not defined in C4/Auth.pm. Indeed, line 154, GetMember is called without the module name prepended, and before the require statement. But I tried to make this piece of code crash, without success. It seems that this is "dead code". The script doesn't test subroutines prepended by the module name (we assume that they should work in every case, even when there are circular dependencies) so you could temporarily hide these messages by prepending GetMember with the module name in C4::Auth::get_template_and_user Another interesting error returned by the script: in acqui/basketgroup.pl GetBookSeller is undefined. You can test by going to acqui/basketgroup.pl and remove the '?booksellerid=X' part of the url. I think it's dead code too, because fixing this bug causes another bug a little bit further. Note: in acqui/basketgroup.pl, GetBiblioData and GetItemTypes are returned as not defined too, but there is no problem with them in "real code". This is because C4::Biblio and C4::Koha are imported by using require/import instead of use and this is not handled by the script (could be an enhancement). Important note: This script requires PPI module which is not a Koha dependency, but I don't think it should be because this script is only for debugging purposes and of no usefulness for end-users. Note: this require to have $KOHA_CONF to be correctly set ($PERL5LIB is not needed) 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
Created attachment 10374 [details] [review] Bug 8244: New script xt/find-undefined-subroutines.pl Added --help option --src-path and --dest-path no longer required. --src-path defaults to: dirname($0)."/..". It is displayed with --help --dest-path defaults to a temporary directory (use File::Temp) Fixed many bugs on pathnames Removed --trap option of prove since it seems that it's not available everywhere 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? 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. (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. 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> Created attachment 10543 [details] [review] 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 The first patch has been signed off, but my follow-up still requires a sign-off. 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. Created attachment 10943 [details] [review] Bug 8244 follow-up: improvements to find-undefined-subroutines Fixed follow-up: - No more errors when not using --ignore option - Help message now tells that --ignore takes a regular expression PPI needs to be added as a dependency for this. 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> 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. (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? (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 :) (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. Created attachment 12239 [details] [review] Bug 8244: Add PPI to the list of Perl dependencies Created attachment 13223 [details] [review] Bug 8244: Add PPI to the list of Perl dependencies Rebased on master Created attachment 14415 [details] [review] [SIGNED-OFF] Bug 8244: Add PPI to the list of Perl dependencies QA: Looking at this one now.. 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. 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. Still valid? (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 |