Capitalized acronyms in description get lowercased and split

[nhedger]

Hi,

I've come across the following bug.

When using a capitalized acronym inside the test's description (e.g. API), when running the test it will be printed lowercase and a space will be added between each characters.

Code sample

it('requires an API key', function () {
    assertTrue(true);
});

This gets printed as:

PASS  Tests\Unit\TranslatorTest
✓ it requires an  a p i key

Tests:  1 passed
Time:   0.03s

Expected result
The description should probably be printed as is. If this is not possible, capitalized acronyms at least should be preserved.

Steps to reproduce

1. Create a new dummy test
2. Use a capitalized acronym inside the description (e.g. API)
3. Run the test
4. See that the printed description now shows the acronym as a p i instead of API

Have a nice day.

[nunomaduro]

Thanks for reporting this, just asked on discord if someone wanna take this bug. If not, I will solve it tonight or tomorrow. 🔥

[m50]

This is a bug in PHPUnit as well (and may be caused by PHPUnit), just so you are aware.

[nhedger]

This is a bug in PHPUnit as well (and may be caused by PHPUnit), just so you are aware.

Thanks for pointing this out @m50. I was actually wondering about that myself but didn't have the time to dive into PHPUnit.

[nhedger]

Did some more digging and I believe this might be linked to the way collision is creating the description from the test name.

https://github.com/nunomaduro/collision/blob/a4ef05049dbefcacef5081ce34943dc4ef8106fa/src/Adapters/Phpunit/TestResult.php#L113-L142

[alexmartinfr]

Until we have a solution that preserve the capitalization, we could use strtolower() on the $description variable to get a decent rendering in the terminal:

function it(string $description, Closure $closure = null): TestCall
{
    $filename = Backtrace::file();

    return new TestCall(TestSuite::getInstance(), $filename, sprintf('it %s', strtolower($description)), $closure);
}

This would at least render the acronym in a legible way:

✓ it requires an api key

Not ideal, but I'm a bit lost in the codebase to come with a better solution for now.

Edit : Adding the uppercase keyword. In case someone search for it again that way.

[m50]

@nhedger yeah, that's the same logic that PHPUnit uses for it's testdox as well.

If you use phpunit with the --testdox flag, with it_does_something_with_the_API as the test function name, it outputs: It does something with the a p i

This is pretty consistent in the PHPUnit world.

Something collision can do is check if the name contains spaces (which it cannot in PHPUnit) and just don't do anything, just accept the formatting as its.

<?php
...
    /**
     * Get the test case description.
     */
    public static function makeDescription(TestCase $testCase): string
    {
        $name = $testCase->getName(false);

        if (preg_match('/\s/', $name)) {
            return $name;
        }

        // First, lets replace underscore by spaces.
        $name = str_replace('_', ' ', $name);

        // Then, replace upper cases by spaces.
        $name = (string) preg_replace('/([A-Z])/', ' $1', $name);

        // Finally, if it starts with `test`, we remove it.
        $name = (string) preg_replace('/^test/', '', $name);

        // Removes spaces
        $name = trim($name);

        // Lower case everything
        $name = mb_strtolower($name);

        // Add the dataset name if it has one
        if ($dataName = $testCase->dataName()) {
            if (is_int($dataName)) {
                $name .= sprintf(' with data set #%d', $dataName);
            } else {
                $name .= sprintf(' with data set "%s"', $dataName);
            }
        }

        return $name;
    }

That would then it mean, it'll just accept whatever formatting we provide in the string, which I think probably makes more sense than attempting to re-format it.

[nhedger]

Very interesting. I was actually geared towards slightly modifying collision's makeDescription method.

Instead of inserting a space before each capital, I thought we could do :

$name = (string) preg_replace_callback('/([A-Z])[^A-Z\s]/', function ($matches) {
    return ' ' . mb_strtolower($matches[1]);
}, $name);

That way, we only add a space before capitals that are not followed by other capitals. We also lowercase that capital here, and we then can the get rid of $name = mb_strtolower($name); on line 134 which no longer serves a purpose.

What do you think ?

[m50]

