Bug 16155 - Composite keys in TestBuilder and more
Summary: Composite keys in TestBuilder and more
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: Marcel de Rooy
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 16331 (view as bug list)
Depends on:
Blocks: 16320 16500 16607 16868
  Show dependency treegraph
 
Reported: 2016-03-29 12:57 UTC by Marcel de Rooy
Modified: 2022-07-11 23:35 UTC (History)
4 users (show)

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


Attachments
Bug 16155: [DO_NOT_PUSH] OldBehavior.t (3.29 KB, patch)
2016-03-29 14:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Composite keys in TestBuilder and more (30.79 KB, patch)
2016-03-29 14:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Remove a second use from Members_Attributes.t (800 bytes, patch)
2016-03-29 14:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Adjust TestBuilder.t (19.72 KB, patch)
2016-03-29 14:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Adjust a few other tests (5.29 KB, patch)
2016-03-29 14:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Composite keys in TestBuilder and more (30.86 KB, patch)
2016-04-21 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Remove a second use from Members_Attributes.t (800 bytes, patch)
2016-04-21 07:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Adjust TestBuilder.t (19.72 KB, patch)
2016-04-21 07:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: Adjust a few other tests (7.40 KB, patch)
2016-04-21 07:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: [QA Follow-up] Add transaction to BiblioFrameworks.t (1.52 KB, patch)
2016-04-21 07:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16155: (QA followup) fix small bug in t/db_dependent/ILSDI_Services.t (867 bytes, patch)
2016-04-21 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16155: Composite keys in TestBuilder and more (30.90 KB, patch)
2016-04-21 14:50 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16155: Remove a second use from Members_Attributes.t (832 bytes, patch)
2016-04-21 14:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16155: Adjust TestBuilder.t (19.75 KB, patch)
2016-04-21 14:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16155: Adjust a few other tests (7.42 KB, patch)
2016-04-21 14:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16155: [QA Follow-up] Add transaction to BiblioFrameworks.t (1.55 KB, patch)
2016-04-21 14:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16155: (QA followup) fix small bug in t/db_dependent/ILSDI_Services.t (1.05 KB, patch)
2016-04-21 14:51 UTC, Bernardo Gonzalez Kriegel
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-03-29 12:57:01 UTC

    
Comment 1 Marcel de Rooy 2016-03-29 14:23:40 UTC
Created attachment 49634 [details] [review]
Bug 16155: [DO_NOT_PUSH] OldBehavior.t

This test just serves to illustrate the points mentioned in the commit
message of the second patch.

Test plan:
Run this test before applying patch 2.
See also commit message of the next patch.
Comment 2 Marcel de Rooy 2016-03-29 14:23:44 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-03-29 14:23:48 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2016-03-29 14:23:51 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-03-29 14:23:55 UTC Comment hidden (obsolete)
Comment 6 Bernardo Gonzalez Kriegel 2016-04-20 14:44:21 UTC
Testing...

No koha-qa errors

patch1) All tests pass

patch2) Test plan good, but not all test pass now
Following tests now fail

prove t/db_dependent/Creators/Lib.t
prove t/db_dependent/Koha/BiblioFrameworks.t
prove t/db_dependent/Members.t
prove t/db_dependent/Reports_Guided.t
prove t/db_dependent/Review.t

patch3) Test pass

patch4) Test fail
 prove t/db_dependent/TestBuilder.t
 ...
   #   Failed test 'TestBuilder should be able to create an object for every source'
    #   at t/db_dependent/TestBuilder.t line 71.
    #          got: '1'
    #     expected: '0'
    # The following sources have not been generated correctly: Deletedborrower
    # Looks like you failed 1 test of 1.
t/db_dependent/TestBuilder.t .. 3/9 
#   Failed test 'Build all sources'

patch5) All test pass
Comment 7 Marcel de Rooy 2016-04-20 14:59:58 UTC
Following tests now fail
prove t/db_dependent/Creators/Lib.t
prove t/db_dependent/Koha/BiblioFrameworks.t
prove t/db_dependent/Members.t
prove t/db_dependent/Reports_Guided.t
prove t/db_dependent/Review.t

