Feature/zend test load module #3942

Closed
wants to merge 9 commits into
from

3 participants

@blanchonvincent

The goal is to have an helper for write unit tests on module :

@blanchonvincent

@Ocramius can read this PR ? thank you :)

@Ocramius Ocramius and 1 other commented on an outdated diff Mar 3, 2013
library/Zend/Test/PHPUnit/Util/ModuleLoader.php
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+namespace Zend\Test\PHPUnit\Util;
+
+use PHPUnit_Framework_TestCase;
+use Zend\Mvc\Service;
+use Zend\ServiceManager\ServiceManager;
+use Zend\Stdlib\Exception;
+
+abstract class ModuleLoader extends PHPUnit_Framework_TestCase
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

Why abstract? And why extending a PHPUnit_Framework_TestCase?

@blanchonvincent
blanchonvincent added a line comment Mar 3, 2013

Look my unit test, the goal was provide a "loadModule" method with direct assertion without usage of "new"

@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

@blanchonvincent perfectly clear what the intent was, I think that isn't an optimal approach (especially since it's an util, not a test case).

@blanchonvincent
blanchonvincent added a line comment Mar 3, 2013

@Ocramius agree with you, it was just a draft :) i will change that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on an outdated diff Mar 3, 2013
library/Zend/Test/PHPUnit/Util/ModuleLoader.php
+use Zend\Mvc\Service;
+use Zend\ServiceManager\ServiceManager;
+use Zend\Stdlib\Exception;
+
+abstract class ModuleLoader extends PHPUnit_Framework_TestCase
+{
+ /**
+ * @var ServiceManager
+ */
+ protected $serviceManager;
+
+ /**
+ * Load module
+ * @param string $moduleName
+ */
+ public function loadModule($moduleName)
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

What about passing this as constructor param?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on an outdated diff Mar 3, 2013
library/Zend/Test/PHPUnit/Util/ModuleLoader.php
+ /**
+ * Load modules
+ * @param array $modules
+ */
+ public function loadModules(array $modules)
+ {
+ $modulesPath = array();
+ $modulesList = array();
+
+ foreach ($modules as $key => $module) {
+ if (is_numeric($key)) {
+ $modulesList[] = $module;
+ continue;
+ }
+ $modulesList[] = $key;
+ $modulesPath[$key] = $module;
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

This is not really needed. The ModuleManager already recognizes modules passed in $name => $path format as far as I know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on an outdated diff Mar 3, 2013
library/Zend/Test/PHPUnit/Util/ModuleLoader.php
+ {
+ $smConfig = isset($configuration['service_manager']) ? $configuration['service_manager'] : array();
+ $this->serviceManager = new ServiceManager(new Service\ServiceManagerConfig($smConfig));
+ $this->serviceManager->setService('ApplicationConfig', $configuration);
+ $this->serviceManager->get('ModuleManager')->loadModules();
+ }
+
+ /**
+ * Get an instance of a module class by the module name
+ *
+ * @param string $moduleName
+ * @return mixed
+ */
+ public function getModule($moduleName)
+ {
+ return $this->serviceManager->get('ModuleManager')->getModule($moduleName);
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

Assumes the $this->serviceManager is set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on an outdated diff Mar 3, 2013
library/Zend/Test/PHPUnit/Util/ModuleLoader.php
+ * @param string $moduleName
+ * @return mixed
+ */
+ public function getModule($moduleName)
+ {
+ return $this->serviceManager->get('ModuleManager')->getModule($moduleName);
+ }
+
+ /**
+ * Get the array of module names that this manager should load.
+ *
+ * @return array
+ */
+ public function getModules()
+ {
+ return $this->serviceManager->get('ModuleManager')->getModules();
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

Same here: assumption that $this->serviceManager is set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on an outdated diff Mar 3, 2013
library/Zend/Test/PHPUnit/Util/ModuleLoader.php
+ *
+ * @return array
+ */
+ public function getModules()
+ {
+ return $this->serviceManager->get('ModuleManager')->getModules();
+ }
+
+ /**
+ * Get the service manager
+ * @var ServiceManager
+ */
+ public function getServiceManager()
+ {
+ if (null === $this->serviceManager) {
+ throw new Exception\LogicException('You must load modules before to have access to the service manager');
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

Instead of throwing, instantiate one with provided module config (see my previous suggestion of having a constructor and making this NOT a test case)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius
Zend Framework member

@blanchonvincent the idea looks good so far, BUT there's way too much public API in my opinion.

You should probably have something like:

  • __construct($modules, $modulePaths) (or $applicationConfig)
  • getServiceManager()
  • getModuleManager()
  • getApplication()

No other public API is needed in my opinion (that's already probably too much) :)

Usage would be:

class MyTest extends PHPUnit_Framework_TestCase
{
    public function testFoo()
    {
        $moduleLoader = new \Zend\Test\PHPUnit\Util\ModuleLoader(array(
            'modules' => array('Application')
        ));

        $this->assertInstanceOf(
            'Zend\ServiceManager\ServiceLocatorInterface',
            $moduleLoader->getServiceManager()
        );
        $this->assertInstanceOf(
            'Zend\ModuleManager\ModuleManager',
            $moduleLoader->getModuleManager()
        );
        $this->assertInstanceOf(
            'Zend\Mvc\ApplicationInterface',
            $moduleLoader->getApplication()
        );
    }
}
@blanchonvincent

@Ocramius I did not know if I had to make a external class (like you) or internal class with PHPUnit_Framework_TestCase integrated (like me).
Your solution look more natural and easy to understand for new user.

@Ocramius
Zend Framework member

@blanchonvincent it's not really about ease of use... I'm mainly thinking of something that doesn't enforce inheritance from another test case (we got so many already :( )

@blanchonvincent

@Ocramius yeah, you are right

@Ocramius
Zend Framework member

@blanchonvincent ping again once you think you've sorted it out, and thank you for picking this =)

@blanchonvincent

@Ocramius second draft, look better, i will add unit tests again later

@Ocramius Ocramius and 1 other commented on an outdated diff Mar 3, 2013
library/Zend/Test/Util/ModuleLoader.php
+ {
+ if (!isset($modules['modules'])) {
+ $modulesPath = array();
+ $modulesList = array();
+
+ foreach ($modules as $key => $module) {
+ if (is_numeric($key)) {
+ $modulesList[] = $module;
+ continue;
+ }
+ $modulesList[] = $key;
+ $modulesPath[$key] = $module;
+ }
+ $configuration = array(
+ 'module_listener_options' => array(
+ 'module_paths' => $modulesPath,
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013

As before: if a module is provided as name => path, then the module manager should already be able to handle that :)

@blanchonvincent
blanchonvincent added a line comment Mar 4, 2013

@Ocramius if you write in your config :

'modules' => array(
    'Foo' => '/path/to/my/foo',
),

?

Currently in "modules" key, only name is accepted : https://github.com/zendframework/zf2/blob/master/library/Zend/ModuleManager/ModuleManager.php#L80

@Ocramius
Zend Framework member
Ocramius added a line comment Mar 4, 2013

@EvanDotPro ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius and 1 other commented on an outdated diff Mar 3, 2013
library/Zend/Test/Util/ModuleLoader.php
+use Zend\ServiceManager\ServiceManager;
+
+class ModuleLoader
+{
+ /**
+ * @var ServiceManager
+ */
+ protected $serviceManager;
+
+ /**
+ * Load list of modules or application configuration
+ * @param array $modules
+ */
+ public function __construct(array $modules)
+ {
+ if (!isset($modules['modules'])) {
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 3, 2013
@blanchonvincent
blanchonvincent added a line comment Mar 4, 2013

@Ocramius Ok, but i don't understand how i can use this to improve the ModuleLoader ?

@Ocramius
Zend Framework member
Ocramius added a line comment Mar 4, 2013

@blanchonvincent you're basically merging config here :)

@blanchonvincent
blanchonvincent added a line comment Mar 4, 2013

@Ocramius i improve this part, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius
Zend Framework member

Looks much better, thank you! :)

@weierophinney weierophinney was assigned Mar 8, 2013
@weierophinney
Zend Framework member

@blanchonvincent I tried merging, but I'm missing the file tests/ZendTest/Test/_files/cache/module-config-cache.phpunit.php (notified in ModuleLoaderTest::testCanLoadModulesFromConfig).

@blanchonvincent

@weierophinney you must only create the "cache" directory in "tests/ZendTest/Test/_files/". I add the directory in the versioning.

@weierophinney
Zend Framework member

@blanchonvincent Do not write files inside the tests directory; write them to the system temporary directory. (We need to do this in part to allow developers to run the test suite when installed via their system package manager; if we write inside the tests directory, they may not have permissions. Plus, it's cleaner.)

@blanchonvincent

@weierophinney sorry, i changed with the system temporary directory.

@weierophinney
Zend Framework member

@blanchonvincent I ran into issues running the test suite multiple times, as the test suite is not cleaning out the cache files. Interestingly, when I implemented that logic... a new test started failing consistently, AbstractControllerTestCaseTest::testApplicationClassAndTestRestoredConsoleFlag now fails on the very first assertion of Console::isConsole. Trying to track it down...

@weierophinney weierophinney added a commit that referenced this pull request Mar 12, 2013
@weierophinney weierophinney [#3942] Clear cache files
- Clears module cache files on setup and teardown
35e077e
@weierophinney weierophinney added a commit that referenced this pull request Mar 12, 2013
@weierophinney weierophinney [#3942] CS fixes
- trailing whitespace, braces
b319113
@weierophinney weierophinney added a commit that referenced this pull request Mar 12, 2013
@weierophinney weierophinney Merge branch 'feature/3942' into develop
Close #3942
35b513b
@weierophinney
Zend Framework member

Tracked it down, and all tests are running. Thanks, @blanchonvincent -- merged to develop for release with 2.2.0.

@Ocramius
Zend Framework member

👍!

@blanchonvincent

@weierophinney @Ocramius Thank you both for your work

@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney [#3942] Clear cache files
- Clears module cache files on setup and teardown
af23c18
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney [#3942] CS fixes
- trailing whitespace, braces
d6a5f0e
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'feature/3942' into develop
Close #3942
35fc241
@weierophinney weierophinney added a commit to zendframework/zend-test that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#3942 from blanchonvinc…
…ent/feature/zend-test-load-module

Feature/zend test load module
9d43c95
@weierophinney weierophinney added a commit to zendframework/zend-test that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#3942] Clear cache files
- Clears module cache files on setup and teardown
98da0af
@weierophinney weierophinney added a commit to zendframework/zend-test that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#3942] CS fixes
- trailing whitespace, braces
9d40ad7
@weierophinney weierophinney added a commit to zendframework/zend-test that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/3942' into develop dea5c37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment