Bug 37245 - Add test script that runs `perl -c` on all Perl files
Summary: Add test script that runs `perl -c` on all Perl files
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 35959
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-03 13:20 UTC by Julian Maurice
Modified: 2024-09-05 23:48 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 37245: Add test script that runs `perl -c` on all Perl files (2.52 KB, patch)
2024-07-03 13:21 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 37245: Add test script that runs `perl -c` on all Perl files (2.57 KB, patch)
2024-07-03 20:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 37245: Modify 00-strict.t to check all Perl files (2.90 KB, patch)
2024-09-05 10:27 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2024-07-03 13:20:33 UTC

    
Comment 1 Julian Maurice 2024-07-03 13:21:37 UTC
Created attachment 168449 [details] [review]
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
Comment 2 Julian Maurice 2024-07-03 13:24:16 UTC
Do not forget to apply bug 35959 before testing
Comment 3 David Nind 2024-07-03 20:30:41 UTC
Created attachment 168477 [details] [review]
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>
Comment 4 David Cook 2024-07-04 01:37:22 UTC
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...
Comment 5 Jonathan Druart 2024-07-04 11:13:19 UTC
You should use `git ls-tree` to list the files to test.
Comment 6 David Cook 2024-07-04 23:15:03 UTC
(In reply to Jonathan Druart from comment #5)
> You should use `git ls-tree` to list the files to test.

Good thinking
Comment 7 David Cook 2024-09-03 02:17:07 UTC
(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.
Comment 8 Julian Maurice 2024-09-03 07:15:15 UTC
I agree with David.

Tests should not require git.

But we might want to exclude the 'blib' directory ?
Comment 9 Jonathan Druart 2024-09-03 07:24:57 UTC
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).
Comment 10 Julian Maurice 2024-09-03 10:17:02 UTC
(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"
Comment 11 Jonathan Druart 2024-09-03 10:37:28 UTC
Not going further with this discussion.

Feel free to provide an alternative that works.
Comment 12 Julian Maurice 2024-09-05 08:34:25 UTC
(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.
Comment 13 Jonathan Druart 2024-09-05 08:58:22 UTC
(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.
Comment 14 Jonathan Druart 2024-09-05 09:02:21 UTC
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?
Comment 15 Julian Maurice 2024-09-05 09:38:22 UTC
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
Comment 16 Jonathan Druart 2024-09-05 10:12:05 UTC
(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?...
Comment 17 Julian Maurice 2024-09-05 10:27:02 UTC
Created attachment 171052 [details] [review]
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>
Comment 18 Julian Maurice 2024-09-05 11:17:45 UTC
(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.
Comment 19 Jonathan Druart 2024-09-05 12:10:42 UTC
Yes, see bug 34088. I told you on Mattermost! :D
Comment 20 Jonathan Druart 2024-09-05 12:13:03 UTC
(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.
Comment 21 David Cook 2024-09-05 23:48:21 UTC
(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...