Bug 16607

Summary: Remove CPL/MPL from two unit tests
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Test SuiteAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: minor    
Priority: P5 - low CC: f.demians, jonathan.druart, julian.maurice, kyle, veron
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16500
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11023, 16155    
Bug Blocks:    
Attachments: Bug 16607: Add quiet flag to builder sub in TestBuilder
Bug 16607: Follow-up for Holds.t
Bug 16607: Remove CPL/MPL from two unit tests
Bug 16607: Remove CPL/MPL from two unit tests
Bug 16607: Remove CPL/MPL from two unit tests

Description Marcel de Rooy 2016-05-26 20:03:49 UTC

    
Comment 1 Marcel de Rooy 2016-05-27 07:16:19 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-05-27 11:59:25 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-06-10 13:55:50 UTC
I don't think it's a good idea to hide these warnings.
If they appear it means that the developer is doing something wrong. Giving him the ability not to think but just add a quiet flag is not helping him :)
For instance, we should not specify the primary (or unique) key, but let TestBuilder generating it, don't you think?
Comment 4 Marcel de Rooy 2016-06-10 19:03:07 UTC
(In reply to Jonathan Druart from comment #3)
Thx for looking into this.

> I don't think it's a good idea to hide these warnings.

I agree in general. But if you have a closer look, you will see that this is only a one-time, non-recursive hide for one build call. (Generated recursive build calls already are verbose again.)

> If they appear it means that the developer is doing something wrong. Giving
> him the ability not to think but just add a quiet flag is not helping him :)
> For instance, we should not specify the primary (or unique) key, but let
> TestBuilder generating it, don't you think?

A developer could of course misuse this flag. He can do much more bad things :) This flag is intended to intelligently suppress one warning.
As example: 
    $builder->build( { source => 'Branch', value => { branchcode => 'CPL' }, quiet => 1 });
In this case the developer uses TestBuilder to INSERT IGNORE a CPL branch in just one line. He knows that TestBuilder will not insert it if it is already there and he does not want to see the warning. So he passes the quiet flag.
Normally, we do not specify primary keys in TestBuilder, but I would not like to block this specific use.
Comment 5 Jonathan Druart 2016-06-11 13:19:38 UTC
(In reply to Marcel de Rooy from comment #4)
> In this case the developer uses TestBuilder to INSERT IGNORE a CPL branch in
> just one line. He knows that TestBuilder will not insert it if it is already
> there and he does not want to see the warning.

This assumption is wrong, tests should create the data they need to pass and not rely on existing data.
Comment 6 Marcel de Rooy 2016-06-13 07:44:02 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> > In this case the developer uses TestBuilder to INSERT IGNORE a CPL branch in
> > just one line. He knows that TestBuilder will not insert it if it is already
> > there and he does not want to see the warning.
> 
> This assumption is wrong, tests should create the data they need to pass and
> not rely on existing data.

It was a nice flag, do not add it in the future ;)
I adjusted the Accounts.t test now. Hilarious actually.
Removed CPL/MPL from AutomaticItemModificationByAge.t.
The Holds.t patch goes on another report.
Comment 7 Marcel de Rooy 2016-06-13 07:57:49 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2016-06-13 08:16:38 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-06-13 16:51:43 UTC
Created attachment 52338 [details] [review]
Bug 16607: Remove CPL/MPL from two unit tests

Test t/db_dependent/Accounts.t:
This test created a MPL branch, but did not use it any more.

Test t/db_dependent/Items/AutomaticItemModificationByAge.t:
This test still used CPL/MPL branches. Now it uses two new branches.

Test plan:
[1] Run t/db_dependent/Accounts.t.
[2] Run t/db_dependent/Items/AutomaticItemModificationByAge.t.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Both tests pass OK
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Kyle M Hall 2016-06-17 14:44:55 UTC
Pushed to master for Koha 16.11, thanks Marcel!
Comment 11 Frédéric Demians 2016-06-24 05:46:01 UTC
Pushed in 16.05. Will be in 16.05.01.