Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Bug 17885
Created attachment 58942 [details] [review] Bug 17897 - Koha::SearchMappingManager - Simplify search mapping modifications Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Buug 17885
Test plan is to run the elasticsearch-dependent tests in t/db_dependent/Koha*
Created attachment 65392 [details] [review] Bug 17897 - (follow-up) Cleanup after tests Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t To test: 1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
fatal: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
Created attachment 102475 [details] [review] Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Rebased patch. Added 'search' to 'get_search_mappings' and adjusted tests to work with changed mappings.
Created attachment 102476 [details] [review] Bug 17897: (follow-up) Cleanup after tests
git bz apply 17897 Bug 17897 - Koha::SearchMappingManager - Simplify search mapping modifications 102475 - Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications 102476 - Bug 17897: (follow-up) Cleanup after tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Using index info to reconstruct a base tree... M Koha/SearchEngine/Elasticsearch.pm Falling back to patching base and 3-way merge... Auto-merging Koha/SearchEngine/Elasticsearch.pm CONFLICT (content): Merge conflict in Koha/SearchEngine/Elasticsearch.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-17897-KohaSearchMappingManager---Simplify-sear-JiTslW.patch patch does not apply
Created attachment 115320 [details] [review] Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Buug 17885 Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 115321 [details] [review] Bug 17897: (follow-up) Cleanup after tests Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t To test: 1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 115322 [details] [review] Bug 17897: Fix failing tests Tests failed when txn_begin and rollback were used inside of subtests. To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Patch still applies, but tests fail: prove -vvv t/db_dependent/Koha/SearchEngine/ElasticSearch.t t/db_dependent/Koha/SearchEngine/ElasticSearch.t .. # Subtest: Reset Elasticsearch mappings ok 1 - Scenario: Reset Elasticsearch mappings to an empty database ok 2 - Given empty search mapping tables ok 3 - When reset_elasticsearch_mappings() has been ran ok 4 - Then search mapping tables have been populated ok 5 - Scenario: Reset Elasticsearch mappings when custom mappings already exist 1..5 ok 1 - Reset Elasticsearch mappings DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Objects.pm line 114 # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 11 just after 1. Dubious, test returned 11 (wstat 2816, 0xb00) All 1 subtests passed Test Summary Report ------------------- t/db_dependent/Koha/SearchEngine/ElasticSearch.t (Wstat: 2816 Tests: 1 Failed: 0) Non-zero exit status: 11 Parse errors: No plan found in TAP output Files=1, Tests=1, 4 wallclock secs ( 0.03 usr 0.00 sys + 3.53 cusr 0.19 csys = 3.75 CPU) Result: FAIL
Created attachment 143744 [details] [review] Bug 17897: Fix failing tests Tests failed when txn_begin and rollback were used inside of subtests. To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Fixed tests, patches should apply cleanly.
Tests are still failing for me: prove -vvv t/db_dependent/Koha/SearchEngine/ElasticSearch.t t/db_dependent/Koha/SearchEngine/ElasticSearch.t .. # Subtest: Reset Elasticsearch mappings ok 1 - Scenario: Reset Elasticsearch mappings to an empty database ok 2 - Given empty search mapping tables ok 3 - When reset_elasticsearch_mappings() has been ran ok 4 - Then search mapping tables have been populated ok 5 - Scenario: Reset Elasticsearch mappings when custom mappings already exist 1..5 ok 1 - Reset Elasticsearch mappings DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Objects.pm line 114 # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 11 just after 1. Dubious, test returned 11 (wstat 2816, 0xb00) All 1 subtests passed Test Summary Report ------------------- t/db_dependent/Koha/SearchEngine/ElasticSearch.t (Wstat: 2816 Tests: 1 Failed: 0) Non-zero exit status: 11 Parse errors: No plan found in TAP output Files=1, Tests=1, 4 wallclock secs ( 0.02 usr 0.00 sys + 3.65 cusr 0.15 csys = 3.82 CPU) Result: FAIL
I just realized tests also lack "tests", "plan tests" etc. and subtests aren't declared inside sub. Also, should test file be named something else? It's now ElasticSearch.t but we also have Elasticsearch.t.
(In reply to David Nind from comment #14) > Tests are still failing for me: > > prove -vvv t/db_dependent/Koha/SearchEngine/ElasticSearch.t > t/db_dependent/Koha/SearchEngine/ElasticSearch.t .. > # Subtest: Reset Elasticsearch mappings > ok 1 - Scenario: Reset Elasticsearch mappings to an empty database > ok 2 - Given empty search mapping tables > ok 3 - When reset_elasticsearch_mappings() has been ran > ok 4 - Then search mapping tables have been populated > ok 5 - Scenario: Reset Elasticsearch mappings when custom mappings > already exist > 1..5 > ok 1 - Reset Elasticsearch mappings > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Data truncated for column 'type' at row 1 at > /kohadevbox/koha/Koha/Objects.pm line 114 > # Tests were run but no plan was declared and done_testing() was not seen. > # Looks like your test exited with 11 just after 1. > Dubious, test returned 11 (wstat 2816, 0xb00) > All 1 subtests passed > > Test Summary Report > ------------------- > t/db_dependent/Koha/SearchEngine/ElasticSearch.t (Wstat: 2816 Tests: 1 > Failed: 0) > Non-zero exit status: 11 > Parse errors: No plan found in TAP output > Files=1, Tests=1, 4 wallclock secs ( 0.02 usr 0.00 sys + 3.65 cusr 0.15 > csys = 3.82 CPU) > Result: FAIL Tables search_field column type only accepts following values: '','string','date','number','boolean','sum','isbn','stdno','year','callnumber'. Tests use value "keyword".
Created attachment 157224 [details] [review] Bug 17897: Fix failing tests and QA issues Tests failed because 1) txn_begin and rollback were used inside of subtests. 2) column search_field.type was filled with unallowed value. This patch also fixes QA issue with test file exec rights and incorrect license statement. To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Fixed tests and QA issues qa-tool found. Didn't change test files name yet.
Created attachment 157282 [details] [review] Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Buug 17885 Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157283 [details] [review] Bug 17897: (follow-up) Cleanup after tests Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t To test: 1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157284 [details] [review] Bug 17897: Fix failing tests and QA issues Tests failed because 1) txn_begin and rollback were used inside of subtests. 2) column search_field.type was filled with unallowed value. This patch also fixes QA issue with test file exec rights and incorrect license statement. To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Signed-off-by: David Nind <david@davidnind.com>
Please rebase!
Created attachment 158487 [details] [review] Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Buug 17885 Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 158488 [details] [review] Bug 17897: (follow-up) Cleanup after tests Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t To test: 1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 158489 [details] [review] Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Buug 17885 Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158490 [details] [review] Bug 17897: (follow-up) Cleanup after tests Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t To test: 1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158491 [details] [review] Bug 17897: Fix failing tests and QA issues Tests failed because 1) txn_begin and rollback were used inside of subtests. 2) column search_field.type was filled with unallowed value. This patch also fixes QA issue with test file exec rights and incorrect license statement. To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Signed-off-by: David Nind <david@davidnind.com>
Made some mess and accidentally rebased patches without Davids sign off first. Also these are missing "filter" param, will add it in a moment.
Created attachment 158492 [details] [review] Bug 17897: Add search_marc_to_field.filter to SearhMappingManager Methods in SearchMappingManager didn't take into account search_marc_to_field.filter indtroduced in bug 27153. To test, prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Sponsored-by: Koha-Suomi Oy
Created attachment 158561 [details] [review] Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing. Includes tests for Buug 17885 Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158562 [details] [review] Bug 17897: (follow-up) Cleanup after tests Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t To test: 1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158563 [details] [review] Bug 17897: Fix failing tests and QA issues Tests failed because 1) txn_begin and rollback were used inside of subtests. 2) column search_field.type was filled with unallowed value. This patch also fixes QA issue with test file exec rights and incorrect license statement. To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158564 [details] [review] Bug 17897: Add search_marc_to_field.filter to SearhMappingManager Methods in SearchMappingManager didn't take into account search_marc_to_field.filter indtroduced in bug 27153. To test, prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t Sponsored-by: Koha-Suomi Oy Signed-off-by: David Nind <david@davidnind.com>
WARN Koha/SearchMappingManager.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 11) WARN t/db_dependent/Koha/SearchEngine/ElasticSearch.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 76) +@THROWS die when parameters are not properly given. Should throw a Koha::Exception::BadParameter, + but pushing all those Exceptions to the community version would take ages. Hmm. Just adding that exception does not take ages. One die ? diff --git a/t/db_dependent/Koha/SearchEngine/ElasticSearch.t b/t/db_dependent/Koha/SearchEngine/ElasticSearch.t new file mode 100644 OK Fixed later +# Copyright 2015 Catalyst IT Fix? +use Modern::Perl qw(2014); Not what we do elsewhere? + $rv = Koha::SearchMappingManager::flush(); No transactions, no rollbacks ? OK Added later. General remark / question Given this comment: "Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing." Koha/SearchEngine/Elasticsearch.pm | 27 +--- Koha/SearchMappingManager.pm | 151 ++++++++++++++++++ .../Koha/SearchEngine/ElasticSearch.t | 127 +++++++++++++++ 3 files changed, 280 insertions(+), 25 deletions(-) I am not seeing large number of replaces to start using this new class? So could you explain the benefit of adding those 250 lines please?