|
Description
Olli-Antti Kivilahti
2017-03-17 07:02:35 UTC
Created attachment 61196 [details] [review] Bug 18292 - t/db_dependent/Circulation.t doesn't need to return 1; Initially thought the test fails because of this when ran via prove, but it was maybe something leaking between test cases. What about the tons of *.t files with 1; as last line? git grep '^1;' -- *.t (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. (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 (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. ;) (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. 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> 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> 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 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 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 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 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.
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 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> 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> Created attachment 65766 [details] [review] Bug 18292: Tests do not need to return 1 - xt Created attachment 65768 [details] [review] Bug 18292: Tests do not need to return 1 - xt Signed-off-by: Mark Tompsett <mtompset@hotmail.com> (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.. 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> 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> 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> Pushed to master for 17.11, thanks to everybody involved! Pushed to 17.05.x, will be in 17.05.05. I push this enh to stable in order to avoid future rebase errors. Patch set doesn't apply cleanly to 16.11.x. Please rebase if you want this to be included. |