$builder->clear( { source => 'Issue' } );
Might be a new call in Members.t

Other tests seem to pass with me. Will come back here tomorrow.
Comment 8 Marcel de Rooy 2016-04-21 07:26:56 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2016-04-21 07:27:02 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2016-04-21 07:27:07 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2016-04-21 07:27:11 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2016-04-21 07:27:16 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2016-04-21 07:34:57 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #6)
> patch2) Test plan good, but not all test pass now
> Following tests now fail
> 
> prove t/db_dependent/Creators/Lib.t
Passes with me. Please run the test again and report which tests failed.

> prove t/db_dependent/Koha/BiblioFrameworks.t
The last follow-up deals with that. This test had no transaction and does not need TestBuilder actually.

> prove t/db_dependent/Members.t
Replaced the clear statement. Should pass now with 5th patch.

> prove t/db_dependent/Reports_Guided.t
This test passes with me. Please run again and report which tests failed.

> prove t/db_dependent/Review.t
This test passes with me. Please run again and report which tests failed.

> patch4) Test fail
>  prove t/db_dependent/TestBuilder.t
>  ...
>    #   Failed test 'TestBuilder should be able to create an object for every
> source'
>     #   at t/db_dependent/TestBuilder.t line 71.
>     #          got: '1'
>     #     expected: '0'
>     # The following sources have not been generated correctly:
> Deletedborrower
This test passes with me.
Please run t/db_dependent/db_structure.t in order to check if your deletedborrowers table is in line with borrowers.

Finally, I added Barcodes.t to the 5th patch. Replaced a clear there.
Comment 14 Tomás Cohen Arazi 2016-04-21 13:54:57 UTC
Marcel and Bernardo, I ran the following in kohadevbox:ansible:

$ sudo koha-shell kohadev ; cd kohaclone
$ git grep -l "use t::lib::TestBuilder" | grep -v -e 'pm$' -e Old | xargs prove

The results are:

* Current master
=> SUCCESS: All tests pass in current master

* master + 16155
=> FAIL:

 - t/db_dependent/ILSDI_Services.t fails:
#   Failed test 'Test GetPatronInfo - show_attributes parameter'
#   at t/db_dependent/ILSDI_Services.t line 197.
#     Structures begin differing at:
#          $got = undef
#     $expected = ARRAY(0x8d3b150)
# Looks like you failed 1 test of 16.

 - I see some new warnings, which I assume are related to the original tests being buggy, so this is a good one for this patchset:

 t/db_dependent/ILSDI_Services.t
=> Violation of unique constraint in Branch
=> Null value for borrowernumber in BorrowerAttribute not allowed

 t/db_dependent/Holds.t
=> Violation of unique constraint in Category

 t/db_dependent/Accounts.t
=> Violation of unique constraint in Branch

That's what I found on a fresh environment (kohadevbox:ansible / jessie)

I fail it because of the failing tests. This patchset is not responsible for the warnings. Will try to provide a followup and retest.
Comment 15 Bernardo Gonzalez Kriegel 2016-04-21 14:18:52 UTC
Same result on standard clean install
Comment 16 Tomás Cohen Arazi 2016-04-21 14:27:31 UTC Comment hidden (obsolete)
Comment 17 Bernardo Gonzalez Kriegel 2016-04-21 14:50:56 UTC
Created attachment 50525 [details] [review]
[SIGNED-OFF] Bug 16155: Composite keys in TestBuilder and more

The number of tests using TestBuilder is gradually growing. Once you
are familiar with its use, you will appreciate it and find yourself
using it when writing new tests. Although it works quite well, some details
still needs some polishing.

While looking at the handling of composite keys in TestBuilder, a number
of related points came up too. This patch finally ended up in setting
the following design goals:

[1] TB should not only look at the first column of a composite FK.
[2] TB should optionally create records for fixed FK values.
[3] TB should create a new record, never change an existing record.
[4] TB should respect auto_increment columns.
[5] Passing a hash for a foreign key should always imply a new record.
[6] Explicitly passing undef for a column should mean NULL.
[7] Add a delete method; it will replace the clear method.
[8] Simplify by removing $default_values, hash key _fk and param only_fk.

