Created attachment 51857 [details] [review] Bug 16607: Add quiet flag to builder sub in TestBuilder If you know what you are doing, you sometimes do not like to see the warnings raised by TestBuilder. This patch allows you to add a quiet flag to an individual build call. Test plan: [1] Add a CPL and a MPL branch (if not present). [2] Run t/db_dependent/TestBuilder.t. You should see no difference. The fix from bug 16500 (suppressing the warn already) has been replaced by using the quiet flag. [3] Run t/db_dependent/Accounts.t too. Before this patch, an existing MPL branch also raised a warn. [4] Run t/db_dependent/Items/AutomaticItemModificationByAge.t too. This test included a check for the existence of a CPL branch. The check has been removed. You should see no warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 51873 [details] [review] Bug 16607: Follow-up for Holds.t This test adds a patron category and should raise no warning if it already exists. The new quiet parameter is your friend. Test plan: Add patron category S. Run t/db_dependent/Holds.t and see no warnings. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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?
(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.
(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.
(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.
Created attachment 52302 [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>
Created attachment 52303 [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>
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>
Pushed to master for Koha 16.11, thanks Marcel!
Pushed in 16.05. Will be in 16.05.01.