Bug 16622 - some tests triggered by prove t fail for unset KOHA_CONF
Summary: some tests triggered by prove t fail for unset KOHA_CONF
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-31 14:33 UTC by Mark Tompsett
Modified: 2017-12-07 22:16 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16622: some tests triggered by prove t fail for unset KOHA_CONF (1.89 KB, patch)
2016-07-11 19:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16622: some tests triggered by prove t fail for unset KOHA_CONF (1.89 KB, patch)
2016-07-26 18:11 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16622: some tests triggered by prove t fail for unset KOHA_CONF (2.10 KB, patch)
2016-08-02 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2016-05-31 14:33:16 UTC
unset'ing KOHA_CONF from your environment, and then triggering 'prove t' results in several tests warning and complaining about C4::Context.

Make sure failing tests are in the correct location (t vs t/db_dependent), properly mocked, or catch the warnings. The second option being the preferred one.
Comment 1 Mark Tompsett 2016-06-01 04:55:43 UTC
This line in C4/Auth_with_cas.pm is the culprit of all those warns:

my $context = C4::Context->new() or die 'C4::Context->new failed';

Comment it out and run 'prove t' and you are back to the basic mess that is expected. Sadly, I believe it is necessary, even though $context is not used.

What do you think, Marcel?
Comment 2 Marcel de Rooy 2016-06-01 06:15:37 UTC
I think btw we could at least do:

    if( $cas ) {
        require C4::Auth_with_cas;    # no import
    }
Comment 3 Marcel de Rooy 2016-06-01 06:32:43 UTC
(In reply to M. Tompsett from comment #1)
> This line in C4/Auth_with_cas.pm is the culprit of all those warns:
> 
> my $context = C4::Context->new() or die 'C4::Context->new failed';
> 
> Comment it out and run 'prove t' and you are back to the basic mess that is
> expected. Sadly, I believe it is necessary, even though $context is not used.
> 
> What do you think, Marcel?

Just running Prices.t with this line changed to my $context; does not resolve my warnings with a missing KOHA_CONF.
If I am adding a fake KOHA_CONF without db password, no warns. (Should be kind of equal to stopping the sql server.)
So, more fundamental is: if we put a test in t and we do not want database access, should the test need existence of koha-conf (without database access info)? Of course, some tests may do completely without it. Should we mock it somehow in an early begin block?
Making a distinction between tests with conf and without would be a bridge too far.
Comment 4 Jacek Ablewicz 2016-06-01 09:13:37 UTC
(In reply to M. Tompsett from comment #1)
> This line in C4/Auth_with_cas.pm is the culprit of all those warns:
> 
> my $context = C4::Context->new() or die 'C4::Context->new failed';
> 
> Comment it out and run 'prove t' and you are back to the basic mess that is
> expected. Sadly, I believe it is necessary, even though $context is not used.

Hm, why do you think this call is necessary in C4/Auth_with_cas.pm?

It caught my attention because I was planning to post a patch which eliminates C4::Context->new() calls in C4/Auth_with_cas.pm and in Koha/Database.pm (config file is being parsed 3 times for an average Koha script for no aparent reason, this is bad for performance). I've concluded that both those calls are 100% redundant, but maybe I overlooked something?
Comment 5 Mark Tompsett 2016-06-01 14:54:42 UTC
This is crazy. I do a fresh git install on Debian SID (yes, I know it's SID), and install the elastic search stuff, and cpan2deb missing (or ones with too low versions) koha perl deps. Everything seems to actually be working as expected.

export unset KOHA_CONF
drop database koha_library;
sudo service mysql stop
-- possibly be more aggressive in making sure there is no DB. :)

prove t
-- dies horribly in 00-load.t
git bz apply 16618
prove t
-- finished, but fails because of Price.t
git bz apply 16582
prove t
-- some noise as expected, but passes.

The "Need Test::DBIx::Class" messages would make you think that they need the DB, but actually, they are only looking for the schema of the tables so they can mock test things.
Comment 6 Mark Tompsett 2016-06-01 14:59:27 UTC
> (M. Tompsett from comment #1)
> This line in C4/Auth_with_cas.pm is the culprit of all those warns:
> my $context = C4::Context->new() or die 'C4::Context->new failed';
> Comment it out and run 'prove t' and you are back to the basic mess that is
> expected. Sadly, I believe it is necessary, even though $context is not
> used.

> (Jacek Ablewicz from comment #4)
> Hm, why do you think this call is necessary in C4/Auth_with_cas.pm?
[SNIP]
> I've concluded that both those calls are 100% redundant, but maybe
> I overlooked something?

While you are likely correct, I didn't trace it completely, my worry is the order in which things are run. There may be an accidental order dependency in terms of the caching, object creation, or whatever. We don't have full test coverage, so I don't know if something will break. I believe there "... be dragons".
Comment 7 Mark Tompsett 2016-07-11 17:56:06 UTC
(In reply to Jacek Ablewicz from comment #4)
> (In reply to M. Tompsett from comment #1)
> > This line in C4/Auth_with_cas.pm is the culprit of all those warns:
> > my $context = C4::Context->new() or die 'C4::Context->new failed';
> > ... Sadly, I believe it is necessary, even though $context is not used.
> 
> Hm, why do you think this call is necessary in C4/Auth_with_cas.pm?

I've re-evaluated whether it is necessary. I think the point of the line was to prevent further cascading errors.

Auth_with_cas uses:
intranetdir from the KOHA_CONF
casServerUrl from the system preferences table
OPACBaseURL from the system preferences table
staffClientBaseURL from the system preferences table

A simple use (as triggered by 00-load.t) should be fine with the line removed. A more complex use should bomb, so the problems can get fixed. :) Sorry for the worry about dragons, Jacek Ablewicz.
Comment 8 Mark Tompsett 2016-07-11 19:33:39 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2016-07-26 18:11:57 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-08-01 09:26:40 UTC
Why do you want to unset KOHA_CONF?
Comment 11 Mark Tompsett 2016-08-01 13:44:58 UTC
(In reply to Jonathan Druart from comment #10)
> Why do you want to unset KOHA_CONF?

You shouldn't. But the stranger question is... why is it only 2 tests that fail? We should have tests or set of tests that purposefully fail in this case.
Comment 12 Jonathan Druart 2016-08-02 08:33:36 UTC
Created attachment 53873 [details] [review]
Bug 16622: some tests triggered by prove t fail for unset KOHA_CONF

TEST PLAN
---------
1) unset KOHA_CONF
2) prove t
   -- 00-load.t dies miserably
3) prove t/Creators.t
   -- fails
4) apply patch
5) prove t
   -- noisy, but all tests successful
6) prove -v t/Creators.t
   -- 2 skipped tests
7) run koha qa test tools

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Brendan Gallagher 2016-08-06 18:14:07 UTC
Pushed to master - Should be in the November 2016 - Thanks!
Comment 14 Frédéric Demians 2016-08-07 07:37:00 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 15 Frédéric Demians 2016-08-07 07:38:02 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 16 Julian Maurice 2016-08-17 13:53:49 UTC
Patch pushed to 3.22.x, will be in 3.22.10