Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication
Summary: Remove "use Test::DBIx::Class" instantiations' dangerous code duplication
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Keywords:
Depends on: 14778 18089 18285 18286
Blocks: 18287 18288 18289 18851
  Show dependency treegraph
 
Reported: 2017-03-07 16:55 UTC by Olli-Antti Kivilahti
Modified: 2018-06-04 20:10 UTC (History)
6 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 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication (8.08 KB, patch)
2017-03-07 16:58 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1 (7.13 KB, patch)
2017-03-07 17:22 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2 (4.06 KB, patch)
2017-03-08 15:35 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication (8.34 KB, patch)
2017-03-08 16:07 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1 (7.17 KB, patch)
2017-05-02 14:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2 (4.09 KB, patch)
2017-05-02 14:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication (7.89 KB, patch)
2017-05-02 14:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1 (7.23 KB, patch)
2017-05-02 14:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2 (4.15 KB, patch)
2017-05-02 14:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication (7.95 KB, patch)
2017-05-02 14:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226: [QA Follow-up] Remove further assumptions on branch count (1.85 KB, patch)
2017-05-24 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226: [QA Follow-up] Remove verbose (1.49 KB, patch)
2017-05-24 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18226: Perltidy + license (6.80 KB, patch)
2017-06-05 20:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1 (7.32 KB, patch)
2017-06-05 20:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2 (4.20 KB, patch)
2017-06-05 20:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication (8.06 KB, patch)
2017-06-05 20:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18226: [QA Follow-up] Remove further assumptions on branch count (1.89 KB, patch)
2017-06-05 20:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18226: [QA Follow-up] Remove verbose (1.53 KB, patch)
2017-06-05 20:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18226: Perltidy + license (6.87 KB, patch)
2017-06-05 20:55 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2017-03-07 16:55:34 UTC
Everywhere where Test::DBIx::Class is used, a following blob of code follows:

use Test::DBIx::Class {
    schema_class => 'Koha::Schema',
    connect_info => ['dbi:SQLite:dbname=:memory:','',''],
    connect_opts => { name_sep => '.', quote_char => '`', },
    fixture_class => '::Populate',
}, 'SocialData', 'Biblioitem' ;


Instead of duplicating this same code everywhere, use a default configuration for a Test::DBIx::Class in t/etc/schema.pl
Comment 1 Olli-Antti Kivilahti 2017-03-07 16:58:39 UTC
Created attachment 60885 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication

Is 'instantiations' even a word?

Use a Test::DBIx::Class defaults instead.

Save your keyboard and prevent horrible bugs from emerging from rampant code duplication.

This change doesn't seem to have any impact on the speed of executing those tests.
Comment 2 Olli-Antti Kivilahti 2017-03-07 17:22:11 UTC
Created attachment 60886 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1

t/00-load.t already checks if all of the perl modules can be compiled.

The tests deleted in this commit do a duplicate test with t/00-load.t
Hence they have become unnecessary.
Comment 3 Olli-Antti Kivilahti 2017-03-07 17:28:02 UTC
t/db_dependent/Circulation/CalcDateDue.t fails with date calculus. SQLite is peculiar with date calculus.
Comment 4 Olli-Antti Kivilahti 2017-03-08 15:35:19 UTC
Created attachment 60920 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2

Couldn't make the tests pass using Test::DBIx::Class, so reverted to the "usual way" since these tests are
in db_dependent anyway.
Comment 5 Olli-Antti Kivilahti 2017-03-08 16:07:39 UTC
Created attachment 60923 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication

Is 'instantiations' even a word?

Use a Test::DBIx::Class defaults instead.

Save your keyboard and prevent horrible bugs from emerging from rampant code duplication.

This change doesn't seem to have any impact on the speed of executing those tests.
Comment 6 Olli-Antti Kivilahti 2017-03-16 19:30:55 UTC
See the blocked list of Bugs for more individual test fixes.
Comment 7 Jonathan Druart 2017-03-16 19:59:25 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2017-03-16 20:00:13 UTC

% git log --oneline

619aa5f Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2
5fb9461 Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1
5875738 Bug 18286 - Test::DBIx::Class connection/schema is shadowed by a cached connection/schema
8814c10 Bug 18285 - Koha::Database schema cache accessors

Last patch from this bug does not apply!

% prove t/Members/cardnumber.
t/Members/cardnumber.t .. # Can't initialize a schema with the given configuration
# Returned Error: `' is not a module name
# configuration: {
#   'fixture_sets' => {}
# }
...

% pmvers Test::DBIx::Class
0.52
Comment 9 Josef Moravec 2017-03-23 10:30:44 UTC
does not apply on top of 18285 and 18286
Comment 10 Marcel de Rooy 2017-05-02 14:24:51 UTC
Applying: Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication
fatal: sha1 information is lacking or useless (t/Calendar.t).
Comment 11 Marcel de Rooy 2017-05-02 14:41:30 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2017-05-02 14:41:37 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-05-02 14:41:41 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2017-05-02 14:44:58 UTC
Created attachment 62968 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1

t/00-load.t already checks if all of the perl modules can be compiled.

The tests deleted in this commit do a duplicate test with t/00-load.t
Hence they have become unnecessary.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2017-05-02 14:45:03 UTC
Created attachment 62969 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2

