Bug 18292 - Tests do not need to return 1;
Summary: Tests do not need to return 1;
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 12596
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-17 07:02 UTC by Olli-Antti Kivilahti
Modified: 2018-06-04 20:10 UTC (History)
9 users (show)

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


Attachments
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1; (704 bytes, patch)
2017-03-17 07:06 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1; (769 bytes, patch)
2017-06-08 01:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1; (854 bytes, patch)
2017-06-08 15:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18292: Remove return 1 statements in tests (42.95 KB, patch)
2017-06-09 12:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18292: Remove return 1 statements in tests (44.10 KB, patch)
2017-08-07 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18292: Remove return 1 statements in tests (42.66 KB, patch)
2017-08-09 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1; (814 bytes, patch)
2017-08-09 15:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18292: Remove return 1 statements in tests (42.76 KB, patch)
2017-08-09 15:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18292: Tests do not need to return 1 - xt (649 bytes, patch)
2017-08-09 16:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18292: Tests do not need to return 1 - xt (694 bytes, patch)
2017-08-09 16:22 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1; (912 bytes, patch)
2017-08-11 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18292: Remove return 1 statements in tests (42.83 KB, patch)
2017-08-11 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18292: Tests do not need to return 1 - xt (792 bytes, patch)
2017-08-11 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2017-03-17 07:02:35 UTC

    
Comment 1 Olli-Antti Kivilahti 2017-03-17 07:06:08 UTC
Created attachment 61196 [details] [review]
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1;
Comment 2 Olli-Antti Kivilahti 2017-03-17 07:06:57 UTC
Initially thought the test fails because of this when ran via prove, but it was maybe something leaking between test cases.
Comment 3 Marc Véron 2017-03-17 08:12:58 UTC
What about the tons of *.t files with 1; as last line?

