Calls to C4/Charset.pm's NormalizeString function with an undefined string was triggering errors when running: prove -v t/db_dependent/Holds.t Sadly, t/Charset.t is lacking any function calls to NormalizeString. Silence the errors, and add minimal tests to t/Charset.t for testing.
Created attachment 32282 [details] [review] Bug 13075 - Silence warnings and improve Charset testing. Calls to C4/Charset.pm's NormalizeString function with an undefined string were triggering warnings when running: prove -v t/db_dependent/Holds.t Sadly, t/Charset.t was also lacking calls to NormalizeString. TEST PLAN --------- 1) prove -v t/db_dependent/Holds.t -- This should generate the uninitialized string warnings. Make sure CPL and MPL are in your branches to save yourself from headaches due to expected data. 2) cat t/Charset.t -- note there are no function calls to NormalizeString. You can see other shortfalls in the tests beyond NormalizeString with: grep ^sub C4/Charset.pm 3) prove -v t/Charset.t 4) Apply patch 5) prove -v t/Charset.t -- Run as before with more tests. 6) cat t/Charset.t -- note there are now function calls to NormalizeString. 7) prove -v t/db_dependent/Holds.t -- Nice and clean run! :) 8) koha-qa.pl -v 2 -c 1 -- all should be Ok.
Comment on attachment 32282 [details] [review] Bug 13075 - Silence warnings and improve Charset testing. Review of attachment 32282 [details] [review]: ----------------------------------------------------------------- ::: C4/Charset.pm @@ +178,4 @@ > > sub NormalizeString{ > my ($string,$nfd,$transform)=@_; > + return $string if !defined($string); could we write this better as return unless $string; Or should we be actually croaking if we call this without actually passing it anything to normalize, instead of silently hiding that we are calling it wrong?
(In reply to Chris Cormack from comment #2) > > + return $string if !defined($string); > > could we write this better as return unless $string; return unless defined($string), perhaps. String could be 0 or q{}. > Or should we be actually croaking if we call this without actually passing > it anything to normalize, instead of silently hiding that we are calling it > wrong? I did not determine why undef was passed, but perhaps further investigation as to why undef is passed would clarify if croak is necessary. Seeing as no other warnings were generated with this solution, I don't think it is. It should be noted that NFC(undef) and NFD(undef) both return the empty string, so perhaps another solution (possibly better?) is: $string //= q{}; Though, I think it is rather ugly to mangle an undef into an empty string to avoid warnings. That is why I choose this current solution. I'll rework this to the 'unless' idea that you suggested, Chris. -- time passes -- While doing this, I discovered that a NormalizeString call is pushed into an array, and so the context is a list, so return unless defined($string); triggers warnings while trying to run prove -v t/db_dependent/Holds.t for testing. Inserting an explicit $string (return $string unless defined($string)) forces a scalar return, and prevents warnings.
Created attachment 32326 [details] [review] Bug 13075 - Silence warnings and improve Charset testing. Calls to C4/Charset.pm's NormalizeString function with an undefined string were triggering warnings when running: prove -v t/db_dependent/Holds.t Sadly, t/Charset.t was also lacking calls to NormalizeString. TEST PLAN --------- 1) prove -v t/db_dependent/Holds.t -- This should generate the uninitialized string warnings. Make sure CPL and MPL are in your branches to save yourself from headaches due to expected data. 2) cat t/Charset.t -- note there are no function calls to NormalizeString. You can see other shortfalls in the tests beyond NormalizeString with: grep ^sub C4/Charset.pm 3) prove -v t/Charset.t 4) Apply patch 5) prove -v t/Charset.t -- Run as before with more tests. 6) cat t/Charset.t -- note there are now function calls to NormalizeString. 7) prove -v t/db_dependent/Holds.t -- Nice and clean run! :) 8) koha-qa.pl -v 2 -c 1 -- all should be Ok.
Auto-merging t/Charset.t CONFLICT (content): Merge conflict in t/Charset.t Auto-merging C4/Charset.pm Failed to merge in the changes.
Created attachment 32635 [details] [review] Bug 13075 - Silence warnings and improve Charset testing. Calls to C4/Charset.pm's NormalizeString function with an undefined string were triggering warnings when running: prove -v t/db_dependent/Holds.t Sadly, t/Charset.t was also lacking calls to NormalizeString. TEST PLAN --------- 1) prove -v t/db_dependent/Holds.t -- This should generate the uninitialized string warnings. Make sure CPL and MPL are in your branches to save yourself from headaches due to expected data. 2) cat t/Charset.t -- note there are no function calls to NormalizeString. You can see other shortfalls in the tests beyond NormalizeString with: grep ^sub C4/Charset.pm 3) prove -v t/Charset.t 4) Apply patch 5) prove -v t/Charset.t -- Run as before with more tests. 6) cat t/Charset.t -- note there are now function calls to NormalizeString. 7) prove -v t/db_dependent/Holds.t -- Nice and clean run! :) 8) koha-qa.pl -v 2 -c 1 -- all should be Ok.
Rebased. Another bug fix had added an additional test.
Oops. Still working on it.
Nope. All good. Less speed, more haste. :) This needs testing as per the test plan in comment #6.
Created attachment 33503 [details] [review] [SIGNED-OFF] Bug 13075: Silence warnings and improve Charset testing. Calls to C4/Charset.pm's NormalizeString function with an undefined string were triggering warnings when running: prove -v t/db_dependent/Holds.t Sadly, t/Charset.t was also lacking calls to NormalizeString. TEST PLAN --------- 1) prove -v t/db_dependent/Holds.t -- This should generate the uninitialized string warnings. Make sure CPL and MPL are in your branches to save yourself from headaches due to expected data. 2) cat t/Charset.t -- note there are no function calls to NormalizeString. You can see other shortfalls in the tests beyond NormalizeString with: grep ^sub C4/Charset.pm 3) prove -v t/Charset.t 4) Apply patch 5) prove -v t/Charset.t -- Run as before with more tests. 6) cat t/Charset.t -- note there are now function calls to NormalizeString. 7) prove -v t/db_dependent/Holds.t -- Nice and clean run! :) 8) koha-qa.pl -v 2 -c 1 -- all should be Ok. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Created attachment 33504 [details] [review] Bug 13075: (followup) remove remaining warnings There's no point creating a MARC record with undef subfields for testing holds. This patch avoids that so no warnings are shown. To test: - Run $ prove t/db_dependent/Holds.t => FAIL: verify several warnings show - Apply the patch - Re-run => SUCCESS: no warnings showed. - Sign off :-D Regards
Created attachment 33505 [details] [review] [SIGNED OFF] Bug 13075: (followup) remove remaining warnings There's no point creating a MARC record with undef subfields for testing holds. This patch avoids that so no warnings are shown. To test: - Run $ prove t/db_dependent/Holds.t => FAIL: verify several warnings show - Apply the patch - Re-run => SUCCESS: no warnings showed. - Sign off :-D Regards NOTE: Not noticable under Ubuntu 12.04 LTS, but verifiable under Debian Wheezy. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 33532 [details] [review] [PASSED QA] Bug 13075: Silence warnings and improve Charset testing. Calls to C4/Charset.pm's NormalizeString function with an undefined string were triggering warnings when running: prove -v t/db_dependent/Holds.t Sadly, t/Charset.t was also lacking calls to NormalizeString. TEST PLAN --------- 1) prove -v t/db_dependent/Holds.t -- This should generate the uninitialized string warnings. Make sure CPL and MPL are in your branches to save yourself from headaches due to expected data. 2) cat t/Charset.t -- note there are no function calls to NormalizeString. You can see other shortfalls in the tests beyond NormalizeString with: grep ^sub C4/Charset.pm 3) prove -v t/Charset.t 4) Apply patch 5) prove -v t/Charset.t -- Run as before with more tests. 6) cat t/Charset.t -- note there are now function calls to NormalizeString. 7) prove -v t/db_dependent/Holds.t -- Nice and clean run! :) 8) koha-qa.pl -v 2 -c 1 -- all should be Ok. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33533 [details] [review] [PASSED QA] Bug 13075: (followup) remove remaining warnings There's no point creating a MARC record with undef subfields for testing holds. This patch avoids that so no warnings are shown. To test: - Run $ prove t/db_dependent/Holds.t => FAIL: verify several warnings show - Apply the patch - Re-run => SUCCESS: no warnings showed. - Sign off :-D Regards NOTE: Not noticable under Ubuntu 12.04 LTS, but verifiable under Debian Wheezy. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patches pushed to master. Thanks Mark!
*** Bug 12928 has been marked as a duplicate of this bug. ***