Couldn't make the tests pass using Test::DBIx::Class, so reverted to the "usual way" since these tests are
in db_dependent anyway.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2017-05-02 14:45:07 UTC
Created attachment 62970 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication

Is 'instantiations' even a word?

Use a Test::DBIx::Class defaults instead.

Save your keyboard and prevent horrible bugs from emerging from rampant code duplication.

This change doesn't seem to have any impact on the speed of executing those tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-05-02 14:46:33 UTC
Moved the t/etc/schema to an earlier report.
Comment 18 Jonathan Druart 2017-05-23 20:05:18 UTC
The comparaison
  $libCount = Koha::Libraries->search->count
does not work when there is no libraries defined (0 vs undef)

prove t/db_dependent/01-test_dbic.t
t/db_dependent/01-test_dbic.t .. 
    #   Failed test '  When the libraries are counted'
    #   at t/db_dependent/01-test_dbic.t line 41.
    # Looks like you failed 1 test of 19.
t/db_dependent/01-test_dbic.t .. 1/? 
#   Failed test 'Scenario: Show how caching prevents Test::DBIx::Class from working properly and how to circumvent it'
#   at t/db_dependent/01-test_dbic.t line 95.
# Looks like you failed 1 test of 1.
t/db_dependent/01-test_dbic.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
t/db_dependent/01-test_dbic.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  3 wallclock secs ( 0.02 usr  0.02 sys +  2.09 cusr  0.88 csys =  3.01 CPU)
Result: FAIL

You should also use is instead of ok and follow the coding guidelines of the project (camel_case for instance), please...

$verbose should be removed
Comment 19 Marcel de Rooy 2017-05-24 12:05:53 UTC
(In reply to Jonathan Druart from comment #18)
> The comparaison
>   $libCount = Koha::Libraries->search->count
> does not work when there is no libraries defined (0 vs undef)

Hmm. I think it is an assignment, not an comparison. Both 0 and undef will be interpreted as false by ok. (Note that I am not the author.)
Comment 20 Marcel de Rooy 2017-05-24 12:12:08 UTC
Created attachment 63688 [details] [review]
Bug 18226: [QA Follow-up] Remove further assumptions on branch count

See also follow-up on bug 18286.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2017-05-24 12:18:17 UTC
Created attachment 63689 [details] [review]
Bug 18226: [QA Follow-up] Remove verbose

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2017-05-24 12:21:24 UTC
(In reply to Jonathan Druart from comment #18)
> prove t/db_dependent/01-test_dbic.t
Please try again with the follow-ups.

> You should also use is instead of ok and follow the coding guidelines of the
> project (camel_case for instance), please...
Not sure what you exactly want here. The lines where ok now is called, are fine with me. I'd rather see ok than using 'is' for comparing with false.
Please specify which names should be adjusted.

> $verbose should be removed
Fixed
Comment 23 Marc Véron 2017-05-31 06:34:06 UTC
Wanted to apply for testing Bug 18287 on top of this bug, but fails with:

 Bug 18226: [QA Follow-up] Remove further assumptions on branch count
fatal: sha1 information is lacking or useless (t/db_dependent/01-test_dbic.t).
Comment 24 Marcel de Rooy 2017-05-31 11:04:31 UTC
(In reply to Marc Véron from comment #23)
> Wanted to apply for testing Bug 18287 on top of this bug, but fails with:
> 
>  Bug 18226: [QA Follow-up] Remove further assumptions on branch count
> fatal: sha1 information is lacking or useless
> (t/db_dependent/01-test_dbic.t).

Applies fine with me, no conflicts or so.
Did you apply the dependencies? 18285 and 18286
Comment 25 Jonathan Druart 2017-06-05 20:35:12 UTC
Created attachment 63999 [details] [review]
Bug 18226: Perltidy + license
Comment 26 Jonathan Druart 2017-06-05 20:54:44 UTC
Created attachment 64008 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 1

t/00-load.t already checks if all of the perl modules can be compiled.

The tests deleted in this commit do a duplicate test with t/00-load.t
Hence they have become unnecessary.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2017-06-05 20:54:50 UTC
Created attachment 64009 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication. Replace mock_dbh 2

Couldn't make the tests pass using Test::DBIx::Class, so reverted to the "usual way" since these tests are
in db_dependent anyway.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2017-06-05 20:54:56 UTC
Created attachment 64010 [details] [review]
Bug 18226 - Remove "use Test::DBIx::Class" instantiations' dangerous code duplication

Is 'instantiations' even a word?

Use a Test::DBIx::Class defaults instead.

Save your keyboard and prevent horrible bugs from emerging from rampant code duplication.

This change doesn't seem to have any impact on the speed of executing those tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2017-06-05 20:55:03 UTC
Created attachment 64011 [details] [review]
Bug 18226: [QA Follow-up] Remove further assumptions on branch count

See also follow-up on bug 18286.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2017-06-05 20:55:11 UTC
Created attachment 64012 [details] [review]
Bug 18226: [QA Follow-up] Remove verbose

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2017-06-05 20:55:18 UTC
Created attachment 64013 [details] [review]
Bug 18226: Perltidy + license

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2017-06-05 20:56:39 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 33 Fridolin Somers 2017-06-13 07:28:42 UTC
Enhancement not pushed to 17.05.x