Bug 19821 - Run tests on a separate database
Summary: Run tests on a separate database
Status: CLOSED FIXED
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: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 19185
  Show dependency treegraph
 
Reported: 2017-12-15 14:17 UTC by Julian Maurice
Modified: 2022-06-06 20:24 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00


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
Bug 19821: Run tests on a separate database (4.23 KB, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Close filehandle after writing (1.10 KB, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Install sample data, ES mappings and Version syspref (2.07 KB, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t (702 bytes, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead (3.58 KB, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Use database_test entry if exists (2.16 KB, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Require database_test != database (1.97 KB, patch)
2020-02-21 20:21 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19821: Run tests on a separate database (4.55 KB, patch)
2020-04-21 19:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Close filehandle after writing (1.16 KB, patch)
2020-04-21 19:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Install sample data, ES mappings and Version syspref (2.15 KB, patch)
2020-04-21 19:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t (767 bytes, patch)
2020-04-21 19:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead (3.64 KB, patch)
2020-04-21 19:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Use database_test entry if exists (2.23 KB, patch)
2020-04-21 19:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Require database_test != database (2.03 KB, patch)
2020-04-21 19:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Run tests on a separate database (4.62 KB, patch)
2020-06-15 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Close filehandle after writing (1.23 KB, patch)
2020-06-15 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Install sample data, ES mappings and Version syspref (2.22 KB, patch)
2020-06-15 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t (839 bytes, patch)
2020-06-15 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead (3.71 KB, patch)
2020-06-15 12:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Use database_test entry if exists (2.30 KB, patch)
2020-06-15 12:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Require database_test != database (2.10 KB, patch)
2020-06-15 12:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19821: Run tests on a separate database (4.61 KB, patch)
2021-05-03 22:26 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Close filehandle after writing (1.21 KB, patch)
2021-05-03 22:26 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Install sample data, ES mappings and Version syspref (2.20 KB, patch)
2021-05-03 22:26 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t (823 bytes, patch)
2021-05-03 22:27 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead (3.69 KB, patch)
2021-05-03 22:27 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Use database_test entry if exists (2.29 KB, patch)
2021-05-03 22:27 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 19821: Require database_test != database (2.09 KB, patch)
2021-05-03 22:27 UTC, Victor Grousset/tuxayo
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.
Comment 37 Marcel de Rooy 2019-09-13 07:51:43 UTC
QA: Spending some time here now
Comment 38 Marcel de Rooy 2019-09-13 09:47:07 UTC
I am not sure if we should have coupled the option of a test database and the selenium test environment on this report.
The two config options database and database_test do not seem a good idea to me, intuitively. It looks more like a hack? (Also taken in consideration that git grep shows me that the database config variable is used in more places.)

On another report we already had a discussion about the rollback of changes for selenium tests. I think we'd better separate the test suite from the selenium tests. And actually, we have already the tools to create separate instances for selenium and testing. With a separate KOHA_CONF in the webserver configuration selenium can be run on its own. Same for t/db_dependent. Only thing is that we want to do it faster. But does that justify a bunch of code changes that might jeopardize code quality?

This needs (imo) more serious thought and a broader discussion.
Comment 39 Marcel de Rooy 2019-09-13 09:48:56 UTC
And for less changes what about mocking C4::Context->config for the database entry?
Comment 40 Jonathan Druart 2019-10-15 09:16:10 UTC
Hi Marcel,

Thanks for your interest :)

(In reply to Marcel de Rooy from comment #38)
> I am not sure if we should have coupled the option of a test database and
> the selenium test environment on this report.

I am not sure I understand your concern, one this report we are not talking about selenium tests. On bug 19185 we are going to use this enhancement to test the onboarding and installation process.

> The two config options database and database_test do not seem a good idea to
> me, intuitively. It looks more like a hack? (Also taken in consideration
> that git grep shows me that the database config variable is used in more
> places.)

It does not sound bad for me. We specify in our config the name of the DB for tests. We could have hardcoded here in the code, but seemed better to make it configurable.
What did you git grep exactly? To get the dbh you must call C4::Context->dbh, nothing else.

> On another report we already had a discussion about the rollback of changes
> for selenium tests. I think we'd better separate the test suite from the
> selenium tests. And actually, we have already the tools to create separate
> instances for selenium and testing. With a separate KOHA_CONF in the
> webserver configuration selenium can be run on its own. Same for
> t/db_dependent. Only thing is that we want to do it faster. But does that
> justify a bunch of code changes that might jeopardize code quality?

We could improve it later. So far it the safest and easiest way to test the onboarding and installation process. It seems a fundamental to back the RM before they roll a new release. Keep in mind that the original idea is to make the onboarding and installation process run for all languages.

> This needs (imo) more serious thought and a broader discussion.

IIRC the discussion has been launched on koha-devel about 2 years ago ;)

(In reply to Marcel de Rooy from comment #39)
> And for less changes what about mocking C4::Context->config for the database
> entry?

That was the original idea, see "Bug 19821: Do not modify KOHA_CONF, use new database_test entry instead".
Comment 41 Bouzid Fergani 2020-02-21 20:21:38 UTC
Created attachment 99435 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 42 Bouzid Fergani 2020-02-21 20:21:41 UTC
Created attachment 99436 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 43 Bouzid Fergani 2020-02-21 20:21:45 UTC
Created attachment 99437 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 44 Bouzid Fergani 2020-02-21 20:21:48 UTC
Created attachment 99438 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 45 Bouzid Fergani 2020-02-21 20:21:51 UTC
Created attachment 99439 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 46 Bouzid Fergani 2020-02-21 20:21:54 UTC
Created attachment 99440 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 47 Bouzid Fergani 2020-02-21 20:21:58 UTC
Created attachment 99441 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 48 Victor Grousset/tuxayo 2020-04-18 07:43:52 UTC
From what I understand, to test this, the only thing needed is the test plan in comment 41

Is that correct? I'm not sure if there is something to do with comment 43.

> 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))

Which is the "test file"? The one picked in step 3? Then one must chose one that use transactions.
Comment 49 Jonathan Druart 2020-04-20 09:24:51 UTC
(In reply to Victor Grousset/tuxayo from comment #48)
> From what I understand, to test this, the only thing needed is the test plan
> in comment 41
> 
> Is that correct? I'm not sure if there is something to do with comment 43.

See also bug 19185 comment 23.

> > 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))
> 
> Which is the "test file"? The one picked in step 3? Then one must chose one
> that use transactions.

Whichever you want that is in t/db_dependent
Comment 50 Victor Grousset/tuxayo 2020-04-21 03:29:59 UTC
Thanks for the explanations.

The DB is not being created, here is what I did (on koha_testing_docker)

Patches applied

reset_all

> root@13b368ace1d6:/# mysql -u root --password=password                

[...]

> MariaDB [(none)]> GRANT ALL PRIVILEGES ON koha_test.* TO koha_kohadev;

> kohadev-koha@76c874795df6:/kohadevbox/koha$ prove t/db_dependent/Holds.t 

> MariaDB [(none)]> show databases;

No koha_test
Comment 51 Jonathan Druart 2020-04-21 08:46:44 UTC
(In reply to Victor Grousset/tuxayo from comment #50)
> No koha_test

The DB is created then dropped. Try with a test file that takes several seconds to finish(selenium tests for instance), then try the "show databases" while the tests are running.
Comment 52 Victor Grousset/tuxayo 2020-04-21 17:45:20 UTC
It didn't happen during the following.
prove t/db_dependent/selenium/basic_workflow.t

The following confirmed that the test was done in the main DB.
select count(*) from koha_kohadev.borrowers;

note that I only did what I wrote in comment 50

Do I need to apply the patches of bug 19185 and the patches of this bug at the same time? And then I run the test plan of comment 41 and then the test plan of bug 19185 comment 23?
Comment 53 Victor Grousset/tuxayo 2020-04-21 17:56:52 UTC
comment 33

> 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.

Okay I get it now. I'm trying this.
Comment 54 Victor Grousset/tuxayo 2020-04-21 19:53:55 UTC
Created attachment 103409 [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. Apply bug 19185 which will also apply these patches
2. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
     (adapt if your usual koha DB isn't koha_kohadev)
3. Run the test plan of bug 19185 and during execution of the
   installation test, pay attention to the following:
4. List the databases in the DBMS (show databases;) to ensure that
   koha_test is created
5. After the test has run, the koha_test database should not be here anymore.
6. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
7. That was to verify that the database you use usually was untouched.
   The counts should be the same.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 55 Victor Grousset/tuxayo 2020-04-21 19:54:00 UTC
Created attachment 103410 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 56 Victor Grousset/tuxayo 2020-04-21 19:54:04 UTC
Created attachment 103411 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 57 Victor Grousset/tuxayo 2020-04-21 19:54:08 UTC
Created attachment 103412 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 58 Victor Grousset/tuxayo 2020-04-21 19:54:13 UTC
Created attachment 103413 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 59 Victor Grousset/tuxayo 2020-04-21 19:54:17 UTC
Created attachment 103414 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 60 Victor Grousset/tuxayo 2020-04-21 19:54:21 UTC
Created attachment 103415 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 61 Victor Grousset/tuxayo 2020-04-21 20:02:54 UTC
Signed off, I did nothing more than what's the test plan in the patch so I hope I didn't missed something when I rewrote it.

The only thing that I'm not confident about is «verify that the database you use usually was untouched»

Before there was in the test plan:

> (you can edit the test file and comment the lines related to a transaction (txn_begin, txn_rollback))

There are none in selenium/00-installation.t

Is counting the users in the main DB before and after the install tests enough?
Or checking that the user "SuperL" created during the test isn't in the main DB?


== Diff of the test plan ==
=== Before ===
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))


=== After ===
Test plan:
1. Apply bug 19185 which will also apply these patches
2. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
     (adapt if your usual koha DB isn't koha_kohadev)
3. Run the test plan of bug 19185 and during execution of the
   installation test, pay attention to the following:
4. List the databases in the DBMS (show databases;) to ensure that
   koha_test is created
5. After the test has run, the koha_test database should not be here anymore.
6. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
7. That was to verify that the database you use usually was untouched.
   The counts should be the same.
Comment 62 Kyle M Hall 2020-06-03 11:05:08 UTC
Jonathan, can we get a unified test plan for this patch set? I'm not sure if I'm testing wrong. Selenium tests pass in 'normal' mode, so I know it's not my Selenium container that's not problematic:

root@16935ef8a671:koha(bug19821-qa)$ prove -Mt::lib::Bootstrap t/db_dependent/selenium/00-installation.t
t/db_dependent/selenium/00-installation.t .. Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/t/lib/Selenium.pm line 87.
Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/t/lib/Selenium.pm line 87.

STRACE: /usr/share/perl5/Try/Tiny.pm:124 in Selenium::Remote::Driver::catch {...} 
        /usr/local/share/perl/5.24.1/Selenium/Remote/Driver.pm:353 in Try::Tiny::try
        (eval 340):1 in Selenium::Remote::Driver::__ANON__
        (eval 342):2 in Selenium::Remote::Driver::__ANON__
        /usr/local/share/perl/5.24.1/Selenium/Remote/Driver.pm:937 in Selenium::Remote::Driver::_execute_command
        t/db_dependent/selenium/00-installation.t:54 in Selenium::Remote::Driver::get


SCREENSHOT: https://framapic.org/g4mzi23sJZJh/qs6oGBNksp4u
Error while executing command: get: An unknown server-side error occurred while processing the command.: Target URL /cgi-bin/koha/mainpage.pl is not well-formed.
Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
System info: host: '61269cb7f179', ip: '172.18.0.4', os.name: 'Linux', os.arch: 'amd64', os.version: '4.19.76-linuxkit', java.version: '1.8.0_91'
Driver info: driver.version: unknown at /usr/local/share/perl/5.24.1/Selenium/Remote/Driver.pm line 404.
 at /usr/local/share/perl/5.24.1/Selenium/Remote/Driver.pm line 348.
# Looks like your test exited with 255 before it could output anything.
t/db_dependent/selenium/00-installation.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/2 subtests 

Test Summary Report
-------------------
t/db_dependent/selenium/00-installation.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 2 tests but ran 0.
Files=1, Tests=0,  5 wallclock secs ( 0.02 usr  0.00 sys +  1.10 cusr  0.20 csys =  1.32 CPU)
Result: FAIL
Comment 63 Jonathan Druart 2020-06-05 10:32:26 UTC
(In reply to Kyle M Hall from comment #62)
> Jonathan, can we get a unified test plan for this patch set? I'm not sure if
> I'm testing wrong. Selenium tests pass in 'normal' mode, so I know it's not
> my Selenium container that's not problematic:

Hi Kyle,

The test plan provided by Victor is quite good.

If you want to "just it passes" you can:

1.
$ mysql -h db -u root --password=password
> GRANT ALL PRIVILEGES ON `koha_test`.* TO 'koha_kohadev';

2.
# Edit $KOHA_CONF and add 
 <database_test>koha_test</database_test>

3.
$ prove -Mt::lib::Bootstrap t/db_dependent/selenium/00-installation.t

And the tests should pass. But the different steps described by Victor are useful to understand how it works.
Comment 64 Jonathan Druart 2020-06-05 10:33:23 UTC
ha, don't forget to apply patches from both bug 19821 and bug 19185. They go together.
Comment 65 Kyle M Hall 2020-06-12 12:43:53 UTC
(In reply to Jonathan Druart from comment #63)
> (In reply to Kyle M Hall from comment #62)
> > Jonathan, can we get a unified test plan for this patch set? I'm not sure if
> > I'm testing wrong. Selenium tests pass in 'normal' mode, so I know it's not
> > my Selenium container that's not problematic:
> 
> Hi Kyle,
> 
> The test plan provided by Victor is quite good.
> 
> If you want to "just it passes" you can:
> 
> 1.
> $ mysql -h db -u root --password=password
> > GRANT ALL PRIVILEGES ON `koha_test`.* TO 'koha_kohadev';
> 
> 2.
> # Edit $KOHA_CONF and add 
>  <database_test>koha_test</database_test>
> 
> 3.
> $ prove -Mt::lib::Bootstrap t/db_dependent/selenium/00-installation.t
> 
> And the tests should pass. But the different steps described by Victor are
> useful to understand how it works.

$ prove -Mt::lib::Bootstrap t/db_dependent/selenium/00-installation.t
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 '(0x562c034c75a0)' at line 1 at /kohadevbox/koha/t/lib/Bootstrap.pm line 30.
DBIx::Class::Storage::DBI::catch {...} (): DBI Connection failed: DBI connect('database=ARRAY(0x562c034c75a0);host=db;port=3306','koha_kohadev',...) failed: Access denied for user 'koha_kohadev'@'%' to database 'ARRAY(0x562c034c75a0)' at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1517. at /kohadevbox/koha/Koha/Database.pm line 116
END failed--call queue aborted.
Comment 66 Kyle M Hall 2020-06-15 12:39:34 UTC
Created attachment 105876 [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. Apply bug 19185 which will also apply these patches
2. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
     (adapt if your usual koha DB isn't koha_kohadev)
3. Run the test plan of bug 19185 and during execution of the
   installation test, pay attention to the following:
4. List the databases in the DBMS (show databases;) to ensure that
   koha_test is created
5. After the test has run, the koha_test database should not be here anymore.
6. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
7. That was to verify that the database you use usually was untouched.
   The counts should be the same.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 67 Kyle M Hall 2020-06-15 12:39:47 UTC
Created attachment 105877 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Kyle M Hall 2020-06-15 12:39:53 UTC
Created attachment 105878 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Kyle M Hall 2020-06-15 12:39:57 UTC
Created attachment 105879 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Kyle M Hall 2020-06-15 12:40:01 UTC
Created attachment 105880 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Kyle M Hall 2020-06-15 12:40:05 UTC
Created attachment 105881 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall 2020-06-15 12:40:09 UTC
Created attachment 105882 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Victor Grousset/tuxayo 2021-05-03 22:26:47 UTC
Created attachment 120417 [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. Apply bug 19185 which will also apply these patches
2. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
     (adapt if your usual koha DB isn't koha_kohadev)
3. Run the test plan of bug 19185 and during execution of the
   installation test, pay attention to the following:
4. List the databases in the DBMS (show databases;) to ensure that
   koha_test is created
5. After the test has run, the koha_test database should not be here anymore.
6. In the DBMS run `select count(*) from koha_kohadev.borrowers;`
7. That was to verify that the database you use usually was untouched.
   The counts should be the same.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Victor Grousset/tuxayo 2021-05-03 22:26:53 UTC
Created attachment 120418 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Victor Grousset/tuxayo 2021-05-03 22:26:58 UTC
Created attachment 120419 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Victor Grousset/tuxayo 2021-05-03 22:27:04 UTC
Created attachment 120420 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 77 Victor Grousset/tuxayo 2021-05-03 22:27:11 UTC
Created attachment 120421 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 78 Victor Grousset/tuxayo 2021-05-03 22:27:18 UTC
Created attachment 120422 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 79 Victor Grousset/tuxayo 2021-05-03 22:27:24 UTC
Created attachment 120423 [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>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 80 Victor Grousset/tuxayo 2021-05-03 22:32:23 UTC
Still works, solved a simple conflict for ".proverc and .proverc.dist in Makefile.t"

*But* I missed the test plan of comment 75 in my first signoff and I don't know how to make it work.
How it that supposed to be ran?

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

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove --rc .proverc.dist
t/00-checkdatabase-version.t ................ ok   
t/00-deprecated.t ........................... ok   
t/00-load.t ................................. DBD::mysql::st execute failed: Table 'koha_test.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='casauthentication'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_test.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='caslogout'] at /kohadevbox/koha/Koha/Database.pm line 124.


And more errors about the DB.
Comment 81 Victor Grousset/tuxayo 2021-05-04 21:08:22 UTC
Don't forget to remove the .proverc from the root of the code. It will make prove fail with
Can't load module t::lib::Bootstrap at /usr/bin/prove line 13.
(when not having this patch applied anymore)
Comment 82 Jonathan Druart 2021-10-25 09:44:37 UTC
Pushed to master for 21.11, thanks to everybody involved!