[Bug 37245] New: Add test script that runs `perl -c` on all Perl files
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Bug ID: 37245 Summary: Add test script that runs `perl -c` on all Perl files Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: julian.maurice@biblibre.com Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org Depends on: 35959 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35959 [Bug 35959] Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue' (and a few other modules) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 168449 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168449&action=edit Bug 37245: Add test script that runs `perl -c` on all Perl files and minor changes to Koha::Localization and Koha::MarcOverlayRules to make tests pass Test plan: - Run `prove xt/perl-check.t` - Wait... (it took ~15 minutes for me) - Should be green at the end -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #2 from Julian Maurice <julian.maurice@biblibre.com> --- Do not forget to apply bug 35959 before testing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168449|0 |1 is obsolete| | --- Comment #3 from David Nind <david@davidnind.com> --- Created attachment 168477 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168477&action=edit Bug 37245: Add test script that runs `perl -c` on all Perl files and minor changes to Koha::Localization and Koha::MarcOverlayRules to make tests pass Test plan: - Run `prove xt/perl-check.t` - Wait... (it took ~15 minutes for me) - Should be green at the end Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Interesting idea! It'll run any code in BEGIN{} but most of the time that's probably fine, as I think we mostly have used that for the Exporter... Also wow we're using the Exporter more than we should be in Koha::*. Coding guidelines only work when we enforce them, eh... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #5 from Jonathan Druart <jonathan.druart@gmail.com> --- You should use `git ls-tree` to list the files to test. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #5)
You should use `git ls-tree` to list the files to test.
Good thinking -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #6)
(In reply to Jonathan Druart from comment #5)
You should use `git ls-tree` to list the files to test.
Good thinking
Actually, I take that back. You won't necessarily be running this test in a git checkout. Someone could've downloaded the tarball and want to run it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- I agree with David. Tests should not require git. But we might want to exclude the 'blib' directory ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #9 from Jonathan Druart <jonathan.druart@gmail.com> --- It's how we do everywhere else. And we agreed it should work like that. See bug 33743 and bug 32352 for more context. If we want to change that, then be consistent and fix the other places (taking into account the original problems). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|julian.maurice@biblibre.com |koha-bugs@lists.koha-commun | |ity.org --- Comment #10 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #9)
It's how we do everywhere else. And we agreed it should work like that. Did "we" write something about it in the wiki or somewhere else ? I can't find any occurrence of "ls-files" or "ls-tree"
-- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- Not going further with this discussion. Feel free to provide an alternative that works. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #11)
Not going further with this discussion. I just wanted to point out that a rule cannot be enforced if it's not known by everyone in the QA team. If the community agrees to enforce a rule it should be written somewhere.
Feel free to provide an alternative that works. I would if you can tell me what's wrong about the patch. Several scripts in xt/ use File::Find and it looks like they're working fine.
-- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #13 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Julian Maurice from comment #12)
(In reply to Jonathan Druart from comment #11)
Not going further with this discussion. I just wanted to point out that a rule cannot be enforced if it's not known by everyone in the QA team. If the community agrees to enforce a rule it should be written somewhere.
Sorry for the grumpiness. Sometimes people are not around for discussion, and 1 or 2 people have to take quick decisions. At the time we had to deal with Vue, node_modules, etc. which was a huge task. We (at least Tomas, Martin and me regarding bug 32352 comments 4-6) decided to run the test suite on the files we knew that must be tested: the ones in Koha, ie. the ones in the git index. It seemed better than adding exceptions all over the xt files (see patch on bug 32352). Bug 33743 is also dealing with files we do not know that exists in the subdirs. how-to.tt: we know it is there, but others could be present for whatever reasons.
Feel free to provide an alternative that works. I would if you can tell me what's wrong about the patch. Several scripts in xt/ use File::Find and it looks like they're working fine.
They are wrong and should be adjusted IMO (using the same methods, `git ls-tree`). I just picked one randomly, xt/find-license-problems.t 36 unless $name =~ /\/(\.git|koha-tmpl|node_modules|swagger-ui)(\/.*)?$/ || This is not necessary if we were looping on `git ls-tree` output. We want the test suite to be run by different people and have the same result everywhere. If you have how-to.tt (that is not passing some tests), we don't care about it. If you have local files not in the git index, we don't care about them. We want the test suite to be as reliable as possible. I understand that we want more, and have the test suite to pass even if we are not within a git repo. But most importantly for me is to have consistency in the runs and easy reproducibility. Most of us are using ktd, and Jenkins is using it as well, which certainly covers more of 90% of the (dev) usages. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #14 from Jonathan Druart <jonathan.druart@gmail.com> --- Also, in my understanding t/db_dependent/00-strict.t (it also needs to be adjusted btw!) which is using Test::Strict is already running perl -c https://metacpan.org/pod/Test::Strict all_perl_files_ok Applies strict_ok() and syntax_ok() to all perl files found syntax_ok Run a syntax check on $file by running perl -c $file What's the differences? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED --- Comment #15 from Julian Maurice <julian.maurice@biblibre.com> --- t/db_dependent/00-strict.t might be the same thing, but at the moment it does not check files in C4 and Koha directories I just tested it, adding 'Koha' in the list of directories, and it would have caught the error fixed by bug 35959. (But why is it in db_dependent ?... I missed it when searching for something similar) I'll try to submit a new patch, modifying t/db_dependent/00-strict.t instead of adding a new script -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #16 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Julian Maurice from comment #15)
t/db_dependent/00-strict.t might be the same thing, but at the moment it does not check files in C4 and Koha directories
I just tested it, adding 'Koha' in the list of directories, and it would have caught the error fixed by bug 35959.
(But why is it in db_dependent ?... I missed it when searching for something similar)
Didn't we have $dbh in the BEGIN block of C4::Context, or somewhere else?... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168477|0 |1 is obsolete| | --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 171052 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171052&action=edit Bug 37245: Modify 00-strict.t to check all Perl files and minor changes to Koha::Localization and Koha::MarcOverlayRules to make tests pass Test plan: - Run `prove t/db_dependent/00-strict.t` - Wait... (it took ~5 minutes for me) - Should be green at the end Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #16)
Didn't we have $dbh in the BEGIN block of C4::Context, or somewhere else?...
Running 00-strict.t (with the patch) with MariaDB stopped I got only one failure: not ok 1908 - Syntax check installer/data/mysql/updatedatabase.pl # Failed test 'Syntax check installer/data/mysql/updatedatabase.pl' # at t/db_dependent/00-strict.t line 32. # DBI connect('database=koha;host=localhost;port=3306','koha',...) failed: Can't connect to local server through socket '/run/mysqld/mysqld.sock' (2) at /home/koha/koha/Koha/Database.pm line 91. # BEGIN failed--compilation aborted at installer/data/mysql/updatedatabase.pl line 37. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #19 from Jonathan Druart <jonathan.druart@gmail.com> --- Yes, see bug 34088. I told you on Mattermost! :D -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #20 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #19)
Yes, see bug 34088. I told you on Mattermost! :D
Maybe not the same error we have there actually, but related to the DB connection in BEGIN. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #13)
Sorry for the grumpiness.
Much appreciated and apologies for my grumpiness lately too. Looking forward to a break before Kohacon... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #22 from Jonathan Druart <jonathan.druart@gmail.com> --- +my @files = `git ls-files '*.pl' '*.PL' '*.pm' '*.psgi'`; Shouldn't we pick *.t as well? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #22)
+my @files = `git ls-files '*.pl' '*.PL' '*.pm' '*.psgi'`;
Shouldn't we pick *.t as well?
Not sure if it is useful: if one of them is not syntactically correct, we should be able to see it in Jenkins output -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #24 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Julian Maurice from comment #23)
(In reply to Jonathan Druart from comment #22)
+my @files = `git ls-files '*.pl' '*.PL' '*.pm' '*.psgi'`;
Shouldn't we pick *.t as well?
Not sure if it is useful: if one of them is not syntactically correct, we should be able to see it in Jenkins output
Yes agreed. It could be useful later however if we decide to stop the run if 00-strict if not passing (which would me sense). But that's not for now. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #25 from Jonathan Druart <jonathan.druart@gmail.com> --- all_perl_files_ok "A perl file is *.pl or *.pm or *.t or a file starting with #!...perl" We are loosing the files from svc and opac/svc (at least). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #26 from Jonathan Druart <jonathan.druart@gmail.com> --- Totally forgot about this bug, I opened bug 39365 for a similar need. However I run perl -cw directly on the Perl files. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #27 from Jonathan Druart <jonathan.druart@gmail.com> --- *** Bug 39365 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 --- Comment #28 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #27)
*** Bug 39365 has been marked as a duplicate of this bug. ***
Actually it's not a duplicate, or at least they don't test for the same errors/warnings. For instance: $ perl -wc C4/Tags.pm Name "Lingua::Ispell::path" used only once: possible typo at C4/Tags.pm line 63. C4/Tags.pm syntax OK $ echo $? 0 We want this warning to be caught. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Bug 37245 depends on bug 35959, which changed state. Bug 35959 Summary: Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue' (and a few other modules) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35959 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Abdivasiyev Sunnatillo <abdivasiyevsunnatillo2008@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abdivasiyevsunnatillo2008@g | |mail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37245 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |RESOLVED Resolution|--- |DUPLICATE --- Comment #29 from Jonathan Druart <jonathan.druart@gmail.com> --- Looks like I forget (again) this one. I've just submitted bug 42126. *** This bug has been marked as a duplicate of bug 42126 *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org