Bug 37245

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

Do not forget to apply bug 35959 before testing

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>

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 should use `git ls-tree` to list the files to test.

(In reply to Jonathan Druart from comment #5)
> You should use `git ls-tree` to list the files to test.

Good thinking

(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.

I agree with David.

Tests should not require git.

But we might want to exclude the 'blib' directory ?

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).

(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"

Not going further with this discussion.

Feel free to provide an alternative that works.

(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.

(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.

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?

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

(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?...

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>

(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.

Yes, see bug 34088. I told you on Mattermost! :D

(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.

(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...

+my @files = `git ls-files '*.pl' '*.PL' '*.pm' '*.psgi'`;

Shouldn't we pick *.t as well?

(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

(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.

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).