git grep '^1;' -- *.t
Comment 4 Olli-Antti Kivilahti 2017-03-17 08:15:11 UTC
(In reply to Marc Véron from comment #3)
> What about the tons of *.t files with 1; as last line?
> 
> git grep '^1;' -- *.t

This patch is just cosmetic and seems to have no impact on test results.

If I had a delete button for this issue I would delete this.
But as I don't, it is better to make some use of this ticket.
Comment 5 Mark Tompsett 2017-06-07 17:58:06 UTC
(In reply to Marc Véron from comment #3)
> What about the tons of *.t files with 1; as last line?
> 
> git grep '^1;' -- *.t

Comparing
git grep '^1;' -- *.t | wc
with
git grep -- *.t | wc

I believe 'tons' is an exaggeration. :)
This is a small patch. It doesn't hurt.

P.S.

for i in `git grep -l '^1;' -- *.t`; do git blame $i | tail -1 | cut -f2 -d'(' | cut -f1 -d2 | sed -e "s#\s*\$##g"; done | sort -u
Alex Sassmannshausen
A. Sassmannshausen
Charles Farmer
Emmanuele Somma
Galen Charlton
Jacek Ablewicz
Jared Camins-Esakov
Jonathan Druart
Kyle M Hall
Lari Taskula
Marcel de Rooy
Martin Renvoize
Paul Poulain
Ryan Higgins
Tomás Cohen Arazi
Comment 6 Mark Tompsett 2017-06-07 18:00:46 UTC
(In reply to Olli-Antti Kivilahti from comment #4)
> This patch is just cosmetic and seems to have no impact on test results.

Actually, while generally true, I think a microscopic time gain would happen if all of them were fixed. 1-2 less lines for 127ish out of 1000+ tests to load. Who knows if any of them trigger paging in the OS while running. ;)
Comment 7 Mark Tompsett 2017-06-07 18:02:31 UTC
(In reply to M. Tompsett from comment #5)
> Ryan Higgins

P.P.S. 10ish years ago. I suspect all of these are cut and paste from early in the project.
Comment 8 Mark Tompsett 2017-06-08 01:13:56 UTC
Created attachment 64097 [details] [review]
[SIGNED-OFF] Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1;

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Marcel de Rooy 2017-06-08 15:41:34 UTC
Created attachment 64109 [details] [review]
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1;

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Jonathan Druart 2017-06-09 11:49:25 UTC
Same for me, no need to fix only 1 occurrence when there are 115 left and a simple regex can get rid of all of them.
Patching is coming
Comment 11 Jonathan Druart 2017-06-09 12:01:20 UTC
Created attachment 64127 [details] [review]
Bug 18292: Remove return 1 statements in tests

The test files do not need to return 1

Patch generated with:
perl -p -i -e "s/^1;\n//xsm" t/**/*.t

Test plan:
  git grep '^1;$' t/**/*.t
should not return any results
Comment 12 Alex Buckley 2017-08-03 04:21:13 UTC
Hi Jonathan

Can you please merge your patches with master as they will not apply successfully, with the below error message being thrown:

Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/www/search_utf8.t
Auto-merging t/db_dependent/www/history.t
Auto-merging t/db_dependent/www/batch.t
Auto-merging t/db_dependent/Utils/Datatables_Members.t
CONFLICT (content): Merge conflict in t/db_dependent/Utils/Datatables_Members.t
Auto-merging t/db_dependent/Serials/ReNewSubscription.t
Auto-merging t/db_dependent/Search.t
Auto-merging t/db_dependent/Reserves/MultiplePerRecord.t
Auto-merging t/db_dependent/Patron/Borrower_PrevCheckout.t
Auto-merging t/db_dependent/Patron/Borrower_Discharge.t
Auto-merging t/db_dependent/Members/IssueSlip.t
Auto-merging t/db_dependent/Members/GetPendingIssues.t
Auto-merging t/db_dependent/Members/GetAllIssues.t
Auto-merging t/db_dependent/Koha_Elasticsearch_Indexer.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha_Elasticsearch_Indexer.t
Auto-merging t/db_dependent/Koha/Patrons.t
Auto-merging t/db_dependent/Koha/Objects.t
Auto-merging t/db_dependent/Circulation/SwitchOnSiteCheckouts.t
Auto-merging t/db_dependent/Circulation/Returns.t
Auto-merging t/db_dependent/Circulation/IsItemIssued.t
Auto-merging t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t
Auto-merging t/db_dependent/Circulation/CheckIfIssuedToPatron.t
Auto-merging t/db_dependent/Circulation/Branch.t
Auto-merging t/Charset.t
CONFLICT (content): Merge conflict in t/Charset.t
Failed to merge in the changes.
Patch failed at 0001 Bug 18292: Remove return 1 statements in tests
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18292-Remove-return-1-statements-in-tests-GqX5DD.patch
Comment 13 Jonathan Druart 2017-08-07 13:52:17 UTC
Created attachment 65554 [details] [review]
Bug 18292: Remove return 1 statements in tests

The test files do not need to return 1

Patch generated with:
perl -p -i -e "s/^1;\n//xsm" t/**/*.t

Test plan:
  git grep '^1;$' t/**/*.t
should not return any results
Comment 14 Katrin Fischer 2017-08-09 08:03:19 UTC
I've found a problem in Charset.t:

ok 16 - nsb_clean removes � and �
# Subtest: SetMarcUnicodeFlag
    1..2
    # No tests run!
not ok 17 - No tests run for subtest "SetMarcUnicodeFlag"
#   Failed test 'No tests run for subtest "SetMarcUnicodeFlag"'
#   at t/Charset.t line 95.
Undefined subroutine &t::lib::Mocks::mock_preference called at t/Charset.t line 83.
# Looks like your test exited with 255 just after 17.
Comment 15 Jonathan Druart 2017-08-09 13:08:11 UTC
Created attachment 65709 [details] [review]
Bug 18292: Remove return 1 statements in tests

The test files do not need to return 1

Patch generated with:
perl -p -i -e "s/^1;\n//xsm" t/**/*.t

Test plan:
  git grep '^1;$' t/**/*.t
should not return any results
Comment 16 Mark Tompsett 2017-08-09 15:52:54 UTC
Created attachment 65763 [details] [review]
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1;

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 17 Mark Tompsett 2017-08-09 15:53:05 UTC
Created attachment 65764 [details] [review]
Bug 18292: Remove return 1 statements in tests

The test files do not need to return 1

Patch generated with:
perl -p -i -e "s/^1;\n//xsm" t/**/*.t

Test plan:
  git grep '^1;$' t/**/*.t
should not return any results

NOTE: does not fix C4/SIP/t, nor xt tests.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 18 Jonathan Druart 2017-08-09 16:02:06 UTC
Created attachment 65766 [details] [review]
Bug 18292: Tests do not need to return 1 - xt
Comment 19 Mark Tompsett 2017-08-09 16:22:14 UTC
Created attachment 65768 [details] [review]
Bug 18292: Tests do not need to return 1 - xt

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 20 Marcel de Rooy 2017-08-11 08:42:49 UTC
(In reply to Katrin Fischer from comment #14)
> I've found a problem in Charset.t:
> 
> ok 16 - nsb_clean removes � and �
> # Subtest: SetMarcUnicodeFlag
>     1..2
>     # No tests run!
> not ok 17 - No tests run for subtest "SetMarcUnicodeFlag"
> #   Failed test 'No tests run for subtest "SetMarcUnicodeFlag"'
> #   at t/Charset.t line 95.
> Undefined subroutine &t::lib::Mocks::mock_preference called at t/Charset.t
> line 83.
> # Looks like your test exited with 255 just after 17.

Cannot reproduce it anymore..
Comment 21 Marcel de Rooy 2017-08-11 08:44:34 UTC
Created attachment 65879 [details] [review]
Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1;

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2017-08-11 08:44:39 UTC
Created attachment 65880 [details] [review]
Bug 18292: Remove return 1 statements in tests

The test files do not need to return 1

Patch generated with:
perl -p -i -e "s/^1;\n//xsm" t/**/*.t

Test plan:
  git grep '^1;$' t/**/*.t
should not return any results

NOTE: does not fix C4/SIP/t, nor xt tests.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2017-08-11 08:44:44 UTC
Created attachment 65881 [details] [review]
Bug 18292: Tests do not need to return 1 - xt

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Jonathan Druart 2017-08-15 15:26:33 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 25 Fridolin Somers 2017-09-29 10:04:22 UTC
Pushed to 17.05.x, will be in 17.05.05.

I push this enh to stable in order to avoid future rebase errors.
Comment 26 Katrin Fischer 2017-10-01 21:45:21 UTC
Patch set doesn't apply cleanly to 16.11.x. Please rebase if you want this to be included.