Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[database_tests] section no longer generated when executing php tests #17856

Closed
2 tasks
tsteur opened this issue Aug 4, 2021 · 9 comments · Fixed by #18151
Closed
2 tasks

[database_tests] section no longer generated when executing php tests #17856

tsteur opened this issue Aug 4, 2021 · 9 comments · Fixed by #18151
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Aug 4, 2021

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?

@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Aug 4, 2021
@tsteur tsteur added this to the 4.5.0 milestone Aug 4, 2021
@sgiehl
Copy link
Member

sgiehl commented Aug 4, 2021

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
Copy link
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
Copy link
Member Author

tsteur commented Sep 16, 2021

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
Copy link
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:

return new TestConfig($settingsProvider, $testingEnvironment, $allowSave = false, $doSetTestEnvironment = true);

for the $allowSave parameter?

@tsteur
Copy link
Member Author

tsteur commented Sep 20, 2021

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.

@justinvelluppillai justinvelluppillai modified the milestones: 4.5.0, 4.6.0 Oct 7, 2021
@peterhashair peterhashair self-assigned this Oct 11, 2021
@peterhashair
Copy link
Contributor

peterhashair commented Oct 11, 2021

@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
Copy link
Member Author

tsteur commented Oct 12, 2021

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
Copy link
Contributor

peterhashair commented Oct 12, 2021

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

if ($this->dropDatabaseInSetUp
|| $this->resetPersistedFixture
) {
$this->dropDatabase();
}

@tsteur
Copy link
Member Author

tsteur commented Oct 12, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
5 participants