Bug 18286 - Test::DBIx::Class connection/schema is shadowed by a cached connection/schema
Summary: Test::DBIx::Class connection/schema is shadowed by a cached connection/schema
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: Olli-Antti Kivilahti
QA Contact: Galen Charlton
URL:
Keywords:
Depends on: 18285
Blocks: 18226
  Show dependency treegraph
 
Reported: 2017-03-16 19:07 UTC by Olli-Antti Kivilahti
Modified: 2018-06-04 20:10 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 18286 - Test::DBIx::Class connection/schema is shadowed by a cached connection/schema (4.57 KB, patch)
2017-03-16 19:09 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18286: Test::DBIx::Class connection/schema is shadowed by a cached connection/schema (4.66 KB, patch)
2017-05-02 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18286: [Follow-up] Add t/etc/schema.pl (1.00 KB, patch)
2017-05-02 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18286: [QA Follow-up] Fix some warnings from qa tools (1.95 KB, patch)
2017-05-02 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18286: [Follow-up] Remove assumption on branch count (1.91 KB, patch)
2017-05-02 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18286: Test::DBIx::Class connection/schema is shadowed by a cached connection/schema (4.70 KB, patch)
2017-06-05 20:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18286: [Follow-up] Add t/etc/schema.pl (1.04 KB, patch)
2017-06-05 20:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18286: [QA Follow-up] Fix some warnings from qa tools (1.99 KB, patch)
2017-06-05 20:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18286: [Follow-up] Remove assumption on branch count (1.95 KB, patch)
2017-06-05 20:53 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-16 19:07:07 UTC
If Koha::Database->schema gets called before

    use Test::DBIx::Class

The DB connection from $KOHA_CONF is cached.

This happens most of the time because when C4::Context and friends are loaded (in compile-time?), they already access the DB.

After Test::DBIx::Class is instantiated and hooks put in place to overload Koha::Schema connection, those hooks are never called due to getting the old connection from cache.

This feature introduces a test case to replicate the behaviour and shows how flushing the connection cache solves the problem.
Comment 1 Olli-Antti Kivilahti 2017-03-16 19:09:16 UTC
Created attachment 61186 [details] [review]
Bug 18286 - Test::DBIx::Class connection/schema is shadowed by a cached connection/schema

If Koha::Database->schema gets called before

    use Test::DBIx::Class

The DB connection from $KOHA_CONF is cached.

This happens most of the time because when C4::Context and friends are loaded
(in compile-time?), they already access the DB.

After Test::DBIx::Class is instantiated and hooks put in place to overload
Koha::Schema connection, those hooks are never called due to getting the old
connection from cache.

This feature introduces a test case to replicate the behaviour and shows how
flushing the connection cache solves the problem.
Comment 2 Olli-Antti Kivilahti 2017-03-16 19:11:03 UTC
Prior to this patch, for example t/Koha.t fails due to this caching issue.
Comment 3 Josef Moravec 2017-03-23 10:27:56 UTC
comment from qa tools:

 FAIL	t/db_dependent/01-test_dbic.t
   FAIL	  critic
		Code before strictures are enabled at line 4, column 1. See page 429 of PBP.
		"$schema" is declared but not used at line 18, column 3. Unused variables clutter code and make it harder to read.
		Expression form of "eval" at line 77, column 3. See page 161 of PBP.
Comment 4 Marcel de Rooy 2017-05-02 13:41:32 UTC
Do we need a t/etc/schema file here?
Comment 5 Marcel de Rooy 2017-05-02 14:42:28 UTC
Created attachment 62963 [details] [review]
Bug 18286: Test::DBIx::Class connection/schema is shadowed by a cached connection/schema

If Koha::Database->schema gets called before

    use Test::DBIx::Class

The DB connection from $KOHA_CONF is cached.

This happens most of the time because when C4::Context and friends are loaded
(in compile-time?), they already access the DB.

After Test::DBIx::Class is instantiated and hooks put in place to overload
Koha::Schema connection, those hooks are never called due to getting the old
connection from cache.

This feature introduces a test case to replicate the behaviour and shows how
flushing the connection cache solves the problem.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-05-02 14:42:35 UTC
Created attachment 62964 [details] [review]
Bug 18286: [Follow-up] Add t/etc/schema.pl

We need this file here already.
Copied from bug 18226 and adjusted in order to keep qa tools quiet.

Test plan:
Run t/db_dependent/01-test_dbic.t again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-05-02 14:42:41 UTC
Created attachment 62965 [details] [review]
Bug 18286: [QA Follow-up] Fix some warnings from qa tools

[1] Resolve warning on variable assignment before use strict.
[2] Remove unused $schema variable.
[3] Remove eval expression construction.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-05-02 14:42:45 UTC
Created attachment 62966 [details] [review]
Bug 18286: [Follow-up] Remove assumption on branch count

A test in db_dependent should not make assumptions on the number of
branches in the database. If you need one, create one. Removing the
assumption of a non-zero count.

Removing the library count statement outside the subtest.
Replacing C4::Context by Koha::Database.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Jonathan Druart 2017-06-05 20:53:33 UTC
Created attachment 64004 [details] [review]
Bug 18286: Test::DBIx::Class connection/schema is shadowed by a cached connection/schema

If Koha::Database->schema gets called before

    use Test::DBIx::Class

The DB connection from $KOHA_CONF is cached.

This happens most of the time because when C4::Context and friends are loaded
(in compile-time?), they already access the DB.

After Test::DBIx::Class is instantiated and hooks put in place to overload
Koha::Schema connection, those hooks are never called due to getting the old
connection from cache.

This feature introduces a test case to replicate the behaviour and shows how
flushing the connection cache solves the problem.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2017-06-05 20:53:40 UTC
Created attachment 64005 [details] [review]
Bug 18286: [Follow-up] Add t/etc/schema.pl

We need this file here already.
Copied from bug 18226 and adjusted in order to keep qa tools quiet.

Test plan:
Run t/db_dependent/01-test_dbic.t again.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2017-06-05 20:53:45 UTC
Created attachment 64006 [details] [review]
Bug 18286: [QA Follow-up] Fix some warnings from qa tools

[1] Resolve warning on variable assignment before use strict.
[2] Remove unused $schema variable.
[3] Remove eval expression construction.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2017-06-05 20:53:51 UTC
Created attachment 64007 [details] [review]
Bug 18286: [Follow-up] Remove assumption on branch count

A test in db_dependent should not make assumptions on the number of
branches in the database. If you need one, create one. Removing the
assumption of a non-zero count.

Removing the library count statement outside the subtest.
Replacing C4::Context by Koha::Database.

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

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