The comments below further clarify these points. Note that they refer to
the old behavior (before this patch) unless stated otherwise.

Comments for point 1
====================
Look at:
    $builder->build({
        source => 'UserPermission',
        value => {
            borrowernumber => $borrowerno,
            module_bit => { flag => 'my flag' },
            code => 'will_be_ignored',
        },
    });
Module_bit and code here are a composite FK to permissions.
TB ignores the value for the code column here and creates a record with a
randomized code.

But if we would pass the hash in the second column of this compound key like:
            borrowernumber => $borrowerno,
            module_bit => 10,
            code => { code => 'new_code' },
TB would now crash when passing the hash for code thru to DBIx.

Comments for point 2
====================
Look at:
    $builder->build({
        source => 'UserPermission',
        value => {
            borrowernumber => $borrowerno,
            module_bit => 99,
            code => 'new_super_tool',
        },
    });
TB detects a fixed value for the module_bit, continues and will crash on
DBIx if the foreign keys are not found. In this case it would be friendly
to create a missing linked record.

Comments for point 3
====================
Look at:
    $builder->build({
        source => 'Branch',
        value  => { branchcode => 'CPL' },
    });
If this branch already exists, this call would modify the branch record and
overwrite all columns with randomized values (expected or not). In any case,
it would be safer here to return undef than modifying an existing record.

Comments for point 4
====================
Look at:
    $builder->build({
        source => 'Borrower',
        value  => { borrowernumber => '123456789' },
    });
If this number would exist, we would update (as earlier). But if this
number does not exist, we would create the record. Although that is
technically possible, I would prefer to have TB respect the auto
increment property of this column.

Comments for point 5
====================
Look at:
    $builder->build({
        source => 'Item',
        value  => { homebranch => { branchcode => 'MPL' } },
    });
As discussed under point 3, we should actually not pass primary key values
if we expect to build new records. The same also holds for the deeper
(recursive) calls to build when using hashes like here for homebranch.
In this case again an existing record might be overwritten.

Comments for point 6
====================
Look at:
    $builder->build({
        source => 'Reserve',
        value => { itemnumber => undef },
    });
As you know, a reserve without an itemnumber is a biblio level hold.
Unfortunately, TB did not care about passing undefs until now. So you would
get an item level hold.
In the new situation TB will respect these explicit undefs, as long as the
corresponding foreign key column is nullable of course.

Comments for point 7
====================
This patch will allow you to delete records created by TB:
    my $patron = $builder->build({ source => 'Borrower' });
    $builder->delete({ source => 'Borrower', records => $patron });
Or:
    $builder->delete({ source => 'Borrower', records => [ $patron, ... ] });
For safety, delete requires you to provide all primary key values in the
passed hashref(s).
Deleting all records in a table via clear is no longer supported and can
still be arranged in one statement.

Comments for point 8
====================
Current use of TestBuilder reveals that $default_values and only_fk
are not really needed. The current $default_values should imo not be in the
module anyway; if you want to use it, you could still pass it to TB:
    $builder->build({ ..., value => { %defa, %your_values } });

Only_fk stops at the very last step of saving the top level record while
storing all linked records at the lower levels. Practical use is not
very obvious; it can be easily simulated by one delete statement.

The hash key _fk is now used to store all linked records one or more levels
down. It is used in a few tests to retrieve a value one level down.
Why not retrieve that one value via the database and get rid of the
whole structure?

Implementation
==============
This highlights the main changes:

The $default_value hash (with some hardcoded values for UserPermission)
is removed from the module. It was used by a test in TestBuilder.t and has
been relocated.
The value of $gen_type is returned now by sub _gen_type. (See new.)

The main change in the build method is moving the foreign keys logic to a
new subroutine _create_links. This routine now looks at all columns of a
composite FK. It checks if a linked record exists for passed values, and
it looks at NULL values.

