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

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


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
Comment 29 Jonathan Druart 2019-02-17 13:48:05 UTC
(In reply to Josef Moravec from comment #27)
> 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...

Yes, maybe we need to remove proverc in this iteration, and readd it later. So far no args is used from Bootstrap.

> 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

Yes it is:
 30     $dbh->do("DROP DATABASE IF EXISTS $database_test");
 31     $dbh->do("CREATE DATABASE $database_test");


I cannot retest right now, my selenium tests do not run (local issue).

TODO: remove proverc and update the test plan.
Test plan:
-  prove t/db_dependent/selenium/authentication.t # "database" from conf will be used
-  prove -Mt::lib::Bootstrap t/db_dependent/selenium/authentication.t # will die because "database_test" does not exist
- Add the database_test entry to your koha-conf, use the same value as database
-  prove -Mt::lib::Bootstrap t/db_dependent/selenium/authentication.t # die with "same values"
- Modify database_test to be different than database
-  prove -Mt::lib::Bootstrap t/db_dependent/selenium/authentication.t # Will drop, create and use "database_test" to run the tests.
Comment 30 Josef Moravec 2019-02-18 09:52:25 UTC
(In reply to Jonathan Druart from comment #29)
> -  prove -Mt::lib::Bootstrap t/db_dependent/selenium/authentication.t # Will
> drop, create and use "database_test" to run the tests.

it will drop, create and try to use "database_test", but this db is empty and the test then fail...
Comment 31 Jonathan Druart 2019-02-19 12:48:07 UTC
(In reply to Josef Moravec from comment #30)
> (In reply to Jonathan Druart from comment #29)
> > -  prove -Mt::lib::Bootstrap t/db_dependent/selenium/authentication.t # Will
> > drop, create and use "database_test" to run the tests.
> 
> it will drop, create and try to use "database_test", but this db is empty
> and the test then fail...

Sorry, wrong file, must be tested along with bug 19185 (t/db_dependent/selenium/00-installation.t)
Comment 32 Katrin Fischer 2019-04-08 11:51:28 UTC
(In reply to Jonathan Druart from comment #31)
> (In reply to Josef Moravec from comment #30)
> > (In reply to Jonathan Druart from comment #29)
> > > -  prove -Mt::lib::Bootstrap t/db_dependent/selenium/authentication.t # Will
> > > drop, create and use "database_test" to run the tests.
> > 
> > it will drop, create and try to use "database_test", but this db is empty
> > and the test then fail...
> 
> Sorry, wrong file, must be tested along with bug 19185
> (t/db_dependent/selenium/00-installation.t)

Should bug 19185 be a dependency here?
Comment 33 Martin Renvoize 2019-04-08 16:07:12 UTC
I had it the other way around.. that kind of depend upon each other though I suppose.. they need testing in conjunction with each other.
Comment 34 Katrin Fischer 2019-04-08 16:56:14 UTC
Now I am as confused as before :)
Comment 35 Katrin Fischer 2019-05-11 19:31:11 UTC
Could I get an updated test plan here?

Is it still required to grant the permissions on the db? How do we plan on handling that in kohadevbox etc.?
Comment 36 Jonathan Druart 2019-05-12 17:31:14 UTC
(In reply to Katrin Fischer from comment #35)
> Could I get an updated test plan here?

See bug 19185 comment 23.

> Is it still required to grant the permissions on the db?

Yes of course, we need to drop and create the DB.

> How do we plan on handling that in kohadevbox etc.?

We need to know if the way we handle database_test is correct (for instance if it exists, it will be used, even in production). And then need to document it (also add more doc in the code ($KOHA_CONF and Koha::Database).

It's more jenkins that will need to be adjust, to run 00-installation.t first and with -Mt::lib::Bootstrap

When we will reach that goal, we will still be far from what Julian wanted at the beginning: all tests run on a separate DB.
But at least we will have the installation and onboarding process tested.

Also missing here (bug 19185): test the installation for all languages.

So yes, still a lot of work to do. But until we do not have the bases pushed, we are all stuck.