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!

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.

Created attachment 69842 [details] [review]
Bug 19821: Close filehandle after writing

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 ?

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>

Created attachment 69844 [details] [review]
[SIGNED-OFF] Bug 19821: Close filehandle after writing

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

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?

(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... :))

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

#   Failed test 'All directories should be mapped: .proverc.dist'
#   at t/Makefile.t line 47.
#          got: '1'
#     expected: '0'

Created attachment 75045 [details] [review]
Bug 19821: Ignore .proverc and .proverc.dist in Makefile.t

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.

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

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.

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!

Can we add a test for existance of database_test before running the DROP DATABASE commands

Created attachment 81893 [details] [review]
Bug 19821: Require database_test != database

To prevent developpers to drop their database.

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>

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>

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>

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>

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>

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>

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>

As an initial step for Jenkins and the use of Selenium testing on the installer, this patch works.

Signing Off

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

Setting joubu as assignee because of comment 15

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

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

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

(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?

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.

Now I am as confused as before :)

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

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

QA: Spending some time here now

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.

And for less changes what about mocking C4::Context->config for the database entry?

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

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>

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>

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>

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>

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>

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>

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>

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.

(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

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

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

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

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>

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>

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>

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>

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>

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>

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>

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.

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

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

ha, don't forget to apply patches from both bug 19821 and bug 19185. They go together.

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

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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.

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)

Pushed to master for 21.11, thanks to everybody involved!