Problem with newTestedInstance and class with wrong reflection

[jubianchi]

@vonglasow reported a problem using newTestedInstance/newInstance on PHP classes for which the reflection is not correct:

<?php

namespace tests\units;

use atoum;

class splFileObject extends atoum
{
    public function testFoo()
    {
        new \SplFileObject(__FILE__);
    }

    public function testBar()
    {
        $this->newTestedInstance(__FILE__);
    }
}

This test will produce an error in the testBar method only: SplplFileObject::__construct(test.php): failed to open stream: Invalid argument. Why ? Because the reflection on \SplFileObject is wrong: https://3v4l.org/YP4oR

As a workaround, we could provide some kind of configurable factory for the newTestedInstance/newInstance methods. Here is an example:

<?php

namespace tests\units;

use atoum;

class splFileObject extends atoum
{
    public function beforeTestMethod($method)
    {
        $class = $this->getTestedClassName();

        $this->instanciateTestedClassThrough(function($filename, $open_mode = null, $use_include_path = null, $context = null) use ($class) {
            return new $class($filename, $open_mode ?: 'r', $use_include_path ?: false, $context);
        });
    }

    public function testFoo()
    {
        new \SplFileObject(__FILE__);
    }

    public function testBar()
    {
        $this->newTestedInstance(__FILE__);
    }
}

[jubianchi]

I'm implementing the feature and I don't know what to choose between:

$testedClassName = $this->getTestedClassName();

$this->instanciateTestedClassWith(function($arg1, $arg2 = null) use ($testedClassName) {
    return new $testedClassName($arg1, $arg2);
});

$this->instanciateTestedClassWith(function($testedClassName, $arg1, $arg2 = null) {
    return new $testedClassName($arg1, $arg2);
});

$this->instanciateTestedClassWith(function(\reflectionClass $testedClass, $arg1, $arg2 = null) {
    return $testedClass->newInstanceArgs(array($arg1, $arg2));
});

The 1. forces the user to set a temporary variable holding the tested class name and use it in the closure. So I wrote the 2. which automatically passes the tested class name as the first argument of the factory closure. And then, I came up with the third solution which seems more flexible to me.

if we only consider php >= 5.3 && < 5.6 I would say that the 2. is the best. Now if we consider PHP >= 5.6 the 3. seems better, for example it will allow things like

$this->instanciateTestedClassWith(function(\reflectionClass $testedClass, $arg1 = null, ...$args) {
    array_unshift($args, $arg1 ?: 'foobar');

    return $testedClass->newInstanceArgs($args);
});

@atoum/all let me know what you think :)

[Hywan]

3 sounds pretty good.

[iamdey]

I vote for 2 which sounds simple to use / understand

[jubianchi]

@esion in fact, you are right. As we discussed together, the 2. still lets the user do:

$this->instanciateTestedClassWith(function($testedClassName, $arg1 = null, ...$args) {
    array_unshift($args, $arg1 ?: 'foobar');

    return new $testedClassName(...$args);
});

So now I would say 2. is better for both php >= 5.3 && < 5.6 and >= 5.6

[vonglasow]

2 looks good for me too.

[mikaelrandy]

My initial reaction was to find solution 2 easier to understand, but the @jubianchi last argument finally convinced me for the solution 3

[vonglasow]

ping ?

[Grummfy]

up

[Grummfy]

let drop it?