Bug 38944 - Add Test::NoWarnings to tests without warnings
Summary: Add Test::NoWarnings to tests without warnings
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: dependency
Depends on:
Blocks: 25515
  Show dependency treegraph
 
Reported: 2025-01-22 13:30 UTC by Jonathan Druart
Modified: 2025-02-03 19:52 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds the Test::NoWarnings module to all the tests that currently do not produce warnings. This will then be a good starting point to move toward no warnings in our tests, with the next step to clean the other tests that currently do provide warnings.
Version(s) released in:
Circulation function:


Attachments
Bug 38944: Add new dependency Test::NoWarnings (727 bytes, patch)
2025-01-22 13:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Add Test::NoWarnings to tests no producing warnings (303.03 KB, patch)
2025-01-22 13:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Manual changes (757 bytes, patch)
2025-01-22 13:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Add new dependency Test::NoWarnings (727 bytes, patch)
2025-01-27 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Add Test::NoWarnings to tests no producing warnings (303.03 KB, patch)
2025-01-27 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Manual changes (757 bytes, patch)
2025-01-27 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Test summary report - 2025-01-28 (10.91 KB, text/plain)
2025-01-27 18:02 UTC, David Nind
Details
Bug 38944: Manual changes (18.97 KB, patch)
2025-01-28 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Correctly catch warnings raised by DBI (24.57 KB, patch)
2025-01-28 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Add Test::NoWarnings to tests no producing warnings (303.03 KB, patch)
2025-01-30 20:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Manual changes (18.97 KB, patch)
2025-01-30 20:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Correctly catch warnings raised by DBI (24.57 KB, patch)
2025-01-30 20:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Revert changes to selenium/01-installation.t (1.13 KB, patch)
2025-01-31 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38944: Add new dependency Test::NoWarnings (773 bytes, patch)
2025-02-03 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 38944: Add Test::NoWarnings to tests no producing warnings (301.78 KB, patch)
2025-02-03 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 38944: Manual changes (18.95 KB, patch)
2025-02-03 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 38944: Correctly catch warnings raised by DBI (24.59 KB, patch)
2025-02-03 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 38944: Revert changes to selenium/01-installation.t (1.17 KB, patch)
2025-02-03 19:47 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-01-22 13:30:34 UTC
To prevent new warnings for test files that do not generate warnings we should use Test::NoWarnings.
Comment 1 Jonathan Druart 2025-01-22 13:44:20 UTC
Created attachment 176912 [details] [review]
Bug 38944: Add new dependency Test::NoWarnings
Comment 2 Jonathan Druart 2025-01-22 13:44:24 UTC
Created attachment 176913 [details] [review]
Bug 38944: Add Test::NoWarnings to tests no producing warnings

This has been generated.
Comment 3 Jonathan Druart 2025-01-22 13:44:27 UTC
Created attachment 176914 [details] [review]
Bug 38944: Manual changes

Not caught by the script
Comment 4 Jonathan Druart 2025-01-22 13:45:45 UTC
The idea here is to add Test::NoWarnings to all the tests that currently do not produce warnings. So we have a good starting point to move toward no warnings in our tests.

The next step will be to clean the other tests that provide warnings.

Test plan:
The test suite should still pass after those patches.
Comment 5 Brendan Lawlor 2025-01-24 16:44:16 UTC
I tried testing, but I think Test::NoWarnings module needs to be added to ktd to be able to be able to test.
Comment 6 Jonathan Druart 2025-01-24 19:49:06 UTC
Yes you can install it using `cpanm Test::NoWarnings`
Comment 7 David Nind 2025-01-25 09:34:13 UTC
The patch no longer applies cleanly:

....
Auto-merging t/db_dependent/UsageStats.t
CONFLICT (content): Merge conflict in t/db_dependent/UsageStats.t
Auto-merging t/db_dependent/Koha/Biblio.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 38944: Add Test::NoWarnings to tests no producing warnings
......

Testing notes (using KTD):
1. Start up KTD with selenium: ktd --selenium up
2. Install the new dependency: cpanm Test::NoWarnings
3. Apply the patches.
4. Run all the tests:
   . ktd --shell --root
   . cd /kohadevbox/koha
   . perl /kohadevbox/misc4dev/run_tests.pl --run-all-tests
Comment 8 Jonathan Druart 2025-01-27 09:49:20 UTC
Created attachment 177165 [details] [review]
Bug 38944: Add new dependency Test::NoWarnings
Comment 9 Jonathan Druart 2025-01-27 09:49:23 UTC
Created attachment 177166 [details] [review]
Bug 38944: Add Test::NoWarnings to tests no producing warnings

This has been generated.
Comment 10 Jonathan Druart 2025-01-27 09:49:26 UTC
Created attachment 177167 [details] [review]
Bug 38944: Manual changes

Not caught by the script
Comment 11 David Nind 2025-01-27 18:02:27 UTC
Created attachment 177196 [details]
Test summary report - 2025-01-28

I've attached the tests summary showing the failures from running all the tests.

Testing notes (updated) (using KTD):
1. Start up KTD with selenium: ktd --selenium up
2. Install the new dependency: sudo cpanm Test::NoWarnings
3. Apply the patches.
4. Run all the tests:
   . ktd --shell --root
   . cd /kohadevbox/koha
   . perl /kohadevbox/misc4dev/run_tests.pl --run-all-tests
Comment 12 Jonathan Druart 2025-01-28 14:51:16 UTC
Created attachment 177274 [details] [review]
Bug 38944: Manual changes

Not caught by the script
Comment 13 Jonathan Druart 2025-01-28 14:51:19 UTC
Created attachment 177275 [details] [review]
Bug 38944: Correctly catch warnings raised by DBI
Comment 14 Tomás Cohen Arazi (tcohen) 2025-01-30 17:33:37 UTC
This is pretty awesome. I've always done this. 

Thanks!
Comment 15 David Nind 2025-01-30 19:22:00 UTC
Thanks Jonatahn!

I was too slow re-testing, and the patches no longer apply ...

git bz apply 38944

Bug 38944 - Add Test::NoWarnings to tests without warnings

177165 - Bug 38944: Add new dependency Test::NoWarnings
177166 - Bug 38944: Add Test::NoWarnings to tests no producing warnings
177274 - Bug 38944: Manual changes
177275 - Bug 38944: Correctly catch warnings raised by DBI

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 38944: Add new dependency Test::NoWarnings
Applying: Bug 38944: Add Test::NoWarnings to tests no producing warnings
Using index info to reconstruct a base tree...
M	t/Languages.t
M	t/db_dependent/Koha/CirculationRules.t
M	t/db_dependent/Koha/Holds.t
M	t/db_dependent/Koha/Object/Mixin/AdditionalFields.t
M	t/db_dependent/Languages.t
M	t/db_dependent/api/v1/items.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/api/v1/items.t
Auto-merging t/db_dependent/Languages.t
Auto-merging t/db_dependent/Koha/Object/Mixin/AdditionalFields.t
Auto-merging t/db_dependent/Koha/Holds.t
Auto-merging t/db_dependent/Koha/CirculationRules.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/CirculationRules.t
Auto-merging t/Languages.t
CONFLICT (content): Merge conflict in t/Languages.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 38944: Add Test::NoWarnings to tests no producing warnings
Comment 16 Jonathan Druart 2025-01-30 20:25:20 UTC
Created attachment 177362 [details] [review]
Bug 38944: Add Test::NoWarnings to tests no producing warnings

This has been generated.
Comment 17 Jonathan Druart 2025-01-30 20:25:23 UTC
Created attachment 177363 [details] [review]
Bug 38944: Manual changes

Not caught by the script
Comment 18 Jonathan Druart 2025-01-30 20:25:26 UTC
Created attachment 177364 [details] [review]
Bug 38944: Correctly catch warnings raised by DBI
Comment 19 David Nind 2025-01-30 21:26:26 UTC
Thanks Jonathan!

Still a few test failures:

