Bug 14144

Summary: Silence warnings t/db_dependent/Auth_with_ldap.t
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: Test SuiteAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: minor    
Priority: P5 - low CC: hector.hecaxmmx, jonathan.druart, julian.maurice, kyle
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t
Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t
Bug 14144: Clean ups and refactors
[SIGNED-OFF]Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t
[SIGNED-OFF]Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t
[SIGNED-OFF]Bug 14144: Clean ups and refactors
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t
Bug 14144: Clean ups and refactors

Description Mark Tompsett 2015-05-05 02:00:41 UTC
prove t/db_dependent/Auth_with_ldap.t generates a shibboleth warning, it shouldn't.
Comment 1 Mark Tompsett 2015-05-05 02:05:57 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2015-05-05 02:20:52 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2016-04-04 23:02:05 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2016-04-04 23:02:36 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2016-04-04 23:02:42 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2016-04-04 23:15:45 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2016-04-04 23:15:52 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2016-04-05 00:13:41 UTC
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
Comment 9 Héctor Eduardo Castro Avalos 2016-04-21 18:11:44 UTC Comment hidden (obsolete)
Comment 10 Héctor Eduardo Castro Avalos 2016-04-21 18:11:51 UTC Comment hidden (obsolete)
Comment 11 Héctor Eduardo Castro Avalos 2016-04-21 18:11:57 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-04-23 07:23:28 UTC
> 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.
Comment 13 Mark Tompsett 2016-04-23 13:34:57 UTC
(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.
Comment 14 Jonathan Druart 2016-04-25 14:56:51 UTC
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
Comment 15 Mark Tompsett 2016-04-25 15:36:45 UTC
(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 16 Mark Tompsett 2016-04-25 15:37:28 UTC
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.
Comment 17 Mark Tompsett 2016-04-25 15:39:15 UTC
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=...
Comment 18 Kyle M Hall 2016-04-28 13:31:47 UTC
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>
Comment 19 Kyle M Hall 2016-04-28 13:31:54 UTC
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>
Comment 20 Kyle M Hall 2016-04-29 12:41:17 UTC
Pushed to master for the May 2016 release! Dziekuje Mark!
Comment 21 Julian Maurice 2016-05-16 10:06:29 UTC
Patch pushed to 3.22.x, will be in 3.22.7