Bug 16582 - t/Price.t test should pass if Test::DBIx::Class is not available
Summary: t/Price.t test should pass if Test::DBIx::Class is not available
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-25 02:26 UTC by Mark Tompsett
Modified: 2017-12-07 22:16 UTC (History)
7 users (show)

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


Attachments
Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available (1017 bytes, patch)
2016-05-25 02:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available (1.23 KB, patch)
2016-05-25 02:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available (1.04 KB, patch)
2016-05-30 05:00 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available (1.28 KB, patch)
2016-05-30 05:03 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16582 [DO NOT PUSH] 'prove t' failure on 00-load.t (1.02 KB, patch)
2016-05-30 10:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available (1.28 KB, patch)
2016-05-30 10:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available (1.38 KB, patch)
2016-06-01 06:04 UTC, Marcel de Rooy
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-25 02:26:17 UTC
I just spent hours doing  git clone to attempt the install pain under Ubuntu 16.04 LTS to see what the problem is, and the tests were still bugging me.

I'm going to attach 2 commits. The first is just to get the load test to keep going, and should not be used. The second will fill the t/Price.t in a manner similar to t/Members_AttributeTypes.t in bug 15150 was fixed.

TEST PLAN
---------
1) try prove t
   -- load will bomb
2) apply first patch
   -- prices will fail
3) apply second patch
   -- prices will "succeed"
4) run koha qa test tools
Comment 1 Mark Tompsett 2016-05-25 02:36:32 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2016-05-25 02:36:36 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2016-05-30 05:00:28 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2016-05-30 05:03:45 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2016-05-30 05:09:56 UTC
Just a couple of generic questions/musings not entirely to do with this patch:

* Wouldn't it be better to rename tests that we don't want to .t.NOT_READY_YET or similar instead of doing those regexes in load.t?

* I don't understand why use_ok are in the BEGIN block?

* maybe (if not too much of a hassle) we could leave use_ok for Test::DBIx::Class dependant tests, and skip the rest?
Comment 6 Marcel de Rooy 2016-05-30 08:39:22 UTC
(In reply to Srdjan Jankovic from comment #5)
> Just a couple of generic questions/musings not entirely to do with this
> patch:
> 
> * Wouldn't it be better to rename tests that we don't want to
> .t.NOT_READY_YET or similar instead of doing those regexes in load.t?

We should fix the tests or put some conditional skip in those tests.
I don't favor the regexs in load.t.

PS A new name for the first patch would be helpful.
Comment 7 Marcel de Rooy 2016-05-30 08:42:42 UTC
(In reply to Marcel de Rooy from comment #6)
> We should fix the tests or put some conditional skip in those tests.
> I don't favor the regexs in load.t.
Well probably not possible,,
Comment 8 Mark Tompsett 2016-05-30 10:14:57 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2016-05-30 10:15:21 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2016-05-30 10:23:10 UTC
As per comment #6's PS, first one is renamed.

As per comment #5: "I don't understand why use_ok are in the BEGIN block?"
-- I wondered the same thing.

"A BEGIN code block is executed as soon as possible, that is, the moment it is completely defined, even before the rest of the containing file (or string) is parsed. You may have multiple BEGIN blocks within a file (or eval'ed string); they will execute in order of definition. Because a BEGIN code block executes immediately, it can pull in definitions of subroutines and such from other files in time to be visible to the rest of the compile and run time. Once a BEGIN has run, it is immediately undefined and any code it used is returned to Perl's memory pool."

So, perhaps a memory optimization?
Comment 11 Mark Tompsett 2016-05-30 16:40:02 UTC
Comment on attachment 51913 [details] [review]
Bug 16582 [DO NOT PUSH] 'prove t' failure on 00-load.t

While similar to bug 9054, regular expressions like this don't actually show the real problem of missing dependencies. And if those dependencies are completely optional, we shouldn't even test. This is handled better on bug 16618 now. No need to keep this first patch around. The second can be tested directly.
Comment 12 Marcel de Rooy 2016-05-31 09:36:37 UTC
Why not move this test to dependent btw?
Seeing several warns on Context and one on Templates.
Comment 13 Mark Tompsett 2016-05-31 13:28:18 UTC
(In reply to Marcel de Rooy from comment #12)
> Why not move this test to dependent btw?
> Seeing several warns on Context and one on Templates.

What warns? I see a bunch of mock modules and functions. If I knew what you were refering to, then perhaps I would understand why you said this. Otherwise, with the mocks in place, it should be where it is.
Comment 14 Marcel de Rooy 2016-05-31 13:38:58 UTC
(In reply to M. Tompsett from comment #13)
> (In reply to Marcel de Rooy from comment #12)
> > Why not move this test to dependent btw?
> > Seeing several warns on Context and one on Templates.
> 
> What warns? I see a bunch of mock modules and functions. If I knew what you
> were refering to, then perhaps I would understand why you said this.
> Otherwise, with the mocks in place, it should be where it is.

Run the test. I see these warnings from Context.pm (and Templates.pm)
Comment 15 Mark Tompsett 2016-05-31 13:54:21 UTC
(In reply to Marcel de Rooy from comment #14)
> Run the test. I see these warnings from Context.pm (and Templates.pm)

What warnings? Perhaps I am lacking data to trigger them?
Comment 16 Marcel de Rooy 2016-05-31 13:58:07 UTC
(In reply to M. Tompsett from comment #15)
> (In reply to Marcel de Rooy from comment #14)
> > Run the test. I see these warnings from Context.pm (and Templates.pm)
> 
> What warnings? Perhaps I am lacking data to trigger them?

Remove your KOHA_CONF since you are supposed to have no database?
Comment 17 Marcel de Rooy 2016-05-31 13:58:27 UTC
(In reply to Marcel de Rooy from comment #16)
> (In reply to M. Tompsett from comment #15)
> > (In reply to Marcel de Rooy from comment #14)
> > > Run the test. I see these warnings from Context.pm (and Templates.pm)
> > 
> > What warnings? Perhaps I am lacking data to trigger them?
> 
> Remove your KOHA_CONF since you are supposed to have no database?

from the env :)
Comment 18 Mark Tompsett 2016-05-31 14:29:32 UTC
(In reply to Marcel de Rooy from comment #16)
> (In reply to M. Tompsett from comment #15)
> > (In reply to Marcel de Rooy from comment #14)
> > > Run the test. I see these warnings from Context.pm (and Templates.pm)
> > 
> > What warnings? Perhaps I am lacking data to trigger them?
> 
> Remove your KOHA_CONF since you are supposed to have no database?

It seems that 'prove t' demonstrates this is a larger problem than just Price.t

I would recommend moving that issue to another bug report, and moving forward with this one.
Comment 19 Marcel de Rooy 2016-06-01 06:04:34 UTC
Created attachment 51941 [details] [review]
Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available

TEST PLAN
---------
1) prove t/Prices.t
   -- failure
2) apply patch
3) prove t/Prices.t
   -- nicely skipped when Test::DBIx::Class is not available.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Brendan Gallagher 2016-06-03 07:31:31 UTC
Pushed to Master - Should be in the November 2016 release. Thanks
Comment 21 Frédéric Demians 2016-06-15 07:34:26 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 22 Julian Maurice 2016-06-16 07:59:39 UTC
Patch pushed to 3.22.x, will be in 3.22.8