Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@dataProvider removes strict typing. #2796

Closed
Mvannes opened this issue Oct 9, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@Mvannes
Copy link

commented Oct 9, 2017

Q A
PHPUnit version 6.4.1
PHP version 7.1.9
Installation Method Composer

When running a test that makes use of strict typing, when passing a parameter that is not of the hinted type, a regular test works correctly and throws an exception.
A dataprovider does not, but code still executes. It looks to drop the strict typing.

Example:
Given the following class Foo.php

<?php
declare(strict_types=1);

namespace Test;

class Foo
{
    private $id;

    public function setId(int $id)
    {
        $this->id = $id;
    }

    public function getId(): int
    {
        return $this->id;
    }
}

And the following test class FooTest.php

<?php
declare(strict_types=1);

namespace Test;

use PHPUnit\Framework\TestCase;

class FooTest extends TestCase
{
    public function testStrictFails()
    {
        $foo = new Foo();
        $foo->setId('1');
    }
}

The following error is correctly thrown on test execution:

There was 1 error:

1) Mvannes\Test\FooTest::testStrictFails
TypeError: Argument 1 passed to Test\Foo::setId() must be of the type integer, string given, called in /home/mvannes/projects/test/test/FooTest.php on line 13

If however we write our test as follows:

<?php
declare(strict_types=1);

namespace Test;

use PHPUnit\Framework\TestCase;

class FooTest extends TestCase
{
    public function strictWorksProvider()
    {
        return [
            [ // Proper int, this should work.
                1
            ],
            [ // String. This shouldn't work.
                '1'
            ],
            [ // Bool, this really shouldn't work.
                true
            ]
        ];
    }

    /**
     * @dataProvider strictWorksProvider
     */
    public function testStrictWorks(int $id)
    {
        $foo = new Foo();
        $foo->setId($id);
        self::assertSame(1, $foo->getId());
    }
}

The tests happily execute and confirm their success!

...........................                                       27 / 27 (100%)

Time: 259 ms, Memory: 18.00MB

OK (27 tests, 40 assertions)

This doesn't seem correct. I couldn't quite find any issues like this with some searching so figured I'd make one.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

commented Oct 9, 2017

PHPUnit uses the Reflection API (ReflectionMethod::invokeArgs(), to be precise) to invoke a test method.

The passing of arguments from a data provider to the parameters of a test method is affected by https://bugs.php.net/bug.php?id=75345.

I am afraid that there is nothing I can do about this in PHPUnit.

@hboomsma

This comment has been minimized.

Copy link

commented Oct 9, 2017

Would you welcome a pull request where we check the types by hand and throw TypeErrors? Or would that be too slow?

Do you know a nice way to find if a test is running in strict_types mode without parsing the file header?

Could we maybe do something about the way we call the test method, for example bind a closure to the object scope, or use call_user_method_array. Assuming they do not have the same problem.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

commented Oct 13, 2017

No, I do not think that I would welcome such a pull request. I think that this should be fixed in PHP.

@dnaber-de

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

From the PHP Bug page:

This has been discussed on the internals list in https://externals.io/message/100851, and the decision was not change this behavior.

As it is now clear that PHP wont fix this behavior maybe you want to rethink your decision?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.