to reproduce remove the section [database_tests]
from your local tests. It will then result in a database access exception.
That's because in https://github.com/matomo-org/matomo/blob/4.4.1/tests/PHPUnit/bootstrap.php#L126 it uses a mock config which has "allowSave = false".
It must have regressed at some point but not sure where.
The goals are:
database_tests
section is not configured.config/config.ini.php
or is this maybe now expected?
Not sure what the expected behavior should be. When automatically storing the config it might have the risk that it's the same as the normal db config. There is no need to define a table prefix when installing Matomo, so tests could overwrite the normal database.
We could maybe have some new command like .\console tests:setup
, that ask for various parameters and adjusts the config accordingly. For UI tests there might also http_host
and request_uri
be needed.
./console tests:setup
seems like a good idea. Could it automatically be triggered when the config is missing and ask:
Do you want to configure a test DB now? (Y/n)
I'm not sure what the point of a command is which in the end then also just does it maybe "automatically". Then this logic could be executed directly when bootstrapping.
We could check that the test prefix is different to the regular database (which it is in > 80% of the cases since "matomo_" prefix is the default so it will work most of the time).
It should update the regular config/config.ini.php
when no test DB section is configured, which it used to do AFAIK.
Basically, the goal is that the user does not need to configure this section.
The logic for the UI tests should also configure additionally the config/config.ini.php
if that's not the case yet https://github.com/matomo-org/matomo/blob/4.5.0-b1/tests/PHPUnit/bootstrap.php#L85-L94
@tsteur, i agree. It should check the prefixes are the same as the regular DB parameter and iff they are the same throw an error. Otherwise it should just go ahead and save that config.
Should this mean that we put the logic here:
https://github.com/matomo-org/matomo/blob/4.5.0-b1/tests/PHPUnit/bootstrap.php#L85-L94
and pass in true here:
for the $allowSave
parameter?
Hard to say, I'm not too much into this topic. I only know this used to work but must have regressed at some point. The goal will be that it configures this automatically saves the regular config/config.ini.php
if it's not configured yet. If the section is already configured, then we don't want to change anything. Not sure if allowSave
is false
for a reason maybe. And we need to make sure if we save there, that it doesn't overwrite any other setting in config/config.ini.php
. It should only overwrite this particular setting there. If the test bootstrap process sets maybe other configs, these should not be written.
Might be easier to create a new config instance there just in case something else would change the config by accident. I haven't looked into it too much though.
@tsteur Just confirm the logic here.
when the [database_tests]
is not set, it will create a new database with the _test
suffix through the same connection that already exists in the [database]
section
If the connection failed because of a password (like username not set on %) or [datababse] dbname is the same as [database_test] about to generate, we throw an error message, require the user to do a manual config. Otherwise, just continue PHP tests.
It's that sounds about right?
Another approach is use sqlite driver, store a sqlite file into /tmp/matomo_test.sqlite. If not specify setup, always use sqlite.
when the [database_tests] is not set, it will create a new database with the _test suffix through the same connection that already exists in the [database] section
It shouldn't create a new database. We would expect it to configure the same database details as in [database]
with the only difference that the prefix will be set to an empty string. Unless in [database]
the table prefix is already an empty string, in that case we would probably throw an exception and get users to manually configure a different [database_tests]
.
If the connection failed because of a password (like username not set on %) or [datababse] dbname is the same as [database_test] about to generate, we throw an error message, require the user to do a manual config. Otherwise, just continue PHP tests.
Yes, in that case we would throw an exception 👍
Our queries actually aren't compatible with sqlite unfortunately.
@tsteur not sure I am correct when I fix the database_test issue, if I put them into the same database when I run PHP tests like ./console tests:setup-fixture OmniFixture
or any system tests. it seems like there is a function called trying to drop the entire database.
https://github.com/matomo-org/matomo/blob/ddfa42e16f6648a1f54d2912fcc897e0ae07b75e/tests/PHPUnit/Framework/Fixture.php#L251-L255
It'll be the same database but the tables_prefix
should be different. AFAIK by default Matomo uses a table prefix matomo_
meaning if you set an empty prefix in the tests, they shouldn't overwrite each other. However, if the user has configured no table prefix (meaning an empty table prefix) in [database]
then we can't configure this automatically and we would throw an exception.
I checked again though and by default we indeed use matomo_tests
database. Meaning all we need to configure there is
[database_tests]
host = "the host from [database]"
username = "the username from [database]"
password = "the password from [database]"
If no [database_tests]username
is not configured yet
This is what prepareTestDatabaseConfig
already does. The logic is already there. It's just not modifying the correct config.
You can temporarily remove the [database_tests]
section from your local config/config.ini.php
. Then try to run an integration test and it will fail like this:
Expected outcome would be that the tests run without that error.