Bug 39876

Summary: Centralize listing of files from our codebase
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: Passed QA --- QA Contact: Tomás Cohen Arazi (tcohen) <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: david, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Medium patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 40261    
Bug Blocks: 39877    
Attachments: Bug 39876: Move `git ls-files` to Koha::Devel::Files
Bug 39876: Add context for tidy
Bug 39876: Apply module to other tests
Bug 39876: Move `git ls-files` to Koha::Devel::Files
Bug 39876: Add context for tidy
Bug 39876: Apply module to other tests
Bug 39876: Apply module to other tests
Bug 39876: Move `git ls-files` to Koha::Devel::Files
Bug 39876: Add context for tidy
Bug 39876: Apply module to other tests
Bug 39876: Get TT files for tt_tidy
Bug 39876: Get TT files for tt_tidy
Bug 39876: Move `git ls-files` to Koha::Devel::Files
Bug 39876: Add context for tidy
Bug 39876: Apply module to other tests
Bug 39876: Get TT files for tt_tidy
Bug 39876: Add build-git-snapshot type resolution
Bug 39876: Make find-missing-op-in-forms.t test TT files
Bug 39876: Add build-git-snapshot type resolution

Description Jonathan Druart 2025-05-12 13:34:45 UTC
We are using `git ls-files` in different places (tidy, tests, etc)

We should have this code in a module.
Comment 1 Jonathan Druart 2025-05-12 20:33:13 UTC
Created attachment 182305 [details] [review]
Bug 39876: Move `git ls-files` to Koha::Devel::Files

This patch introduces a new Koha::Devel namespace. It clearly shows
that its purpose is for development and will not contain any business
logic.
Comment 2 Jonathan Druart 2025-05-12 20:33:15 UTC
Created attachment 182306 [details] [review]
Bug 39876: Add context for tidy

With this patch we adjust what's needed to make misc/devel/tidy.pl run
identically as before
Comment 3 Jonathan Druart 2025-05-12 20:33:18 UTC
Created attachment 182307 [details] [review]
Bug 39876: Apply module to other tests

We are ready to reuse Koha::Devel::Files in other places where
`git ls-files` is used.
Comment 4 Jonathan Druart 2025-05-12 20:34:50 UTC
Test plan: prove on the files modified by this patchset should run the same number of tests (or more) and still pass.
misc/devel/tidy.pl should still work as before when using the --perl, --js and --tt options
Comment 5 David Nind 2025-05-12 23:56:39 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2025-05-13 09:45:56 UTC
Created attachment 182344 [details] [review]
Bug 39876: Move `git ls-files` to Koha::Devel::Files

This patch introduces a new Koha::Devel namespace. It clearly shows
that its purpose is for development and will not contain any business
logic.
Comment 7 Jonathan Druart 2025-05-13 09:45:59 UTC
Created attachment 182345 [details] [review]
Bug 39876: Add context for tidy

With this patch we adjust what's needed to make misc/devel/tidy.pl run
identically as before
Comment 8 Jonathan Druart 2025-05-13 09:46:02 UTC
Created attachment 182346 [details] [review]
Bug 39876: Apply module to other tests

