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.
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?
I think btw we could at least do: if( $cas ) { require C4::Auth_with_cas; # no import }
(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.
(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?
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.
> (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".
(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.
Created attachment 53279 [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
Created attachment 53731 [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
Why do you want to unset KOHA_CONF?
(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.
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>
Pushed to master - Should be in the November 2016 - Thanks!
Pushed in 16.05. Will be in 16.05.03.
Patch pushed to 3.22.x, will be in 3.22.10