Add a way to define configuration in atoum/*-edition packages #604

agallou · 2016-06-05T16:31:02Z

This will allow us to create "editions" of atoum, will extensions preloaded and some stuff configured.

For example : https://github.com/agallou/atoum-standard-edition

By requirering only atoum/standard-edition this will :

install atoum/atoum
install atoum/stubs
install atoum/autoloop-extension
install atoum/visibility-extension
install atoum/config-extension
install atoum/reports-extension
load autoloop-extension
load visibility-extension
load config-extension
load reports-extension
if the TELEMETRY_ENABLED environement variable is defined, the telemetry report will be sent (with different anonymization settings depending of it's value (1, 2 ou 3).

we could also imagine

preconfiguring the coveralls report repending on an COVERALLS_API_KEY environnement variable
configure the autoloop using the files defined in the autoload from the project's composer.json
add a XUNIT_REPORT_PATH : that will generate the xunit report in the path specified in the environnement variable : that will also help the integration in CI environments.

you can see an example of usage here : https://github.com/agallou/atoum-std-edition-example (the repositories and atoum/atoum in the require section should not be here. They won't be if diffrent PR are merged).

This standard edition will help users set-up atoum.
This will also :

have stubs installed with just one require (we can see the advantages here add stubs in the require section of composer.json instead of suggest #552 : out of the box autocompletion and no phpdoc in the atoum core)
have the configuration extension preinstalled : users that don't like the configuration in PHP wil be able to set some configuration with a YAML file (for example https://github.com/agallou/atoum-std-edition-example/blob/master/.atoum.yml ). Users that prefers the .atoum.php file will still continue to be able to use it as usual (with all i'ts benefits (configuration in php, we can do everything we want)).

For now we still need to define manually a .bootstrap.atoum.php file, but if the PR #523 is also merged, the installation/configuration will even be easier.

Grummfy · 2016-06-05T21:03:54Z

thanks

agallou · 2016-06-06T19:28:11Z

https://github.com/agallou/atoum-standard-edition has been updated to ease the configuration of the xUnit report.

With this, for example, the configuration in Jenkins will be a lot easier. No config file will be needed.

only an environnement variable will be needed :

export XUNIT_REPORT_PATH=app/logs/xunit.xml
./vendor/bin/atoum

and to xml file will be created (so as the directory if it does not) :

tree app 
app
└── logs
    └── xunit.xml

1 directory, 1 file

jubianchi · 2016-06-11T09:14:57Z

This solution makes atoum coupled to eventual editions: I'm not really comfortable with that.

Here what I would like to propose as an alternative: edition would push their configuration to atoum using autoloader. Here is how.

The extension will have to declare an autoloader file:

{
    ...
    "autoloaders: {
        "files": [
            "configuration.php" 
        ]
    }
}

Inside this configuration.php file there will be the code used to pre-configure atoum:

<?php

use mageekguy\atoum;
use mageekguy\atoum\scripts;

if (defined('mageekguy\atoum\scripts\runner') === true) {
    scripts\runner::configureRunner(function(atoum\runner $runner) {
        $runner->addExtension(new atoum\blackfire\extension());
        //...
    });
}

Then in the project's .atoum.php file we can imagine something like:

<?php

use mageekguy\atoum;

$runner->getExtension(atoum\blackfire\extension::class)
    ->setClientConfiguration(/*...*/)
;

This would require less coupling but some work has to be done in atoum:

Add the mageekguy\atoum\scripts\runner::configureRunner method
Add the mageekguy\atoum\runner::getExtension method

What do you think ?

agallou · 2016-06-11T12:06:34Z

The getExtension could be very usefull. I will add this.

But using composer's autoloader will be problematic (i've also tried it when working on this PR).

The issue there is that atoum does not uses (yet?) composer's autoloader.

Samll change on your proposal, instead of passing a callback, i've tested by adding an array of files that will by loaded by the enableAutorun method :

Like this :

+++ b/classes/scripts/runner.php
@@ -29,8 +29,10 @@ class runner extends atoum\script\configurable
        protected $loop = false;
        protected $looper;

+
        protected static $autorunner = true;
        protected static $runnerFile = null;
+       protected static $autorunnerConfigurationFiles = array();

        public function __construct($name, atoum\adapter $adapter = null, atoum\scripts\runner\looper $looper = null)
        {
@@ -644,6 +646,10 @@ class runner extends atoum\script\configurable

                static::$autorunner = new static($name);

+               foreach (static::$autorunnerConfigurationFiles as $file) {
+                       static::$autorunner->useConfigFile($file);
+               }
+
                return static::$autorunner;
        }

@@ -652,6 +658,11 @@ class runner extends atoum\script\configurable
                static::$autorunner = false;
        }

+       public static function addAutorunnerConfigurationFile($file)
+       {
+               static::$autorunnerConfigurationFiles[] = $file;
+       }

