Description
Mark Tompsett
2016-05-25 02:26:17 UTC
Created attachment 51758 [details] [review] Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available This patch makes it so 00-load.t doesn't cause 'prove t' to prematurely die before reaching other tests. TEST PLAN --------- 1) prove t -- dies on 00-load.t 2) apply patch 3) prove t -- now t/Prices.t should be the failure. Created attachment 51759 [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. Created attachment 51909 [details] [review] [SIGNED-OFF] Bug 16582 t/Price.t test should pass if Test::DBIx::Class is not available This patch makes it so 00-load.t doesn't cause 'prove t' to prematurely die before reaching other tests. TEST PLAN --------- 1) prove t -- dies on 00-load.t 2) apply patch 3) prove t -- now t/Prices.t should be the failure. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51910 [details] [review] [SIGNED-OFF] 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> 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? (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. (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,, Created attachment 51913 [details] [review] Bug 16582 [DO NOT PUSH] 'prove t' failure on 00-load.t This patch makes it so 00-load.t doesn't cause 'prove t' to prematurely die before reaching other tests. TEST PLAN --------- 1) prove t -- dies on 00-load.t 2) apply patch 3) prove t -- now t/Prices.t should be the failure. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51914 [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> 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 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. Why not move this test to dependent btw? Seeing several warns on Context and one on Templates. (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. (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) (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? (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? (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 :) (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. 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> Pushed to Master - Should be in the November 2016 release. Thanks Pushed in 16.05. Will be in 16.05.01. Patch pushed to 3.22.x, will be in 3.22.8 |