|
Description
Mark Tompsett
2015-05-05 02:00:41 UTC
Created attachment 38835 [details] [review] Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t TEST PLAN ---------- 1) prove t/db_dependent/Auth_with_ldap.t -- there is a shibboleth related warning 2) apply patch 3) prove t/db_dependent/Auth_with_ldap.t -- now there is not. 4) koha qa test tools. Created attachment 38836 [details] [review] Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t TEST PLAN ---------- 1) prove t/db_dependent/Auth_with_ldap.t -- there is a shibboleth related warning 2) apply patch 3) prove t/db_dependent/Auth_with_ldap.t -- now there is not. 4) koha qa test tools. NOTE: Further investigation determined this would merely delay it, as the function is not called by this test. A delay is better than nothing. Created attachment 49882 [details] [review] Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t Revisiting this bug, I noticed that C4::Context->config() was mocked poorly. I expanded the mock function a little and have quieted the test which seemed to be working in a broken manner before. TEST PLAN --------- 1) prove t/db_dependend/Auth_with_ldap.t -- shibboleth noise 2) apply patch 3) prove t/db_dependend/Auth_with_ldap.t -- no noise 4) run koha qa test tools Created attachment 49883 [details] [review] Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t Revisiting this bug, I noticed that C4::Context->config() was mocked poorly. I expanded the mock function a little and have quieted the test which seemed to be working in a broken manner before. TEST PLAN --------- 1) prove t/db_dependend/Auth_with_ldap.t -- shibboleth noise 2) apply patch 3) prove t/db_dependend/Auth_with_ldap.t -- no noise 4) run koha qa test tools Created attachment 49884 [details] [review] Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t A little perlcritic cleanup and perltidy Created attachment 49885 [details] [review] Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t Revisiting this bug, I noticed that C4::Context->config() was mocked poorly. I expanded the mock function a little and have quieted the test which seemed to be working in a broken manner before. TEST PLAN --------- 1) prove t/db_dependend/Auth_with_ldap.t -- shibboleth noise 2) apply this patch 3) prove t/db_dependend/Auth_with_ldap.t -- no noise 4) run koha qa test tools Created attachment 49886 [details] [review] Bug 14144: Clean ups and refactors perltidy old new calls to new call format attrType -> attr_type (perlcritic friendlier) double quotes to single quotes '' combinations to q{} (perlcritic friendlier) refactored parameters to mock_net_ldap_search into a HASH This piece is not necessary, but I think it is nicer. There are three patches here. I believe that the second one is the best solution. The third one is clean up and refactors and is not necessary. All three can be applied at once. The third requires the second one first. The first can be applied alone. Testing is merely a prove t/db_dependent/Auth_with_ldap.t Created attachment 50538 [details] [review] [SIGNED-OFF]Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t TEST PLAN ---------- 1) prove t/db_dependent/Auth_with_ldap.t -- there is a shibboleth related warning 2) apply patch 3) prove t/db_dependent/Auth_with_ldap.t -- now there is not. 4) koha qa test tools. NOTE: Further investigation determined this would merely delay it, as the function is not called by this test. A delay is better than nothing. Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Created attachment 50539 [details] [review] [SIGNED-OFF]Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t Revisiting this bug, I noticed that C4::Context->config() was mocked poorly. I expanded the mock function a little and have quieted the test which seemed to be working in a broken manner before. TEST PLAN --------- 1) prove t/db_dependend/Auth_with_ldap.t -- shibboleth noise 2) apply this patch 3) prove t/db_dependend/Auth_with_ldap.t -- no noise 4) run koha qa test tools Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Created attachment 50540 [details] [review] [SIGNED-OFF]Bug 14144: Clean ups and refactors perltidy old new calls to new call format attrType -> attr_type (perlcritic friendlier) double quotes to single quotes '' combinations to q{} (perlcritic friendlier) refactored parameters to mock_net_ldap_search into a HASH This piece is not necessary, but I think it is nicer. Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as expected. No koha-qa erros > NOTE: Further investigation determined this would merely
> delay it, as the function is not called by this test.
> A delay is better than nothing.
I do not have the time right now to investigate but I disagree with this note.
The carp occurs because some shibboleth stuffs are done in the BEGIN block of C4::Auth.
I don't think we want to skip the evaluation at compilation time here for this module.
(In reply to Jonathan Druart from comment #12) [SNIP disagreement with NOTE] > The carp occurs because some shibboleth stuffs are done in the BEGIN block > of C4::Auth. > I don't think we want to skip the evaluation at compilation time here for > this module. Which is why I revisited the bug. Just apply the last two. So should the first patch be obsoleted?
If I apply the 2 last patches, then:
% perltidy t/db_dependent/Auth_with_ldap.t
% perltidy --profile=xt/perltidyrc t/db_dependent/Auth_with_ldap.t
% diff t/db_dependent/Auth_with_ldap.t{,.tdy} | wc -l
I get 186
(In reply to Jonathan Druart from comment #14) > So should the first patch be obsoleted? > > If I apply the 2 last patches, then: > % perltidy t/db_dependent/Auth_with_ldap.t > % perltidy --profile=xt/perltidyrc t/db_dependent/Auth_with_ldap.t > % diff t/db_dependent/Auth_with_ldap.t{,.tdy} | wc -l > I get 186 So, I retested. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12267#c19 Yes, I'll obsolete it. And lastly, I didn't use that perltidyrc. I used whatever is default in Debian 8, such that the errors in perlcritic -1 where ones I didn't care about and it didn't complain about being untidy. Comment on attachment 50538 [details] [review] [SIGNED-OFF]Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t Compile time vs. Run time error checking. Not really a good solution. Switching to signed off, since the sign off pre-dated the busting by bug 12267 requiring the schema files to be regenerated. ./misc/devel/update_dbix_class_files.pl -db_name=koha_library -db_user=... -db_passwd=... Created attachment 50920 [details] [review] Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t Revisiting this bug, I noticed that C4::Context->config() was mocked poorly. I expanded the mock function a little and have quieted the test which seemed to be working in a broken manner before. TEST PLAN --------- 1) prove t/db_dependend/Auth_with_ldap.t -- shibboleth noise 2) apply this patch 3) prove t/db_dependend/Auth_with_ldap.t -- no noise 4) run koha qa test tools Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 50921 [details] [review] Bug 14144: Clean ups and refactors perltidy old new calls to new call format attrType -> attr_type (perlcritic friendlier) double quotes to single quotes '' combinations to q{} (perlcritic friendlier) refactored parameters to mock_net_ldap_search into a HASH This piece is not necessary, but I think it is nicer. Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as expected. No koha-qa erros Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to master for the May 2016 release! Dziekuje Mark! Patch pushed to 3.22.x, will be in 3.22.7 |