Use dataprovider in tests for better UI and UX

[JanMikes]

Hey, comapre results of these screenshots:

Before:

After:

Quick prototype, code before:

    public function test(): void
    {
        $this->doTestFiles([
            __DIR__ . '/Fixture/class_to_new.php.inc',
            __DIR__ . '/Fixture/class_to_interface.php.inc',
            __DIR__ . '/Fixture/interface_to_class.php.inc',
            __DIR__ . '/Fixture/name_insensitive.php.inc',
            __DIR__ . '/Fixture/twig_case.php.inc',
            __DIR__ . '/Fixture/underscore_doc.php.inc',
            __DIR__ . '/Fixture/keep_return_tag.php.inc',
            __DIR__ . '/Fixture/rename_class.php.inc',
            __DIR__ . '/Fixture/rename_interface.php.inc',
        ]);
    }

Code after:

    /**
     * @dataProvider provideTestFiles
     */
    public function test(string $file): void
    {
        $this->doTestFiles([
            $file,
        ]);
    }

    public function provideTestFiles(): array
    {
        return [
            [__DIR__ . '/Fixture/class_to_new.php.inc'],
            [__DIR__ . '/Fixture/class_to_interface.php.inc'],
            [__DIR__ . '/Fixture/interface_to_class.php.inc'],
            [__DIR__ . '/Fixture/name_insensitive.php.inc'],
            [__DIR__ . '/Fixture/twig_case.php.inc'],
            [__DIR__ . '/Fixture/underscore_doc.php.inc'],
            [__DIR__ . '/Fixture/keep_return_tag.php.inc'],
            [__DIR__ . '/Fixture/rename_class.php.inc'],
            [__DIR__ . '/ Fixture/rename_interface.php.inc'],
        ];
    }

Right now, it stops executing next files as soon as first fails.

With this improvement, you can see all failed tests/files at once.

[TomasVotruba]

I didn't know data providers work that way in PHPStorm. Cool 😎

Like it 👍

I'd just go with yield for shorter syntax.

public function provideTestFiles(): iterable
{
    yield [__DIR__ . '/Fixture/class_to_new.php.inc'];
    yield [__DIR__ . '/Fixture/class_to_interface.php.inc'];
}

That's nice job for custom Rector :)

[JanMikes]

Yeah, it is up to you, i just wanted to propose this function, it makes running test much more enjoyable and other thing is if any of your tests faill even while running them through CLI, it tells you exactly which data set was it - improves the experience.

Though please be careful as i have no idea about performance, it would be good to test if it does not slow down tests.

[TomasVotruba]

Yeah, it is up to you.

yield then.

Though please be careful as i have no idea about performance, it would be good to test if it does not slow down tests.

After the big performance improvement from 49 s to 4.8 s last week it should not be a problem. Yet there is only one way to find out :)

Are you going to implement it?

It should be a generic rector, that can accept type and method. In this case something like:

# rector.yaml
services:
    Rector\PHPUnit\Rector\Class_\ArgumentToDataProvider:
        Rector\Testing\PHPUnit\AbstractRectorTestCase:
            # method name: argument position
            doTestFiles: 0

What do you think?

[JanMikes]

In this case it needs a little more refactoring in rector itself, because what you suggested would only result in same situation, having data provider returning 1 set of files that will get tested all at once.

Data provider is for situations where you are testing something in loops:

foreach ($arrayOfFiles as $file) {
    $this->assertTrue($this->fileIsOk($file));
}

vs

$this->assertTrue($this->filesAreOkay($arrayOfFiles)); // This is rector's situation, it only has hidden the foreach logic in `doTestFiles()` method

So running rector suggested by you would result in:

public function testFilesProvider(): array
{
     yield [
            __DIR__ . '/Fixture/class_to_new.php.inc',
            __DIR__ . '/Fixture/class_to_interface.php.inc',
            __DIR__ . '/Fixture/interface_to_class.php.inc',
            __DIR__ . '/Fixture/name_insensitive.php.inc',
            __DIR__ . '/Fixture/twig_case.php.inc',
            __DIR__ . '/Fixture/underscore_doc.php.inc',
            __DIR__ . '/Fixture/keep_return_tag.php.inc',
            __DIR__ . '/Fixture/rename_class.php.inc',
            __DIR__ . '/Fixture/rename_interface.php.inc',
        ];
}

Is it understandable?

[JanMikes]

Anyways imho this is NTH and i am in time press at this moment, unable to go implement it now. Though if it is still undone by tomorrow, i can give it a try.

[JanMikes]

Here is another real life example i have found in our tests:

Before:

/**
     * @test
     */
    public function utf8fix(): void
    {
        $values = [
            "test text" => "test text",
            "Boll\xE9" => false,
            "Boll\xE9 asdfasfsaf" => "Boll asdfasfsaf",
        ];
        foreach ($values as $value => $expect) {
            $result = \Glami\Utils\UTF8Helper::utf8fix($value);
            $this->assertSame($expect, $result);
        }
    }

After:

/**
     * @test
     * @dataProvider utf8fixProvider
     */
    public function utf8fix(string $value, $expect): void
    {
        $result = \Glami\Utils\UTF8Helper::utf8fix($value);
        $this->assertSame($expect, $result);
    }

    public function utf8fixProvider()
    {
        yield ["test text", "test text"];
        yield ["Boll\xE9", false];
        yield ["Boll\xE9 asdfasfsaf", "Boll asdfasfsaf"];
    }

Though this code snippet is short, i can see complication that i transformed array key=>val into array of only values.

Tested by rewriting manually, both before and after tests passes

[TomasVotruba]

Is it understandable?

Yes yes, the doTestFiles() would have to be refactored manually. It's just 1 method, so it's ok.

Anyways imho this is NTH and i am in time press at this moment, unable to go implement it now. Though if it is still undone by tomorrow, i can give it a try.

Go for it! As you have more context about it, just give it a go. Rector patron Hotline is at your service :)

I'm studying Zend 1 now.

Here is another real life example i have found in our tests:

This is perfect example how it can be used in real life. It could be another rule, subscribed to Foreach_.

I think the iteration-to-data-provider set is being born :)

[TomasVotruba]

Covered in #1948

Now I'm going to test that on Rector's code base. Let's break it 😆

[TomasVotruba]

All 321 tests in Rector migrated 👍
in PR #1972

https://twitter.com/rectorphp/status/1172063293046972416