Test Summary Report
-------------------
t/db_dependent/selenium/01-installation.t                                 (Wstat: 65280 (exited 255) Tests: 2 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 3 tests but ran 2.
t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t                  (Wstat: 2816 (exited 11) Tests: 3 Failed: 1)
  Failed test:  2
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 5 tests but ran 3.
t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t                   (Wstat: 5632 (exited 22) Tests: 4 Failed: 0)
  Non-zero exit status: 22
  Parse errors: Bad plan.  You planned 14 tests but ran 4.
t/db_dependent/api/v1/erm_sushi_services.t                                (Wstat: 256 (exited 1) Tests: 2 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
xt/find-misplaced-executables.t                                           (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
t/00-testcritic.t                                                         (Wstat: 0 Tests: 3245 Failed: 1)
  Failed test:  3245
  Parse errors: Plan (1..3244) must be at the beginning or end of the TAP output
                Bad plan.  You planned 3244 tests but ran 3245.
xt/verify-yarnlock.t                                                      (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
xt/find-license-problems.t                                                (Wstat: 256 (exited 1) Tests: 1378 Failed: 1)
  Failed test:  814
  Non-zero exit status: 1
Files=733, Tests=18916, 2790.29 wallclock secs ( 5.20 usr  1.93 sys + 2999.38 cusr 571.04 csys = 3577.55 CPU)
Result: FAIL
Comment 20 Jonathan Druart 2025-01-31 12:47:28 UTC
Created attachment 177380 [details] [review]
Bug 38944: Revert changes to selenium/01-installation.t
Comment 21 Jonathan Druart 2025-01-31 12:47:41 UTC
(In reply to David Nind from comment #19)
> Thanks Jonathan!
> 
> Still a few test failures:
> 
> Test Summary Report
> -------------------
> t/db_dependent/selenium/01-installation.t                                
> (Wstat: 65280 (exited 255) Tests: 2 Failed: 0)
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 3 tests but ran 2.

Should be fixed by the last patch.

> t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t                 
> (Wstat: 2816 (exited 11) Tests: 3 Failed: 1)
>   Failed test:  2
>   Non-zero exit status: 11
>   Parse errors: Bad plan.  You planned 5 tests but ran 3.
> t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t                  
> (Wstat: 5632 (exited 22) Tests: 4 Failed: 0)
>   Non-zero exit status: 22
>   Parse errors: Bad plan.  You planned 14 tests but ran 4.

They are passing for me. Do they pass on main?

> t/db_dependent/api/v1/erm_sushi_services.t                               
> (Wstat: 256 (exited 1) Tests: 2 Failed: 1)
>   Failed test:  1
>   Non-zero exit status: 1

This is currently failing on main.

> xt/find-misplaced-executables.t                                          
> (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
>   Failed test:  1
>   Non-zero exit status: 1
> t/00-testcritic.t                                                        
> (Wstat: 0 Tests: 3245 Failed: 1)
>   Failed test:  3245
>   Parse errors: Plan (1..3244) must be at the beginning or end of the TAP
> output
>                 Bad plan.  You planned 3244 tests but ran 3245.
> xt/verify-yarnlock.t                                                     
> (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
>   Failed test:  1
>   Non-zero exit status: 1
> xt/find-license-problems.t                                               
> (Wstat: 256 (exited 1) Tests: 1378 Failed: 1)
>   Failed test:  814
>   Non-zero exit status: 1
> Files=733, Tests=18916, 2790.29 wallclock secs ( 5.20 usr  1.93 sys +
> 2999.38 cusr 571.04 csys = 3577.55 CPU)
> Result: FAIL

What's the output of those ones? This patchset does not modify them so I guess they also fail on main.
Comment 22 David Nind 2025-02-01 00:00:57 UTC
Hi Jonathan.

These are the only tests that are failing now if I run all the tests at once.

Some of the previous errors were because I had a folder with kidclamp's handy Koha scripts.

Test Summary Report
-------------------
t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t                  (Wstat: 2816 (exited 11) Tests: 3 Failed: 1)
  Failed test:  2
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 5 tests but ran 3.
t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t                   (Wstat: 65280 (exited 255) Tests: 4 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 14 tests but ran 4.
xt/verify-yarnlock.t                                                      (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
t/00-testcritic.t                                                         (Wstat: 0 Tests: 3245 Failed: 1)
  Failed test:  3245
  Parse errors: Plan (1..3244) must be at the beginning or end of the TAP output
                Bad plan.  You planned 3244 tests but ran 3245.
Files=733, Tests=18914, 2958.36 wallclock secs ( 5.49 usr  1.96 sys + 3076.34 cusr 577.41 csys = 3661.20 CPU)
Result: FAIL

1. Elasticsearch tests
======================

Both the tests fail for me when I run all the tests after applying the patches, etc.

However, after the patches are applied, if I shutdown and restart KTD (and then add the module), the tests pass when I run them individually.

2. xt/verify-yarnlock.t
=======================

Fails for me before and after the patches. Lots of warnings and errors.

If I do a yarn install and yarn build, then run the test individually, it passes.

.....
warning "arraybuffer.prototype.slice#function.prototype.name#call-bind#get-intrinsic##has-symbols@^1.0.1" could be deduped from "1.0.3" to "has-symbols@1.0.3"
warning "string.prototype.trim#function.prototype.name#call-bind#get-intrinsic##function-bind@^1.1.1" could be deduped from "1.1.1" to "function-bind@1.1.1"
warning "string.prototype.trim#function.prototype.name#call-bind#get-intrinsic##has@^1.0.3" could be deduped from "1.0.3" to "has@1.0.3"
warning "string.prototype.trim#function.prototype.name#call-bind#get-intrinsic##has-symbols@^1.0.1" could be deduped from "1.0.3" to "has-symbols@1.0.3"
error Found 54 errors.
xt/verify-yarnlock.t .. 1/1 
#   Failed test 'verify yarn.lock file is updated correctly'
#   at xt/verify-yarnlock.t line 45.
#          got: '256'
#     expected: '0'
# Looks like you failed 1 test of 1.
xt/verify-yarnlock.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
xt/verify-yarnlock.t (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  3 wallclock secs ( 0.04 usr  0.02 sys +  2.94 cusr  1.13 csys =  4.13 CPU)
Result: FAIL

3. t/00-testcritic.t
====================

Fails for me before and after the patches.

prove t/00-testcritic.t 
t/00-testcritic.t .. All 3244 subtests passed 

Test Summary Report
-------------------
t/00-testcritic.t (Wstat: 0 Tests: 3245 Failed: 1)
  Failed test:  3245
  Parse errors: Plan (1..3244) must be at the beginning or end of the TAP output
                Bad plan.  You planned 3244 tests but ran 3245.
Files=1, Tests=3245, 30 wallclock secs ( 0.71 usr  0.04 sys + 224.51 cusr  1.36 csys = 226.62 CPU)
Result: FAIL
Comment 23 David Nind 2025-02-03 19:47:09 UTC
Created attachment 177477 [details] [review]
Bug 38944: Add new dependency Test::NoWarnings

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2025-02-03 19:47:13 UTC
Created attachment 177478 [details] [review]
Bug 38944: Add Test::NoWarnings to tests no producing warnings

This has been generated.

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2025-02-03 19:47:16 UTC
Created attachment 177479 [details] [review]
Bug 38944: Manual changes

Not caught by the script

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2025-02-03 19:47:19 UTC
Created attachment 177480 [details] [review]
Bug 38944: Correctly catch warnings raised by DBI

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2025-02-03 19:47:21 UTC
Created attachment 177481 [details] [review]
Bug 38944: Revert changes to selenium/01-installation.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 David Nind 2025-02-03 19:49:08 UTC
Now signed off - see comments on chat https://chat.koha-community.org/koha-community/pl/ck6muapewi8p5e87ehsefyii5c

"..it seems that you can signoff, you don't have more errors after the patches. The failures you get on main can be caused by several reasons (git repo not clean, node_modules not clean, ktd started withou a --es flag). Hard to tell but they do not seem related to this patch"