Bug 12603 - TestBuilder - Module to simplify the writing of tests
Summary: TestBuilder - Module to simplify the writing of tests
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Yohann Dufour
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 12605 12604 12606 12607 14195 14256
  Show dependency treegraph
 
Reported: 2014-07-18 14:05 UTC by Yohann Dufour
Modified: 2016-12-05 21:22 UTC (History)
7 users (show)

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


Attachments
Bug 12603: creating TestBuilder to simplify tests (20.35 KB, patch)
2014-07-18 14:27 UTC, Yohann Dufour
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12603: creating TestBuilder to simplify tests (20.43 KB, patch)
2014-07-25 12:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12603 [QA Followup] (5.01 KB, patch)
2014-07-25 12:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12603: creating TestBuilder to simplify tests (20.52 KB, patch)
2014-10-13 11:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12603 [QA Followup] (5.10 KB, patch)
2014-10-13 11:38 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 Yohann Dufour 2014-07-18 14:05:16 UTC
Hi,

With Jonathan Druart, we have been working on a module which allows to simplify the writing of tests by inserting all the foreign keys which are necessary to insert a new entry in the database.
For example, if you want to test an order, you have to create a biblio, a bookseller, a budget, etc. With this module, two lines are sufficient to create an order.
Comment 1 Yohann Dufour 2014-07-18 14:27:01 UTC Comment hidden (obsolete)
Comment 2 Zeno Tajoli 2014-07-18 15:00:50 UTC
Hi Yohann and Jonathan,

I don't understand very well the situation.
You find a problem in patch  bug 11518 (the FK circular refeence).
But the patch was pushedt to master at 29 April 2014.
Now it is present on master and on stable (3.16.x).

So why do you work on a old bug and you don't create a new bug with your fix.
Clearly with a see also to bug 11518.

