Bug 16607 - Remove CPL/MPL from two unit tests
Summary: Remove CPL/MPL from two unit tests
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Galen Charlton
URL:
Keywords:
Depends on: 11023 16155
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-26 20:03 UTC by Marcel de Rooy
Modified: 2017-12-07 22:17 UTC (History)
5 users (show)

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


Attachments
Bug 16607: Add quiet flag to builder sub in TestBuilder (6.97 KB, patch)
2016-05-27 07:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16607: Follow-up for Holds.t (992 bytes, patch)
2016-05-27 11:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16607: Remove CPL/MPL from two unit tests (3.19 KB, patch)
2016-06-13 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16607: Remove CPL/MPL from two unit tests (3.30 KB, patch)
2016-06-13 08:16 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16607: Remove CPL/MPL from two unit tests (3.37 KB, patch)
2016-06-13 16:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.