the standard edition file has be changed to add an autoloader containing this :

<?php

use mageekguy\atoum;
use mageekguy\atoum\scripts;

if (defined('mageekguy\atoum\scripts\runner') === true) {
    scripts\runner::addAutorunnerConfigurationFile(__DIR__ . DIRECTORY_SEPARATOR . 'configuration.php');
}

This allows us to have configuration files similar to the .atoum.php ones.

By it doesn't work as is.

In order to make this work we need to add

require_once __DIR__ . '/../../../autoload.php';

in the vendor/atoum/atoum/scripts/runner.php file.

(or apply the patch of the #523 PR)

Without that, atoum will never load composer's autoloader, and the configuration will not be loaded.

So, about the autoloader :

if we require composer's autoloader inside atoum, we will be able to use an autoload file to configure the autorunner
if we don't require composer's autoloader, we will have to find another way to automatically load the configuration (like the way it's done for the moment in the PR). But like you said, this ties atoum with the editions.

What's your thought's about that ?
(and if we go for requiring composer's autoloader, what can we do to merge the PR #523 ?)

jubianchi · 2016-06-19T14:04:03Z

classes/scripts/runner.php

@@ -652,6 +657,11 @@ public static function disableAutorun()
 		static::$autorunner = false;
 	}

+	public static function addAutorunnerConfigurationFile($file)


addConfigurationFile

jubianchi · 2016-06-19T15:03:23Z

classes/runner.php

+		}
+
+		if (1 != count($extensions)) {
+			throw new \Exception(sprintf("Extension '%s' Not found", $classname));


s/Not/not/

@agallou use a more specific exception, invalidArgument for example ;)

jubianchi · 2016-06-19T15:04:50Z

classes/runner.php

+	public function getExtension($classname)
+	{
+		$extensions = array();
+		foreach ($this->getExtensions() as $extension) {


{ should be on its on line ;)

jubianchi · 2016-06-19T15:17:39Z

classes/runner.php

+
+		if (1 != count($extensions))
+		{
+			throw new \InvalidArgumentException(sprintf("Extension '%s' not found", $classname));


please, can you throw the mageekguy\atoum\exceptions\logic\invalidArgument

jubianchi · 2016-06-23T22:36:43Z

classes/runner.php

+			$extensions[] = $extension;
+		}
+
+		if (1 != count($extensions))


I think we should improve the addExtension method and move this check there: atoum should refuse to load an extension twice.

The addExtension already checks for an already existing extension instance but it should also check if an extension of the same class is already loaded.

Doing this, we could make the code in getExtension simpler: return early in the foreach loop and throw if we exit the loop without any matching extension.

jubianchi · 2016-06-23T22:37:26Z

One last comment in the code.

Would you mind adding the removeExtension method in this PR ?

This simplifies the getExtension method

Could be called for example like this : ``` $runner->removeExtensionByClassName(mageekguy\atoum\visibility\extension::class); ```

agallou · 2016-06-25T13:04:14Z

@jubianchi PR updated :

the extension storage now only supports one extension of the same class (if another extension with the same class is passed, it will be ignore just like if we put the same object right now)
the getExtension is indeed now simpler
the removeExtension already exists a method to remove it from it's class name has been added (but wil the same limitations has the existing method : the asserters will be removed but not the arguments)

@agallou

This is a partial port of what @agallou did in atoum#604: extensions are indexed by class name. * We can load an extension the way we already did: ```php <?php $extension = new mageekguy\atoum\blackfire\extension($script); $extension ->setClientConfiguration(new \Blackfire\ClientConfiguration( $_ENV['BLACKFIRE_CLIENT_ID'], $_ENV['BLACKFIRE_CLIENT_TOKEN'] )) ->addToRunner($runner) ; ``` * If we later try to re-register the same extension, only its configuration will be updated: ```php <?php $extension = new mageekguy\atoum\blackfire\extension($script); $extension ->setClientConfiguration(new \Blackfire\ClientConfiguration( 'foo', 'bar' )) ->addToRunner($runner) ; ``` * We can also unload an extension, either using an instance or its class name: ```php $extension = new mageekguy\atoum\blackfire\extension($script); $extension->addToRunner($runner); $runner->removeExtension($extension); $runner->removeExtension(new mageekguy\atoum\blackfire\extension()); $runner->removeExtension(mageekguy\atoum\blackfire\extension::class); ```

jubianchi · 2016-09-26T22:15:39Z

I'm closing this one in favor of #634 and #635.

Thanks for your work @agallou!

@agallou

This is a partial port of what @agallou did in atoum#604: extensions are indexed by class name. * We can load an extension the way we already did: ```php <?php $extension = new mageekguy\atoum\blackfire\extension($script); $extension ->setClientConfiguration(new \Blackfire\ClientConfiguration( $_ENV['BLACKFIRE_CLIENT_ID'], $_ENV['BLACKFIRE_CLIENT_TOKEN'] )) ->addToRunner($runner) ; ``` * If we later try to re-register the same extension, only its configuration will be updated: ```php <?php $extension = new mageekguy\atoum\blackfire\extension($script); $extension ->setClientConfiguration(new \Blackfire\ClientConfiguration( 'foo', 'bar' )) ->addToRunner($runner) ; ``` * We can also unload an extension, either using an instance or its class name: ```php $extension = new mageekguy\atoum\blackfire\extension($script); $extension->addToRunner($runner); $runner->removeExtension($extension); $runner->removeExtension(new mageekguy\atoum\blackfire\extension()); $runner->removeExtension(mageekguy\atoum\blackfire\extension::class); ```

agallou force-pushed the edition_configuration branch 3 times, most recently from 2e3db4b to 437eac2 Compare June 19, 2016 13:04

jubianchi reviewed Jun 19, 2016
View reviewed changes

agallou force-pushed the edition_configuration branch from 437eac2 to 710d8d7 Compare June 19, 2016 15:01

jubianchi reviewed Jun 19, 2016
View reviewed changes

agallou force-pushed the edition_configuration branch from 710d8d7 to cd3f6cc Compare June 19, 2016 15:04

jubianchi reviewed Jun 19, 2016
View reviewed changes

agallou force-pushed the edition_configuration branch from cd3f6cc to b5448dd Compare June 19, 2016 15:11

jubianchi reviewed Jun 19, 2016
View reviewed changes

agallou force-pushed the edition_configuration branch 2 times, most recently from d311585 to 0ee9f32 Compare June 19, 2016 16:32

jubianchi changed the title ~~add a way to define configuration in atoum/*-edition packages~~ Add a way to define configuration in atoum/*-edition packages Jun 22, 2016

jubianchi reviewed Jun 23, 2016
View reviewed changes

jubianchi added in progress enhancement labels Jun 23, 2016

Hywan assigned jubianchi Jun 24, 2016

add a way to create editions of atoum

25670b7

agallou force-pushed the edition_configuration branch from 0ee9f32 to 25670b7 Compare June 25, 2016 11:20

agallou added 2 commits June 25, 2016 14:55

an extension of the same class could now only be loaded once

004658f

This simplifies the getExtension method

add a method to remove an extension from it's class name

1beb438

Could be called for example like this : ``` $runner->removeExtensionByClassName(mageekguy\atoum\visibility\extension::class); ```

agallou added 3 commits June 25, 2016 22:22

fix extension removing after extension unique by class name

c32df8e

fix unit tests

67224ce

add test on removeExtensionByClassName method

89ed6d4

jubianchi modified the milestone: 2.9.0 Jul 11, 2016

This was referenced Sep 26, 2016

Only one extension of a kind can be loaded #634

Merged

Add a addConfigurationCallable method on the runner to allow extensions to register themselves #635

Merged

jubianchi closed this Sep 26, 2016

agallou mentioned this pull request Oct 8, 2016

add a README atoum/standard-edition#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to define configuration in atoum/*-edition packages #604

Add a way to define configuration in atoum/*-edition packages #604

agallou commented Jun 5, 2016 •

edited

Grummfy commented Jun 5, 2016

agallou commented Jun 6, 2016

jubianchi commented Jun 11, 2016 •

edited

agallou commented Jun 11, 2016

jubianchi Jun 19, 2016

jubianchi Jun 19, 2016

jubianchi Jun 19, 2016 •

edited

jubianchi Jun 19, 2016

jubianchi Jun 19, 2016

jubianchi Jun 23, 2016

jubianchi commented Jun 23, 2016

agallou commented Jun 25, 2016

jubianchi commented Sep 26, 2016

Add a way to define configuration in atoum/*-edition packages #604

Add a way to define configuration in atoum/*-edition packages #604

Conversation

agallou commented Jun 5, 2016 • edited

Grummfy commented Jun 5, 2016

agallou commented Jun 6, 2016

jubianchi commented Jun 11, 2016 • edited

agallou commented Jun 11, 2016

jubianchi Jun 19, 2016

Choose a reason for hiding this comment

jubianchi Jun 19, 2016

Choose a reason for hiding this comment

jubianchi Jun 19, 2016 • edited

Choose a reason for hiding this comment

jubianchi Jun 19, 2016

Choose a reason for hiding this comment

jubianchi Jun 19, 2016

Choose a reason for hiding this comment

jubianchi Jun 23, 2016

Choose a reason for hiding this comment

jubianchi commented Jun 23, 2016

agallou commented Jun 25, 2016

jubianchi commented Sep 26, 2016

agallou commented Jun 5, 2016 •

edited

jubianchi commented Jun 11, 2016 •

edited

jubianchi Jun 19, 2016 •

edited