Routine _buildColumnValues is slightly adjusted to allow for passed undef
values. The theoretically endless loop is replaced by three tries. For
composite unique constraints we only check complete sets of values.

Routine _getForeignKeys contains a check to not return the same relation
twice in case of doubled belongs_to relations in the DBIx scheme.

The eval in _storeColumnValues is removed. The autoincrement check in sub
_buildColumnValue got more priority; the handling of foreign keys has been
adjusted and a check for not-nullable columns has been added.

TEST PLAN
=========
This patch only deals with the TestBuilder module itself. In the follow-up
patches TestBuilder.t and a few other unit tests are adjusted.

[1] Do not yet apply this patch. But apply the 'OldBehavior' patch.
    Verify that all tests pass. (They cover the first six design goals.)
[2] Apply this patch. Does TestBuilder still compile (perl -c)?
[3] Run the OldBehavior test again. Do all tests fail now? This means
    that we got rid of all unwanted side-effects in the list of goals.
[4] Run some other tests that use TestBuilder. (See below.)
    Skip the following tests; a follow-up patch deals with them.
    t/db_dependent/Accounts.t
    t/db_dependent/Barcodes.t
    t/db_dependent/Circulation/AnonymiseIssueHistory.t
    t/db_dependent/Circulation/CalcFine.t
    t/db_dependent/Holds.t
    t/db_dependent/Items/MoveItemFromBiblio.t
    t/db_dependent/Koha/BiblioFrameworks.t
    t/db_dependent/Members.t
    t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The following tests pass:
    t/db_dependent/Acquisition/OrderUsers.t
    t/db_dependent/Auth/haspermission.t
    t/db_dependent/Barcodes_ValueBuilder.t
    t/db_dependent/Budgets.t
    t/db_dependent/Category.t
    t/db_dependent/Circulation.t
    t/db_dependent/Circulation/GetTopIssues.t
    t/db_dependent/Circulation/IsItemIssued.t
    t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t
    t/db_dependent/Circulation/Returns.t
    t/db_dependent/Circulation/TooMany.t
    t/db_dependent/Circulation_dateexpiry.t
    t/db_dependent/Circulation_transfers.t
    t/db_dependent/CourseReserves.t
    t/db_dependent/Creators/Lib.t
    t/db_dependent/DecreaseLoanHighHolds.t
    t/db_dependent/Exporter/Record.t
    t/db_dependent/Holds/LocalHoldsPriority.t
    t/db_dependent/Holds/RevertWaitingStatus.t:
    t/db_dependent/HoldsQueue.t
    t/db_dependent/Holidays.t
    t/db_dependent/Items.t
    t/db_dependent/Items_DelItem.t
    t/db_dependent/Koha/Acquisition/Currencies.t
    t/db_dependent/Koha/Authorities.t
    t/db_dependent/Koha/BiblioFrameworks.t
    t/db_dependent/Koha/Cities.t
    t/db_dependent/Koha/Libraries.t
    t/db_dependent/Koha/Objects.t
    t/db_dependent/Koha/Patron/Categories.t
    t/db_dependent/Koha/Patron/Images.t
    t/db_dependent/Koha/Patron/Messages.t
    t/db_dependent/Koha/Patrons.t
    t/db_dependent/Koha/SMS_Providers.t
    t/db_dependent/Koha_template_plugin_Branches.t
    t/db_dependent/Letters.t
    t/db_dependent/Members/AddEnrolmentFeeIfNeeded.t
    t/db_dependent/Members/GetUpcomingMembershipExpires.t
    t/db_dependent/Members_Attributes.t
    t/db_dependent/Patron/Borrower_Debarments.t
    t/db_dependent/Patron/Borrower_Discharge.t
    t/db_dependent/Patron/Borrower_Files.t
    t/db_dependent/Ratings.t
    t/db_dependent/Reports_Guided.t
    t/db_dependent/Reserves/GetReserveFee.t
    t/db_dependent/Review.t
    t/db_dependent/Serials_2.t
    t/db_dependent/ShelfBrowser.t
    t/db_dependent/Virtualshelves.t
    t/db_dependent/api/v1/patrons.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 18 Bernardo Gonzalez Kriegel 2016-04-21 14:51:11 UTC
