@tsteur opened this Issue on August 4th 2021 Member

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:

  • [ ] Not throw an exception when database_tests section is not configured.
  • [ ] Automatically pre-fill the database_tests section in the config/config.ini.php

image
image

or is this maybe now expected?

@sgiehl commented on August 4th 2021 Member

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.

@geekdenz commented on September 16th 2021 Contributor

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

@tsteur commented on September 16th 2021 Member

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

  • If the empty test prefix is different to the regular DB prefix, then we can configure the tests section automatically.
  • If test section is not configured and the regular DB prefix is empty, we could throw an exception when running the tests and let the user know to configure the test DB manually. That would then likely mean the user needs to configure a different DB name for the tests as it means the user already is using an empty prefix and the tests need to run with an empty DB prefix or the user could reinstall Matomo with a DB prefix by deleting the config file but then all data gets lost.

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

@geekdenz commented on September 20th 2021 Contributor

@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:

https://github.com/matomo-org/matomo/blob/ddfa42e16f6648a1f54d2912fcc897e0ae07b75e/config/environment/test.php#L56

for the $allowSave parameter?

@tsteur commented on September 20th 2021 Member

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.

@peterhashair commented on October 11th 2021 Contributor

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

@tsteur commented on October 12th 2021 Member

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.

@peterhashair commented on October 12th 2021 Contributor

@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

@tsteur commented on October 12th 2021 Member

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
image

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:
image

Expected outcome would be that the tests run without that error.

Powered by GitHub Issue Mirror