[Bug 16622] New: some tests triggered by prove t fail for unset KOHA_CONF
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Bug ID: 16622 Summary: some tests triggered by prove t fail for unset KOHA_CONF Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Test Suite Assignee: chris@bigballofwax.co.nz Reporter: mtompset@hotmail.com QA Contact: gmcharlt@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16582 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor Assignee|chris@bigballofwax.co.nz |mtompset@hotmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|NEW |In Discussion --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I think btw we could at least do: if( $cas ) { require C4::Auth_with_cas; # no import } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl --- Comment #4 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #6 from M. Tompsett <mtompset@hotmail.com> ---
(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". -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 53279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53279&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53279|0 |1 is obsolete| | --- Comment #9 from Marc Véron <veron@veron.ch> --- Created attachment 53731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53731&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Why do you want to unset KOHA_CONF? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53731|0 |1 is obsolete| | --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53873 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53873&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |brendan@bywatersolutions.co | |m Status|Passed QA |Pushed to Master --- Comment #13 from Brendan Gallagher <brendan@bywatersolutions.com> --- Pushed to master - Should be in the November 2016 - Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr Status|Pushed to Master |Pushed to Stable --- Comment #14 from Frédéric Demians <frederic@tamil.fr> --- Pushed in 16.05. Will be in 16.05.03. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |Pushed to Master --- Comment #15 from Frédéric Demians <frederic@tamil.fr> --- Pushed in 16.05. Will be in 16.05.03. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16622 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #16 from Julian Maurice <julian.maurice@biblibre.com> --- Patch pushed to 3.22.x, will be in 3.22.10 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org