Description
Marcel de Rooy
2016-03-29 12:57:01 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. Created attachment 49635 [details] [review] 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/Circulation/AnonymiseIssueHistory.t t/db_dependent/Circulation/CalcFine.t t/db_dependent/Holds.t t/db_dependent/Items/MoveItemFromBiblio.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.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 Created attachment 49636 [details] [review] Bug 16155: Remove a second use from Members_Attributes.t Test plan: Run t/db_dependent/Members_Attributes.t Created attachment 49637 [details] [review] 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 Created attachment 49638 [details] [review] 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 CalcFine.t: Remove default issuing rule and add one instead of updating. Holds.t: Add category S in case it would not exist. 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 :) 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 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. Created attachment 50490 [details] [review] 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 Created attachment 50491 [details] [review] Bug 16155: Remove a second use from Members_Attributes.t Test plan: Run t/db_dependent/Members_Attributes.t Created attachment 50492 [details] [review] 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 Created attachment 50493 [details] [review] 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 :) Created attachment 50494 [details] [review] 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. (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. 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. Same result on standard clean install Created attachment 50518 [details] [review] Bug 16155: (QA followup) fix small bug in t/db_dependent/ILSDI_Services.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> 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> 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> 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 Thx Bernardo and Tomas 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? Changing the order of the factors does not change the product. Pushed to master for Koha 16.05, thanks Marcel! *** Bug 16331 has been marked as a duplicate of this bug. *** |