Test environment and DI refactor #7931
Conversation
…ndler in TestingEnvironment to TestConfig object.
…te kernel globals. Instead of adding configFile... static members in Environment, use a single static array of EnvironmentManipulators.
…ry because EventDispatcher is now stored in DI).
…t.php and get one system + one UI test to pass.
…nInstance issues after having put certain classes in DI.
…ss::setSingletonInstance. Added ability for IntegrationTestCases to provide DI config overrides.
…tion test setup, and login as superuser in Fixture setUP.
…ce, make Fixture::loginAsSuperUser public and fix LocalTracker issue that occurs if Auth is created by DI (old auth info is not overwritten, causing auth errors).
…tly, instead use 'test' environment name). And Fix ManagerTest (order of plugins in one test changes because they are sorted by TestConfig, and loadTrackerPlugins doesn't remove all plugins and re-add, it just removes plugins from internal array).
…st and Monolog's LogTest.
… ArchvieWebTest).
…ng to define specialized Fixtures.
| @@ -64,6 +64,7 @@ public function test_constructor_shouldEstablishADatabaseConnection_AsSoonAsWeSe | |||
| */ | |||
| public function test_setSettingValue_shouldThrowException_IfAnonymousIsTryingToSetASettingWhichNeedsUserPermission() | |||
| { | |||
| $this->setAnoymous(); | |||
There was a problem hiding this comment.
Anoymous? Shouldn't it be Anonymous?
|
it's nice that you try to help, but actually please take a look our previous Pull requests and our existing style of code reviews. In general, we try not to review trivial points such as most of the ones you point out. During the review we focus on bugs, security, whether the code works, whether it makes usable interfaces, whether it's documented in user and developer guides, wheter hacks are avoided, etc. generally we don't care about minor details like missing comment blocks or missing line breaks etc. |
|
But it will be really good to check the code compliance with PSR-2 before the commit. |
|
@mattab I usually avoid commenting in things that are already in the code. I see there are many "missing empty line in the end of file" and "missing docblocks" and I didn't pointed them out. But if a new code is being introduced or modified (lines with diffs), imho it makes sense to get it following PSR-2 at least. Since these are really trivial points, the author will probably take few minutes to fix all of them in one single commit. Feel free to delete the ones you think are inappropriate. 👍 A good way to force good code style and avoid flooding the PR is by running an automatic tool in Travis to check PSR-2 compliance, so everybody gets forced to follow the same pattern and it helps the team to improve the readability of the code, which also improve productivity. I'd suggest https://github.com/FriendsOfPHP/PHP-CS-Fixer or https://github.com/squizlabs/PHP_CodeSniffer. Cheers! |
| @@ -115,7 +120,10 @@ private function createContainer() | |||
| protected function getGlobalSettingsCached() | |||
| { | |||
| if ($this->globalSettingsProvider === null) { | |||
| $this->globalSettingsProvider = $this->getGlobalSettings(); | |||
| $result = $this->invokeFirstManipulator( | |||
There was a problem hiding this comment.
I'd suggest this approach:
protected function getGlobalSettingsCached()
{
if (null !== $this->globalSettingsProvider) {
return $this->globalSettingsProvider;
}
$this->globalSettingsProvider = $this->invokeFirstManipulator(
'makeKernelObject',
array(
'Piwik\Application\Kernel\GlobalSettingsProvider',
$this->getKernelObjectsArray()
)
);
if (! $this->globalSettingsProvider instanceof GlobalSettingsProvider) {
$this->globalSettingsProvider = $this->getGlobalSettings();
}
return $this->globalSettingsProvider;
}This way, you're checking if globalSettingsProvider is always a instance of GlobalSettingsProvider, since invokeFirstManipulator could return anything. I also made some code cleanup by removing the ternary operator and by adding an early return.
|
@fabiocarneiro You can stop reviewing this PR, I am taking it in a different direction. You should also note, you can avoid flooding PRs with your comments by leaving just one. |
|
@kaz231 @fabiocarneiro it would be good to have PSR-2 in Piwik and fix those little trivial matters. However we won't make the code compliant with human code reviews as it is not high value work for us to manually review those things. Rather as you point out we need to automate it with PHP CS fixer. This is discussed in this issue: #7186 Fix warnings + coding standards with PHP-CS-Fixer |
|
At the same beginning, this will be enough - https://confluence.jetbrains.com/display/PhpStorm/PHP+Code+Sniffer+in+PhpStorm . |
|
Everything in this PR has been done in another PR (2 waiting for merge, 1 new one with changes that aren't in this one). Closing and removing from current milestone. |
diosmosis
added
not-in-changelog
This PR includes several improvements to test environment setup, and includes several DI related improvements.
List of changes:
Piwik\ConfigPiwik\EventDispatcher,Piwik\Access,Piwik_TestingEnvironment(renamed toPiwik\Tests\Framework\TestingEnvironment)2. Allow event observers to be defined before the DI container is created by creating a newobservers.globalDI entry.3. Removed use of the GlobalSettingsProvider singleton, it's no longer needed.4. Introduced the concept of EnvironmentManipulators (or possibly EnvironmentInterceptors). These are hooks that are only used for testing setup, and should not be used by plugins, plugin tests or core tests, only test environment setup.'test'environment name (ie,new Environment('test')).DI\decorate.7. Added the ability for Integration tests and system tests to override DI config for tests (not just for child processes as is done currently). This is done by overridingprovideContainerConfigfor integration tests andprovideContainerConfigBeforeClassfor system tests.There are still some TODOs left, but they are just about removing unused code + filling out docs.