We are ready to reuse Koha::Devel::Files in other places where
`git ls-files` is used.
Comment 9 Jonathan Druart 2025-05-13 09:46:14 UTC
(In reply to David Nind from comment #5)
> Apologies in advance for the long comment!
> 
> Everything looks OK before the patches.
> 
> After the patches, several tests fail:
> 
> prove xt/author/codespell.t - before = 4440 PASS; after = 4616 FAIL

It was missing the exclusion list.
after 4439

> prove xt/perltidy.t - before = 3018 PASS; after = 3296 FAIL

It was missing the exclusion list.

> prove xt/pl_valid.t - before = 3298 PASS; after = 1 FAIL

Fixed now.


In the test plan I stated that there should be more files tested after than before. It's actually true, but the number of tests does not exactly represent the number of files tested, because of duplication. Here is the diff of the list of files:

350a351
> Koha/Devel/Files.pm
2341d2341
< opac/svc/shelfbrowser.pl
2440d2439
< svc/cataloguing/automatic_linker.pl

Those 2 svc files are actually duplicated in the "main" version and are tested twice.

> prove xt/tt_tidy.t - before = 866 PASS; after = 3017 FAIL

Oops, it was retrieving the perl files :-/

It's fixed now. There is this difference in the files tested:

86d85
< koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-open.inc
714d712
< koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-open.inc
854,858d851
< misc/cronjobs/rss/lastAcquired-1.0.tt
< misc/cronjobs/rss/lastAcquired-2.0.tt
< misc/cronjobs/rss/lastAcquired.tt
< misc/cronjobs/rss/longestUnseen.tt
< misc/cronjobs/rss/mostReserved.tt

This is because we are fixing an inconsistency: misc/devel/tidy.pl was excluding them, but xt/tt_tidy.t was not (but the files was skipped later anyway).
Now they are excluded for both.


> After the patches, and a misc/devel/tidy.pl --perl, there is a file change
> when you do a git status:

Oops, fixed now.


> Maybe there is something else I need to do after applying the patches?
> (Other than a restart_all.)

Nope, it's just me submitting patches too late in the evening, and being lazy to run again the whole test suite.
Sorry about that and thanks a lot for your testing!
Comment 10 Jonathan Druart 2025-05-13 12:30:39 UTC
Created attachment 182354 [details] [review]
Bug 39876: Apply module to other tests

We are ready to reuse Koha::Devel::Files in other places where
`git ls-files` is used.
Comment 11 David Nind 2025-05-13 13:39:55 UTC
Created attachment 182380 [details] [review]
Bug 39876: Move `git ls-files` to Koha::Devel::Files

This patch introduces a new Koha::Devel namespace. It clearly shows
that its purpose is for development and will not contain any business
logic.

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2025-05-13 13:39:58 UTC
Created attachment 182381 [details] [review]
Bug 39876: Add context for tidy

With this patch we adjust what's needed to make misc/devel/tidy.pl run
identically as before

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2025-05-13 13:40:00 UTC
Created attachment 182382 [details] [review]
Bug 39876: Apply module to other tests

We are ready to reuse Koha::Devel::Files in other places where
`git ls-files` is used.

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2025-05-13 13:43:25 UTC
Thanks Jonathan!

Tests now all pass and tidying files works as expected (no changes identified).

Testing notes (using KTD, on main):

1. Run these tests, they should all pass:
   - prove xt/author/codespell.t - before = 4440 PASS; after = 4439 PASS
   - prove xt/author/pod_checker.t - before = 3298 PASS; after = 3297 PASS
   - prove xt/author/podcorrectness.t - before = 3297 PASS; after = 3296 PASS
   - prove xt/find-missing-csrf.t - before = 2 PASS; after = 2 PASS
   - prove xt/find-missing-filters.t - before = 2 PASS; after = 2 PASS
   - prove xt/find-missing-op-in-forms.t - before = 2 PASS; after = 2 PASS
   - prove xt/perltidy.t - before = 3018 PASS; after = 3017 PASS
   - prove xt/pl_valid.t - before = 3298 PASS; after = 3293 PASS
   - prove xt/single_quotes.t - before = 2 PASS; after = 2 PASS
   - prove xt/tt_tidy.t - before = 866 PASS; after = 3017 PASS

2. Tidy all the files, there should be no changes using a git status:
   - misc/devel/tidy.pl --perl
   - misc/devel/tidy.pl --js
   - misc/devel/tidy.pl --tt
Comment 15 Jonathan Druart 2025-05-13 15:34:06 UTC
> prove xt/tt_tidy.t - before = 866 PASS; after = 3017 PASS

Are you sure about this "after" number?
I cannot test right now, but it does not look correct.
Comment 16 David Nind 2025-05-13 19:01:38 UTC
(In reply to Jonathan Druart from comment #15)
> > prove xt/tt_tidy.t - before = 866 PASS; after = 3017 PASS
> 
> Are you sure about this "after" number?
> I cannot test right now, but it does not look correct.

I thought I'd updated it, but looks like I didn't? After the patches it is now 3019.

I re-ran, and this is the result now after the patches are applied:

prove xt/tt_tidy.t
xt/tt_tidy.t .. ok      
All tests successful.
Files=1, Tests=3019, 382 wallclock secs ( 0.06 usr  0.00 sys + 1188.61 cusr 261.40 csys = 1450.07 CPU)
Result: PASS
Comment 17 Jonathan Druart 2025-05-13 19:59:36 UTC
Created attachment 182417 [details] [review]
Bug 39876: Get TT files for tt_tidy
Comment 18 Jonathan Druart 2025-05-13 20:00:39 UTC
Well then it's wrong. I am sure I fixed that before!
It is now with the last patch.
Comment 19 David Nind 2025-05-14 17:26:05 UTC Comment hidden (obsolete)
Comment 20 David Nind 2025-05-14 17:29:31 UTC
Created attachment 182461 [details] [review]
Bug 39876: Get TT files for tt_tidy

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2025-05-14 17:44:51 UTC
(In reply to Jonathan Druart from comment #18)
> Well then it's wrong. I am sure I fixed that before!
> It is now with the last patch.

Re-replying as I didn't include a before patch results for for prove xt/tt_tidy.t. 

Now 859 after the additional patch.

I re-ran everything on the latest main:

1. Run these tests, they should all pass:
   - prove xt/author/codespell.t - before = 4442 PASS; after = 4441 PASS
   - prove xt/author/pod_checker.t - before = 3300 PASS; after = 3299 PASS
   - prove xt/author/podcorrectness.t - before = 3299 PASS; after = 3298 PASS
   - prove xt/find-missing-csrf.t - before = 2 PASS; after = 2 PASS
   - prove xt/find-missing-filters.t - before = 2 PASS; after = 2 PASS
   - prove xt/find-missing-op-in-forms.t - before = 2 PASS; after = 2 PASS
   - prove xt/perltidy.t - before = 3020 PASS; after = 3019 PASS
   - prove xt/pl_valid.t - before = 3300 PASS; after = 3295 PASS
   - prove xt/single_quotes.t - before = 2 PASS; after = 2 PASS
   - prove xt/tt_tidy.t - before = 866 PASS; after = 859 PASS [1] [2]

2. Tidy all the files, there should be no changes using a git status:
   - misc/devel/tidy.pl --perl (before = 3018 files, no changes; after = 3019 files)
   - misc/devel/tidy.pl --js (before = 282 files, no changes; after = 282 files)
   - misc/devel/tidy.pl --tt (before = 859 files, no changes; after = 859 files) [3] [4]

[1] prove xt/tt_tidy.t - before patch
-------------------------------------

prove xt/tt_tidy.t
xt/tt_tidy.t .. Something went wrong, Prettier generated an empty file. The original file was kept at misc/devel/tidy.pl line 112.
Something went wrong, Prettier generated an empty file. The original file was kept at misc/devel/tidy.pl line 112.
Something went wrong, Prettier generated an empty file. The original file was kept at misc/devel/tidy.pl line 112.
xt/tt_tidy.t .. ok     
All tests successful.
Files=1, Tests=866, 168 wallclock secs ( 0.02 usr  0.00 sys + 1242.16 cusr 390.99 csys = 1633.17 CPU)
Result: PASS

[2] prove xt/tt_tidy.t - after patch
-------------------------------------

prove xt/tt_tidy.t
xt/tt_tidy.t .. Something went wrong, Prettier generated an empty file. The original file was kept at misc/devel/tidy.pl line 108.
Something went wrong, Prettier generated an empty file. The original file was kept at misc/devel/tidy.pl line 108.
Something went wrong, Prettier generated an empty file. The original file was kept at misc/devel/tidy.pl line 108.
xt/tt_tidy.t .. ok     
All tests successful.
Files=1, Tests=859, 168 wallclock secs ( 0.03 usr  0.01 sys + 1230.65 cusr 387.11 csys = 1617.80 CPU)
Result: PASS

[3] misc/devel/tidy.pl --tt - before patches
--------------------------------------------
Some files cannot be tidied:
	* koha-tmpl/intranet-tmpl/prog/en/includes/ill/backends/Standard/shared/forms/other.inc
Something went wrong, Prettier generated an empty file. The original file was kept
	* koha-tmpl/intranet-tmpl/prog/en/includes/ill/backends/Standard/illview.inc
Something went wrong, Prettier generated an empty file. The original file was kept
	* koha-tmpl/opac-tmpl/bootstrap/en/includes/ill/backends/Standard/shared/forms/other.inc
Something went wrong, Prettier generated an empty file. The original file was kept

[4] misc/devel/tidy.pl --tt - after patches
-------------------------------------------
Some files cannot be tidied:
	* koha-tmpl/intranet-tmpl/prog/en/includes/ill/backends/Standard/illview.inc
Something went wrong, Prettier generated an empty file. The original file was kept
	* koha-tmpl/intranet-tmpl/prog/en/includes/ill/backends/Standard/shared/forms/other.inc
Something went wrong, Prettier generated an empty file. The original file was kept
	* koha-tmpl/opac-tmpl/bootstrap/en/includes/ill/backends/Standard/shared/forms/other.inc
Something went wrong, Prettier generated an empty file. The original file was kept
Comment 22 Jonathan Druart 2025-05-19 08:06:48 UTC
Thanks a lot, David!
Comment 23 Tomás Cohen Arazi (tcohen) 2025-06-27 17:28:13 UTC
Created attachment 183618 [details] [review]
Bug 39876: Move `git ls-files` to Koha::Devel::Files

This patch introduces a new Koha::Devel namespace. It clearly shows
that its purpose is for development and will not contain any business
logic.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi (tcohen) 2025-06-27 17:28:16 UTC
Created attachment 183619 [details] [review]
Bug 39876: Add context for tidy

With this patch we adjust what's needed to make misc/devel/tidy.pl run
identically as before

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi (tcohen) 2025-06-27 17:28:18 UTC
Created attachment 183620 [details] [review]
Bug 39876: Apply module to other tests

We are ready to reuse Koha::Devel::Files in other places where
`git ls-files` is used.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi (tcohen) 2025-06-27 17:28:21 UTC
Created attachment 183621 [details] [review]
Bug 39876: Get TT files for tt_tidy

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi (tcohen) 2025-06-27 17:28:24 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi (tcohen) 2025-06-27 17:28:27 UTC
Created attachment 183623 [details] [review]
Bug 39876: Make find-missing-op-in-forms.t test TT files

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi (tcohen) 2025-06-27 17:41:47 UTC
Created attachment 183625 [details] [review]
Bug 39876: Add build-git-snapshot type resolution

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi (tcohen) 2025-06-27 17:44:16 UTC
Ok, only a minor tweak and porting the change from bug 40261 here. I made it dependent on it because otherwise `xt/perltidy.t` would fail. They should be pushed together.