Created attachment 50526 [details] [review]
[SIGNED-OFF] Bug 16155: Remove a second use from Members_Attributes.t

Test plan:
Run t/db_dependent/Members_Attributes.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 19 Bernardo Gonzalez Kriegel 2016-04-21 14:51:21 UTC
Created attachment 50527 [details] [review]
[SIGNED-OFF] Bug 16155: Adjust TestBuilder.t

The changes in TestBuilder.pm require some changes in this test.

[1] Tests have been organized under subtests. A few superfluous tests have
    been removed. (There is still some overlap between the sections
    of overduerules_transport_type and userpermission.)
[2] The results in the build all sources-test are checked one step further.
[3] Tests are added for field length, null values and delete method.
[4] The former defaults from TestBuilder are incorporated in the tests
    for userpermission.

Test plan:
Run t/db_dependent/TestBuilder.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 20 Bernardo Gonzalez Kriegel 2016-04-21 14:51:32 UTC
Created attachment 50528 [details] [review]
[SIGNED-OFF] Bug 16155: Adjust a few other tests

Accounts.t: Only added a line that ensures the MPL branch exists.
AnonymiseIssueHistory.t: Only add a branch to work with.
Barcodes.t: Replaced clear with delete_all.
CalcFine.t: Remove default issuing rule and add one instead of updating.
Holds.t: Add category S in case it would not exist.
Members.t: Replaced clear with delete_all.
MoveItemFromBiblio.t: Replace last _fk construction.

Test plan:
Run these tests. (See note).
Git grep for only_fk, {_fk} and TestBuilder::default_value.

Note: Holds.t does not pass. Tests 9 and 39 fail, but they did already.
not ok 9 - GetReservesFromItemnumber should return a valid borrowernumber
not ok 39 - Test AlterPriority(), move to bottom
So this test needs attention, but on another report please :)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 21 Bernardo Gonzalez Kriegel 2016-04-21 14:51:42 UTC
Created attachment 50529 [details] [review]
[SIGNED-OFF] Bug 16155: [QA Follow-up] Add transaction to BiblioFrameworks.t

This unit test does not have a transaction.
It does not need TestBuilder.

Test plan:
[1] Optionally remove records with mfw1, mfw2 from biblio_framework table.
    If you ran this test before and it failed, you may have them.
[2] Run the test.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 22 Bernardo Gonzalez Kriegel 2016-04-21 14:51:52 UTC
Created attachment 50530 [details] [review]
[SIGNED-OFF] Bug 16155: (QA followup) fix small bug in t/db_dependent/ILSDI_Services.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
With all patches applied all test pass,
all == git grep -l "use t::lib::TestBuilder" | grep -v -e 'pm$' -e Old | xargs prove

No errors
Comment 23 Marcel de Rooy 2016-04-21 16:27:37 UTC
Thx Bernardo and Tomas
Comment 24 Marcel de Rooy 2016-04-28 07:22:58 UTC
Still applies.
git grep -l "use t::lib::TestBuilder" | grep -v -e 'pm$' -e Old | xargs prove
reports three failing tests:

t/db_dependent/Holds.t
  Failed tests:  9, 39
NOTE: This fails also without this patch set.

t/db_dependent/ILSDI_Services.t
  Failed test:  16
RESOLVED via bug 16320

t/db_dependent/Members/Attributes.t
  Failed tests:  19, 23, 36, 56
Caused by bug 12267, RESOLVED via bug 16377

So, who will pass QA on this dev now? Tomas, you marked yourself as QA contact, did you lose your QA token by signing one of the patches?
Comment 25 Tomás Cohen Arazi 2016-04-28 13:08:07 UTC
Changing the order of the factors does not change the product.
Comment 26 Kyle M Hall 2016-05-04 13:51:29 UTC
Pushed to master for Koha 16.05, thanks Marcel!
Comment 27 Jonathan Druart 2016-05-04 14:56:54 UTC
*** Bug 16331 has been marked as a duplicate of this bug. ***