@tsteur opened this Issue on November 11th 2014 Owner

Either on each test run or only if it doesn't exist yet. To make this useful we should replace REMOTE_ADDR, HTTP_HOST and REQUEST_URI automatically. I think we have at least the URI in SettingsPiwik? Not sure about the rest. If not, maybe we could read them from config?

Currently, a developer has to prepare the DB test config and adjust the phpunit.xml. Having everything only in one place makes it easier to configure and to explain in documentations / blog posts.

By copying the XML all the time it also prevents the issue that sometimes the .dist files changes and people are not aware that they have to update their local file. I'm not sure if an updated XML file would take effect immediately or with the next test run but either way it would be still better.

Not sure what the best way is to resolve it but would be nice to have it. Maybe we can even get rid of the .dist file at all and define those 3 variables in bootstrap.php?

@tsteur commented on November 11th 2014 Owner

If anyone has an idea let me know. I'd like to implement it before 2.9.0 so we can directly explain this in a blog post about testing that I am currently writing.

@mnapoli commented on November 11th 2014 Member

If we override phpunit.xml then there is no point in having it anymore. If we don't override it, then it will be copied once, get outdated and it will create problems later that the developer might not understand.

Why not have the tests work by default? I.e. resolve REMOTE_ADDR & co when running the tests if they are not configured?

REQUEST_URI could be assumed equal to / by default since that's how it's setup on most dev machines I guess.

I think also we should consider having a standard dev environment setup and sticking to it. E.g. I'd say let's use PHP's built web-server (because that's the easiest setup ever, that's also the only one documented in the Getting Started guide), and assume everybody does the same. If someone doesn't, then they just customize phpunit.xml. That way HTTP_HOST can be assumed equal to localhost:8000.

@mattab commented on November 11th 2014 Owner

:+1: it would be nice if it works by default eg. for php webserver and otherwise explain how to override those 3 values eg, in a new config.php file

@tsteur commented on November 11th 2014 Owner

FYI: I tried to copy dist files to phpunit.xml all the time but it was actually only applied on the next test run. Tried quite a few things but nothing really worked so trying to get rid of the dist file at all

@mnapoli commented on November 11th 2014 Member

Why not keep the dist file? (along with all the other changes that you are doing)

I like being able to customize test suites since right now the unit tests will also run unit tests in plugins, which breaks (since many plugins have failing unit tests).

@tsteur commented on November 11th 2014 Owner

if you like to customize it, maybe it's worth adding a new testsuite and share it with all? We can of course copy it all the time but wanted to keep it simple

@tsteur commented on November 11th 2014 Owner

or actually it would be better to fix the tests ^^

@tsteur commented on November 11th 2014 Owner

and I think it is already possible to say eg "run only unit tests from core" eg ./console tests:run --testsuite unit tests/PHPUnit

@sgiehl commented on November 11th 2014 Member

Why is there a need to copy it? phpunit will by default use the phpunit.xml.dist file if no phpunit.xml exists.
+1 for keeping phpunit.xml.dist to be able to customize it (even if it should not be required for most devs)

@mnapoli commented on November 11th 2014 Member

Having phpunit.xml.dist is standard practice and supported by PHPUnit. It is meant for exactly this: developers can customize the config locally.

When phpunit is run, it will look for a phpunit.xml file, then for a dist file if it can't find one. So I don't see any reason not to do it. There is no need to copy the file.

@tsteur commented on November 11th 2014 Owner

ok I didn't know this is supported by PHPUnit so makes sense. As mentioned ideally the test runner is flexible and we make use of groups and testsuites so it is not needed to adjust it as we don't want other devs having to adjust it.

@tsteur commented on November 11th 2014 Owner

@mattab if [database_tests] is not configured yet shall we simply copy the [database] password to this section hoping they use a user having the permission to create databases etc?

@diosmosis I am not sure whether our tests are supposed to use an empty tables_prefix? I think there was something, at least for UI tests. If so, I could make sure that it is empty in bootstrap.php.

@diosmosis commented on November 11th 2014 Member

@tsteur The empty prefix is for travis to check that tables like option are properly quoted with backticks for mysql when no prefix is used. @mattab suggested adding it, and it's caught a few of my mistakes.

@tsteur commented on November 11th 2014 Owner

OK. So should we maybe change the default to an empty string here https://github.com/piwik/piwik/blob/2.9.0-b9/config/global.ini.php#L35 ? This would make sure by default tests are executed the same way as on travis. On the other side if someone configures to use the same database name for Piwik and tests we might delete the actual installation when tables_prefix is empty in [database] and database_tests. This should not happen as tables_prefix for [database] is piwik_ by default and if so it would be only a development installation... Not sure what to do.

@diosmosis commented on November 11th 2014 Member

Fixture.php has a check where if fails to drop the database if the requested database name is the same as in [database], so currently it shouldn't be allowed to use the same database in tests.

@mattab commented on December 1st 2014 Owner

Hi @tsteur closing as it looks fixed. if not fully fixed maybe create a new issue for follow up.

@tsteur commented on December 1st 2014 Owner

This is not done yet see comments waiting for your input re database configuration. Would prefer to leave this open as part of the discussion about this was already here.

@mattab commented on February 11th 2015 Owner

What's the next step here @tsteur ? maybe it would be better in Short term if it is low effort and useful?

@tsteur commented on February 11th 2015 Owner

It is minimal effort and Piwik 2.12.0 would be fine

This Issue was closed on February 23rd 2015
Powered by GitHub Issue Mirror