Bug 19821 - Run tests on a separate database
Summary: Run tests on a separate database
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 19185
  Show dependency treegraph
 
Reported: 2017-12-15 14:17 UTC by Julian Maurice
Modified: 2019-02-14 13:29 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19821: Run tests on a separate database (4.07 KB, patch)
2017-12-15 14:35 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19821: Close filehandle after writing (955 bytes, patch)
2017-12-18 08:10 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19821: Run tests on a separate database (4.12 KB, patch)
2017-12-18 08:27 UTC, Jon Knight
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19821: Close filehandle after writing (1012 bytes, patch)
2017-12-18 08:27 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19821: Install sample data, ES mappings and Version syspref (1.97 KB, patch)
2018-02-16 15:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t (585 bytes, patch)
2018-05-04 05:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead (3.47 KB, patch)
2018-05-04 17:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19821: Use database_test entry if exists (2.05 KB, patch)
2018-05-04 17:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19821: Require database_test != database (1.85 KB, patch)
2018-11-02 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19821: Run tests on a separate database (4.18 KB, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19821: Close filehandle after writing (1.05 KB, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19821: Install sample data, ES mappings and Version syspref (2.03 KB, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t (650 bytes, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead (3.53 KB, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19821: Use database_test entry if exists (2.12 KB, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19821: Require database_test != database (1.92 KB, patch)
2018-11-02 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2017-12-15 14:17:57 UTC
Running the tests on the same database as the one used for dev has some drawbacks:
- Everybody has different data, so we cannot make any assumptions about data in tests and it can make tests fail for non-obvious reasons.
- Tests have to clean up every change to the database using SQL transactions, so we cannot write testable code that use transactions (AFAIK)
- Transactions in tests happen to be committed sometimes, resulting in garbage data added to the dev database

I'll send a patch that allows to run tests in a separate database in order to address these issues.
Comment 1 Julian Maurice 2017-12-15 14:35:59 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2017-12-15 14:44:57 UTC
Note: this patch is a proof of concept and its main purpose is to start a discussion (should we run tests in a separate database, should we provide a .proverc, ...?)

Please test and discuss!
Comment 3 Jon Knight 2017-12-17 20:39:00 UTC
I've tried the patch on my kohadevbox VM and unfortunately I get this output:

vagrant@kohadevbox:kohaclone(bug_19821)$ prove t/db_dependent/Suggestions.t 
Error reading file /tmp/koha-conf.wyAjLQ at /home/vagrant/kohaclone/Koha/Config.pm line 51.
Error reading file /tmp/koha-conf.wyAjLQ at /home/vagrant/kohaclone/Koha/Config.pm line 51.
Error reading file /tmp/koha-conf.wyAjLQ at /home/vagrant/kohaclone/Koha/Config.pm line 51.
The config file (/tmp/koha-conf.wyAjLQ) has not been parsed correctly at /home/vagrant/kohaclone/C4/Context.pm line 262.
Error reading file /tmp/koha-conf.wyAjLQ at /home/vagrant/kohaclone/Koha/Config.pm line 51.
The config file (/tmp/koha-conf.wyAjLQ) has not been parsed correctly at /home/vagrant/kohaclone/C4/Context.pm line 262.
Error reading file /tmp/koha-conf.wyAjLQ at /home/vagrant/kohaclone/Koha/Config.pm line 51.
The config file (/tmp/koha-conf.wyAjLQ) has not been parsed correctly at /home/vagrant/kohaclone/C4/Context.pm line 262.
Create test database...
Use of uninitialized value $host in concatenation (.) or string at /home/vagrant/kohaclone/t/lib/Bootstrap.pm line 50.
Use of uninitialized value $port in concatenation (.) or string at /home/vagrant/kohaclone/t/lib/Bootstrap.pm line 50.
Use of uninitialized value $database in concatenation (.) or string at /home/vagrant/kohaclone/t/lib/Bootstrap.pm line 55.
DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 at /home/vagrant/kohaclone/t/lib/Bootstrap.pm line 55.

I do have the grant applied:

GRANT ALL PRIVILEGES ON `koha_test`.* TO 'koha_kohadev'@'%'

Without the patch, the test runs OK.
Comment 4 Julian Maurice 2017-12-18 08:10:30 UTC
Created attachment 69842 [details] [review]
Bug 19821: Close filehandle after writing
Comment 5 Julian Maurice 2017-12-18 08:12:23 UTC
Hi Jon, thanks for testing.

I cannot reproduce the same problem as you but I noticed that the temporary file is not always written to before it's used. The last patch should fix that.
If that doesn't work for you, could you remove the 'UNLINK => 1' param from tempfile call and inspect the content of the created temporary file after prove ?
Comment 6 Jon Knight 2017-12-18 08:27:41 UTC
Created attachment 69843 [details] [review]
[SIGNED-OFF] Bug 19821: Run tests on a separate database

Running the tests on the same database as the one used for dev has some
drawbacks:
- Everybody has different data, so we cannot make any assumptions about
data in tests and it can make tests fail for non-obvious reasons.
- Tests have to clean up every change to the database using SQL
transactions, so we cannot write testable code that use transactions
(AFAIK)
- Transactions in tests happen to be committed sometimes, resulting in
garbage data added to the dev database

This patch provides a .proverc file that will load t/lib/Bootstrap.pm
before the tests are run.
t/lib/Bootstrap.pm is responsible for recreating a fresh database and
telling the test scripts to use it.
To use it, just run prove normally from the root directory of Koha.
By default, the database is 'koha_test' and it's created using MARC21
SQL files, it can be changed by running:

    prove --norc \
    --Mt::lib::Bootstrap=database,koha_test,marcflavour,UNIMARC

Test plan:
1. mysql> GRANT ALL PRIVILEGES ON koha_test.* TO
          <your-db-user>@localhost
2. cd /path/to/koha
3. prove t/db_dependent/<pick-your-favorite-test-here>.t
4. Verify that it created the database koha_test and that the one you
   use usually was untouched (you can edit the test file and comment the
   lines related to a transaction (txn_begin, txn_rollback))

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 7 Jon Knight 2017-12-18 08:27:44 UTC
Created attachment 69844 [details] [review]
[SIGNED-OFF] Bug 19821: Close filehandle after writing

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 8 Marcel de Rooy 2017-12-18 10:32:19 UTC
Interesting.
I'm just thinking if we should not resolve the garbage data problem instead of working around it with another database. 
I would rather not default to another database.
If you create an additional clone with db and koha-conf, you can just run another export KOHA_CONF and run a test on the other db?
Comment 9 Julian Maurice 2017-12-18 11:12:49 UTC
(In reply to Marcel de Rooy from comment #8)
> Interesting.
> I'm just thinking if we should not resolve the garbage data problem instead
> of working around it with another database.
Sure we could, but
1) I think it cannot be fixed once and for all (patch writer could forget to setup the DB transaction in tests, when you will test the patch it will alter your database)
2) It doesn't fix the other problems (non obvious fails caused by different data, non testable "transactions-using" code)

> I would rather not default to another database.
Can you elaborate on why you prefer to keep only one database ?

> If you create an additional clone with db and koha-conf, you can just run
> another export KOHA_CONF and run a test on the other db?
Right, but
1) It's error-prone (I know I will forget to change KOHA_CONF at some point)
2) If I'm the only one doing that, it only fixes my "garbage data" problem. 

However if everyone starts using the same database for tests, it might ease writing and running tests. It also make testing with another marcflavour very easy (maybe some day all tests will pass in UNIMARC... :))
Comment 10 Julian Maurice 2018-02-16 15:19:38 UTC
Created attachment 71817 [details] [review]
Bug 19821: Install sample data, ES mappings and Version syspref

Also move .proverc to .proverc.dist, so it cannot be used by accident.

Different ways to use it:

1/ prove --rc .proverc.dist
   Use defaults

2/ cp .proverc.dist .proverc && prove
   Allows to configure db name, marcflavour, and prove options
Comment 11 Jonathan Druart 2018-05-03 21:10:04 UTC
#   Failed test 'All directories should be mapped: .proverc.dist'
#   at t/Makefile.t line 47.
#          got: '1'
#     expected: '0'
Comment 12 Julian Maurice 2018-05-04 05:45:25 UTC
Created attachment 75045 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t
Comment 13 Jonathan Druart 2018-05-04 17:54:38 UTC
Created attachment 75091 [details] [review]
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead

I have tried to use the original idea of this patch for bug 19185, but
it cannot work as it. ENV is not shared between processus and KOHA_CONF
will not be read by webserver (used for selenium tests).

Moreover on bug 19185 we are testing the installation process and we do
not want the database to be populated with sample data, we just need a
new and clean DB.
Comment 14 Jonathan Druart 2018-05-04 17:54:58 UTC
Created attachment 75092 [details] [review]
Bug 19821: Use database_test entry if exists

If the database_test entry exists in the config file we are going to use
it. This will be a trick for developers and CI
Comment 15 Jonathan Druart 2018-05-04 17:57:27 UTC
Julian,
I am more or less stealing this bug report for bug 19185. I am willing to move/rewrite these 2 patches somewhere else if you think I lost too much your initial idea.
The goal here is more to have a first working thing pushed, and then see later how we can re-use/modify it.
Comment 16 Martin Renvoize 2018-08-03 21:48:48 UTC
I like this and it all looks reasonably sane.. I'd like to test it against a few full test suit runs before signing it off though.. one for Monday!
Comment 17 Martin Renvoize 2018-11-02 13:48:46 UTC
Can we add a test for existance of database_test before running the DROP DATABASE commands
Comment 18 Jonathan Druart 2018-11-02 14:00:41 UTC
Created attachment 81893 [details] [review]
Bug 19821: Require database_test != database

To prevent developpers to drop their database.
Comment 19 Martin Renvoize 2018-11-02 14:36:09 UTC
Created attachment 81894 [details] [review]
Bug 19821: Run tests on a separate database

Running the tests on the same database as the one used for dev has some
drawbacks:
- Everybody has different data, so we cannot make any assumptions about
data in tests and it can make tests fail for non-obvious reasons.
- Tests have to clean up every change to the database using SQL
transactions, so we cannot write testable code that use transactions
(AFAIK)
- Transactions in tests happen to be committed sometimes, resulting in
garbage data added to the dev database

This patch provides a .proverc file that will load t/lib/Bootstrap.pm
before the tests are run.
t/lib/Bootstrap.pm is responsible for recreating a fresh database and
telling the test scripts to use it.
To use it, just run prove normally from the root directory of Koha.
By default, the database is 'koha_test' and it's created using MARC21
SQL files, it can be changed by running:

    prove --norc \
    --Mt::lib::Bootstrap=database,koha_test,marcflavour,UNIMARC

Test plan:
1. mysql> GRANT ALL PRIVILEGES ON koha_test.* TO
          <your-db-user>@localhost
2. cd /path/to/koha
3. prove t/db_dependent/<pick-your-favorite-test-here>.t
4. Verify that it created the database koha_test and that the one you
   use usually was untouched (you can edit the test file and comment the
   lines related to a transaction (txn_begin, txn_rollback))

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2018-11-02 14:36:13 UTC
Created attachment 81895 [details] [review]
Bug 19821: Close filehandle after writing

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2018-11-02 14:36:16 UTC
Created attachment 81896 [details] [review]
Bug 19821: Install sample data, ES mappings and Version syspref

Also move .proverc to .proverc.dist, so it cannot be used by accident.

Different ways to use it:

1/ prove --rc .proverc.dist
   Use defaults

2/ cp .proverc.dist .proverc && prove
   Allows to configure db name, marcflavour, and prove options

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2018-11-02 14:36:20 UTC
Created attachment 81897 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2018-11-02 14:36:24 UTC
Created attachment 81898 [details] [review]
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead

I have tried to use the original idea of this patch for bug 19185, but
it cannot work as it. ENV is not shared between processus and KOHA_CONF
will not be read by webserver (used for selenium tests).

Moreover on bug 19185 we are testing the installation process and we do
not want the database to be populated with sample data, we just need a
new and clean DB.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2018-11-02 14:36:28 UTC
Created attachment 81899 [details] [review]
Bug 19821: Use database_test entry if exists

If the database_test entry exists in the config file we are going to use
it. This will be a trick for developers and CI

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2018-11-02 14:36:32 UTC
Created attachment 81900 [details] [review]
Bug 19821: Require database_test != database

To prevent developpers to drop their database.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2018-11-02 14:37:54 UTC
As an initial step for Jenkins and the use of Selenium testing on the installer, this patch works.

Signing Off
Comment 27 Josef Moravec 2019-02-14 13:24:50 UTC
So, let's start QA process:

1) Pass the database name from proverc file is not possible, but still in .proverc.dist as example/default...

2) No tables are created in Bootstrap.pm, the structure creation was removed in 
"Do not modify KOHA_CONF, use new database_test entry instead" patch
Comment 28 Julian Maurice 2019-02-14 13:29:04 UTC
Setting joubu as assignee because of comment 15