Bug 14144 - Silence warnings t/db_dependent/Auth_with_ldap.t
Summary: Silence warnings t/db_dependent/Auth_with_ldap.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-05 02:00 UTC by Mark Tompsett
Modified: 2016-12-05 21:25 UTC (History)
4 users (show)

See Also:
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 (1.42 KB, patch)
2015-05-05 02:05 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t (1.58 KB, patch)
2015-05-05 02:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t (3.62 KB, patch)
2016-04-04 23:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t (3.62 KB, patch)
2016-04-04 23:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t (23.10 KB, patch)
2016-04-04 23:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t (3.62 KB, patch)
2016-04-04 23:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14144: Clean ups and refactors (23.31 KB, patch)
2016-04-04 23:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14144: Silence warnings in t/db_dependent/Auth_with_ldap.t (1.65 KB, patch)
2016-04-21 18:11 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t (3.70 KB, patch)
2016-04-21 18:11 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14144: Clean ups and refactors (23.42 KB, patch)
2016-04-21 18:11 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 14144: Silence warnings t/db_dependent/Auth_with_ldap.t (3.74 KB, patch)
2016-04-28 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14144: Clean ups and refactors (23.46 KB, patch)
2016-04-28 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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