Bye
Comment 3 Yohann Dufour 2014-07-18 15:18:05 UTC
You can see examples of the TestBuilder abilities in bug 12604, bug 12605, bug 12606, bug 12607
Comment 4 Jonathan Druart 2014-07-18 15:38:48 UTC
(In reply to Zeno Tajoli from comment #2)
> I don't understand very well the situation.
> You find a problem in patch  bug 11518 (the FK circular refeence).

Yohann was speaking about the last patch, not pushed. The way to solve the problem is not obvious. Any idea/feedback is welcomed, but not on this report.
Comment 5 Kyle M Hall 2014-07-25 12:47:32 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2014-07-25 12:47:44 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2014-08-15 15:36:27 UTC
I'm starting to get my head around this.. basically it sits between the database and a new test right.. and it does the adding to database for you such that you can not bother calling context and manually adding all the data per test.

I'm wondering if this abstraction could go further and allow a mock option such that said data gets pushed into a mocked object instead of the db upon request (or lack of a db to test against).. allowing to write one test for db dependant and independent..

Just some thoughts?
Comment 8 Mark Tompsett 2014-08-15 16:07:10 UTC
This seems like an abstraction that will detract momentum from those just starting to write tests. I'm not saying it isn't useful, but I see no need to refactor existing working tests to use it in the immediate future.
Comment 9 Jonathan Druart 2014-08-18 11:36:46 UTC
(In reply to M. Tompsett from comment #8)
> This seems like an abstraction that will detract momentum from those just
> starting to write tests. I'm not saying it isn't useful, but I see no need
> to refactor existing working tests to use it in the immediate future.

The need is to provide some examples on how use this module for further uses.
Comment 10 Martin Renvoize 2014-08-20 10:25:03 UTC
Comment on attachment 30086 [details] [review]
[SIGNED-OFF] Bug 12603: creating TestBuilder to simplify tests

Review of attachment 30086 [details] [review]:
-----------------------------------------------------------------

I'm reasonably happy that the code does what it's meant to, and goes some way toward making tests somewhat easier to write.

However, before accepting such a patch, I would like to see some efforts go into encouraging it's adoption for writing future tests.

By this I mean, adding a bit of additional off bug documentation around it; for instance a Wiki page clearly defining why using the module is a good idea, 'selling' it to other developers, and a clear set of use cases beyond re-factoring of current passing tests.

My gut still feels that db dependant tests should go hand in hand with db independent tests, so I'd still love to know if adding a Mock layer to this might be achievable.

I'm also not sure the module name is especially descriptive.. I'de prefer something like PopulateData or something.. I'll have a think on that one.  The module isn't 'building tests for you' it's populating the database with sensible stuff for you.

::: t/lib/TestBuilder.pm
@@ +320,5 @@
> +Koha module to insert the foreign keys automatically for the tests
> +
> +=head1 DESCRIPTION
> +
> +This module allows to insert automatically an entry in the database. All the database changes are wrapped in a transaction.

Perhaps change to: 'This module allows one to automatically populate the database with all relevant entries required to test a particular area of functionality. The chages are also wrapped in a transaction, to allow roll-back upon test completion.'

@@ +322,5 @@
> +=head1 DESCRIPTION
> +
> +This module allows to insert automatically an entry in the database. All the database changes are wrapped in a transaction.
> +The foreign keys are created according to the DBIx::Class schema.
> +The taken values are the values by default if it is possible or randomly generated.

I'm not exactly sure what you mean here; could you clarify please?
Comment 11 Yohann Dufour 2014-08-21 08:27:47 UTC
> @@ +322,5 @@
> > +=head1 DESCRIPTION
> > +
> > +This module allows to insert automatically an entry in the database. All the database changes are wrapped in a transaction.
> > +The foreign keys are created according to the DBIx::Class schema.
> > +The taken values are the values by default if it is possible or randomly generated.
> 
> I'm not exactly sure what you mean here; could you clarify please?

You right, it is not really clear. That's mean the inserted values in database fields are randomly generated if none was given as parameter.
Comment 12 Yohann Dufour 2014-08-21 08:36:41 UTC
Chris told us about the module Test::DBIx::Class::Factory (http://search.cpan.org/~gharper/Test-DBIx-Class-Factory-0.03/lib/Test/DBIx/Class/Factory.pm) which seems to do the same job as TestBuilder.

I've tested it, but only few type of database fields are working (timestamp, datetime, text, varchar, integer, tinyint and float)
Comment 13 Yohann Dufour 2014-08-21 09:49:07 UTC
Moreover, the major issue with Test::DBIx::Class::Factory is that you cannot create only the foreign keys for your object.

For instance, if I want to test the order in the acquisition module :
To create an order, I have to create first :
- a basket (which needs a bookseller)
- a budget
- a biblio

With TestBuilder, you can do something like that :
my $order = $builder->build({
    source => 'Aqorder',
    only_fk => 1,
})
The parameter only_fk allows one to insert in database only the foreign keys : basket, budget and biblio and it returns an hashref to create the order.
Then, you can call the NewOrder routine like that : NewOrder($order)

With Test::DBIx::Class::Factory, you can do something similar :
my $order = $factory->create_record('Aqorder')
But it inserts the order in the database, so you cannot test then the NewOrder routine since it is already in the database. Moreover, it returns a DBIx::Class Object which is not compatible with what expects the New or Add routines.

Thus, according to me, Test::DBIx::Class::Factory is not compatible with how we test the Koha modules.
Comment 14 Marcel de Rooy 2014-08-28 08:08:27 UTC
Koha/Schema/Result/ClosureRrule.pm: where does that come from? and is it a typo?
Comment 15 Marcel de Rooy 2014-08-28 08:13:25 UTC
(In reply to M. Tompsett from comment #8)
> This seems like an abstraction that will detract momentum from those just
> starting to write tests. I'm not saying it isn't useful, but I see no need
> to refactor existing working tests to use it in the immediate future.

I have the same idea when glancing thru this code.
Comment 16 Marcel de Rooy 2014-08-28 11:14:20 UTC
You modify a custom relation for biblioitems to has_one.
But note that we already have a has_many by default. (Which is theoretically correct.)
A proposed patch for bug 11518 removes this belongs_to.
I think we should not add the has_one. If we want to resolve this, just get rid of the intermediate table biblioitems.
Comment 17 Marcel de Rooy 2014-10-13 11:37:57 UTC
Created attachment 32285 [details] [review]
Bug 12603: creating TestBuilder to simplify tests

This patch contains a new module t::lib::TestBuilder which allows to write tests easier and it contains the unit tests of this module.
For more information, see the documentation of the module.

This module uses the DBIx::Class schema and works with a clean DBIx::Class schema. In order to use it, you have to remove the current circular dependence (existing in the DBIx::Class) by applying the last patch of the bug 11518.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2014-10-13 11:38:02 UTC
Created attachment 32286 [details] [review]
Bug 12603 [QA Followup]

* Fix syntax error
* Remove Schema files for nonexistant tables
* Fix circular dependency
  * Makes unpushed followup for bug 11518 unnessary

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2014-10-13 11:38:28 UTC
General commment:
The comment on has_one should not block this patch imo (see comment16). [Removing the belongs_to as in bug 11518 also addresses this problem.]
The code, including some less obvious things, contains very little comments (see also the comment of Martin about documentation).
The composite foreign keys like in the overduerules_transport_type are handled by a trick: copy the hash from the first composite key to the other one(s). The code in build() looks at the first key of a composite one when searching for values. Using TestBuilder with such a table is not (immediately) clear. (And obviously, code with such trics may not be the easiest to maintain..)
I suppose that a table with multiple composite foreign keys sharing the same key could present a problem. I could not find an example in Koha though ;)
Passes qa script.
I will pass QA on this code, but somewhat hesitant. Note that I do not view TestBuilder as the only way to write a test now in Koha and would not per se recommend refactoring of existing tests. It is just an alternative.

Small remarks:
+$my_overduerules_transport_type->{categorycode} = $my_overduerules_transport_type->{branchcode};
+my $overduerules_transport_type = $builder->build({
+    source => 'OverduerulesTransportType',
+    value  => $my_overduerules_transport_type,
+});
o my: my $overdue.. and $my_overdue.. Example of obscure variable naming?

+    $source =~ s|(\w+)$|$1|;
What are you actually changing here?

+our $default_value = {
+    UserPermission => {
TestBuilder.t contains the default value for UserPermission. Should not block this patch, but I would rather have it somewhere else. Separate module perhaps?
Comment 20 Tomás Cohen Arazi 2015-04-20 13:30:52 UTC
Feature pushed to master. This will make writing tests much easier.

Good job Yohann!
Comment 21 Olli-Antti Kivilahti 2015-06-22 13:15:14 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to M. Tompsett from comment #8)
> > This seems like an abstraction that will detract momentum from those just
> > starting to write tests. I'm not saying it isn't useful, but I see no need
> > to refactor existing working tests to use it in the immediate future.
> 
> I have the same idea when glancing thru this code.

Also we have the half finished Koha::Object and DBIx :)
Struggling with DBIx certainly detracts momentum.

I am really glad that some work to make writing tests happier is in motion though.
Comment 22 Marcel de Rooy 2015-06-22 13:21:38 UTC
(In reply to Olli-Antti Kivilahti from comment #21)
> Also we have the half finished Koha::Object and DBIx :)
> Struggling with DBIx certainly detracts momentum.

You are nominated for the Koha 2015 understatement ! :-)