That's all fine, until you have this:

<?php

it('echos an A`)

and it outputs something like It echos an a. May be hard to see, but there are two spaces between an and a, and it also changes the meaning of the test name, slightly. The great thing about Pest is that you can provide whatever formatting you want as the name of the test.

While it may be a good idea to do what you are saying @nhedger, I think it would also be a good idea for Collision to not interfere with Pest test names.

[michaeldyrynda]

Slightly related, if you pass a class path like App\User into a dataset, it’ll reformat to app\ user.

[nhedger]

Since we're basically generating our own TestCase using the TestCaseFactory, I though of the following solution:

When building the test case in TestCaseFactory, we can inject a property that will hold a boolean indicating whether we want the description to to reformated.

# \Pest\Factories\TestCaseFactory::build

$createTest = function ($description, $data) use ($className, $test) {
    $testCase = new $className($test, $description, $data);
    $this->factoryProxies->proxy($testCase);
    $testCase->reformatDescription = true;

    return $testCase;
};

Then, in collision, we can simply check for that property on the TestCase and act accordingly:

# \NunoMaduro\Collision\Adapters\Phpunit\TestResult::makeDescription

public static function makeDescription(TestCase $testCase): string
{
    $name = $testCase->getName(false);

    if (property_exists($testCase, 'reformatDescription') && $testCase->reformatDescription === true) {
        // First, lets replace underscore by spaces.
        $name = str_replace('_', ' ', $name);

        // Then, replace upper cases by spaces.
        $name = (string) preg_replace('/([A-Z])/', ' $1', $name);

        // Finally, if it starts with `test`, we remove it.
        $name = (string) preg_replace('/^test/', '', $name);

        // Removes spaces
        $name = trim($name);

        // Lower case everything
        $name = mb_strtolower($name);
    }

    // Add the dataset name if it has one
    if ($dataName = $testCase->dataName()) {
        if (is_int($dataName)) {
            $name .= sprintf(' with data set #%d', $dataName);
        } else {
            $name .= sprintf(' with data set "%s"', $dataName);
        }
    }

    return $name;
}

A probably nicer solution would be to create a Pest Adapter for collision but it seems overkill to copy all the code from the PHPUnit Adapter only to change the makeDescription method since the PHPUnit Adapter uses final classes which we currently can't extend.

Let me know what you guys think.

[alexmartinfr]

So, we have three candidate solutions so far.
We need to know if patching Collision is an option before going further on this issue.

A - Collision patch only:

• Solution: Check for spaces inside the test name string, and don't reformat if there are some:
• Drawback: This needs to alter Collision's codebase.
  Details

B - Collision patch + Pest patch:

• Solution: Adding a $reformatDescription boolean to TestCaseFactory.
• Drawback: This needs to alter Collision's codebase, unless we create a Pest Adapter.
  Details

C - Pest patch only:

• Solution: Apply strtolower() to $description on the test() & it() methods definitions.
• Drawback: it will only fix half of the issue, as this doesn't preserve characters's capitalisation.
  Details

So far, Nuno doesn't have time to evaluate the issue, so I suggest we go try the Pest-only patch, and revisit the others valid options later on.
Any thought on that?

[m50]

I personally still believe option A is the best option.

One of the big benefits of using a string as the name of the test, instead of parsing a function name, is that we can use whatever formatting we want. Option C completely erases that, defeating the biggest benefit of using a string for generating a test name.

[nhedger]

@alexmartinfr There is also the option to create a Pest Adapter for Collision.

@m50 While I initially thought this would be a good idea too, there's at least one drawback to this approach. Even if highly unlikely, one may use the function like this:

test('API', function() {
    // do something
})

In that case, there are no spaces in the description so the formatting would not be preserved even though we'd want it to be.

[m50]

Ok, I agree with your point there, so option B is the best option then @nhedger.

Or create a pest adapter for collision, which may go hand in hand with option B.

[alexmartinfr]

You are both right about A & C.

Option B with the Pest Adapter seems to be the cleanest way 👍

[nunomaduro]

Fixed nunomaduro/collision@5e853c5