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

[Feature] Make uuid mockable #23

Closed
dimsav opened this issue Mar 14, 2014 · 12 comments
Closed

[Feature] Make uuid mockable #23

dimsav opened this issue Mar 14, 2014 · 12 comments

Comments

@dimsav
Copy link

dimsav commented Mar 14, 2014

Hi and thanks for your nice package!

I tried today to write some tests to classes using Uuid::uuid4() but it was not possible. The current structure of the class doesn't allow it to be mocked in unit tests since there is no way to get an instance of the class.

@marijn
Copy link
Contributor

marijn commented Mar 14, 2014

Why do you need to mock the method?

@ramsey
Copy link
Owner

ramsey commented Mar 14, 2014

There's no need for Uuid to be mockable, especially since you can create a Uuid from a static string (Uuid::fromString()). In your tests, if you need to use a UUID that is predictable, you can use the fromString method to create one.

@dimsav
Copy link
Author

dimsav commented Mar 15, 2014

Here is my scenario.

I have a class Acme/Uuid that contains two methods:

  • generate() generates a random uuid v4 by calling Rhumsaa\Uuid:uuid4() calls
  • validate($input) a function that returns a boolean if the input is uuid v4 or not.

I want to test the generate() method, but it wouldn't make sense to write a test that will checks the returned value as this would actually test Rhumsaa\Uuid:uuid4(). That's why I need the mock. I want to write a test that makes sure that uuid4 is being called and its result is being returned. If another dev changes the method, the test will protect the project.

@marijn
Copy link
Contributor

marijn commented Mar 15, 2014

I think you're coupling your test to your implementation. I would probably test it like this:

<?php

namespace Acme;

use Acme\Uuid;
use PHPUnit_Framework_TestCase;

final class UuidTest extends PHPUnit_Framework_TestCase {

    /**
     * @test
     */
    public function generates_uuid () {
        assertThat(Uuid::generate(), isInstanceOf(Uuid::class));
    }

    /**
     * @test
     */
    public function generates_UUID4_compatible_values () {
        assertThat(Uuid::generate(), logicalNot(equalTo(Uuid::generate())));
    }

    /**
     * @test
     */
    public function knows_when_UUID_is_valid () {
        $aVersion4Uuid = Uuid::fromString('b6543b53-0d87-4de0-af80-3777f26fef02');

        assertThat($aVersion4Uuid->isValidUuid4(), isTrue());
    }

    /**
     * @test
     */
    public function knows_when_UUID_is_invalid () {
        $aVersion1Uuid = Uuid::fromString('677da381-ac38-11e3-a5e2-0800200c9a66');

        assertThat($aVersion1Uuid->isValidUuid4(), isFalse());
    }
}

But so far I'm not seeing the advantage of your layer of composition over the default implementation.

@dimsav
Copy link
Author

dimsav commented Mar 17, 2014

Hi Marijn and thanks for your reply.

The Acme/Uuid class contains the non-static method generate() which returns a random string and not an instance of Rhumsaa\Uuid\Uuid:

    public function generate()
    {
        return (string) \Rhumsaa\Uuid\Uuid::uuid4();
    }

Acme/Uuid works as a wrapper of Rhumsaa\Uuid\Uuid into my project. So how would you write a test that ensures a valid random uuid v4 is returned?

@ramsey
Copy link
Owner

ramsey commented Mar 17, 2014

@dimsav, there are a few ways you can go about this. The first is through dependency injection with something like this:

public function generate($generator = '\Rhumsaa\Uuid\Uuid')
{
    return (string) $generator::uuid4();
}

In this way, you can create a mock class and pass the name of it in as the generator, so at test time, you can test that your code does what it should. Kind of like this:

class Bar
{
    public static function uuid4()
    {
        return 'foobar';
    }
}

$f = new Foo;
var_dump($f->generate('Bar'));

However, it might not be easy for you to approach it from this way, depending on how the rest of your code is structured, so an approach like this might be better:

public function generate()
{
    $uuidGenerator = $this->getUuidGenerator();
    return (string) $uuidGenerator::uuid4();
}

public function getUuidGenerator()
{
    return '\Rhumsaa\Uuid\Uuid';
}

Now, none of your code external to this class should have to change, and you can mock getUuidGenerator to return a different class name and follow the same approach shown above with the Bar class.

Further still, you can do something like this:

public function generate()
{
    return $this->getUuidV4();
}

/**
 * @codeCoverageIgnore
 */
public function getUuidV4()
{
    return (string) \Rhumsaa\Uuid\Uuid::uuid4();
}

Now, you're telling PHPUnit/XDebug to ignore the getUuidV4 method, and you can mock this method from your tests, so that it returns whatever string you want it to return.

@dimsav
Copy link
Author

dimsav commented Mar 21, 2014

@ramsey, I tried your suggestions hoping to find a solution to the problem. None of them worked, unfortunately.

public function generate()
{
    $uuidGenerator = $this->getUuidGenerator();
    return (string) $uuidGenerator::uuid4();
}

public function getUuidGenerator()
{
    return '\Rhumsaa\Uuid\Uuid';
}

If we want to test the above script, we have the following issues:

  • it is not possible to mock getUuidGenerator(), as it belongs to the instance currently being tested.
  • it is not possible to mock the casting to string
  • we create the method 'getUuidGenerator()` only to for the tests
public function generate()
{
    return $this->getUuidV4();
}

/**
 * @codeCoverageIgnore
 */
public function getUuidV4()
{
    return (string) \Rhumsaa\Uuid\Uuid::uuid4();
}

I can't consider the above code as a solution to this issue, as we ignore the method we want to test.

Static methods are death to testability. I wish this repo helped its users by allowing them to write testable code.

@marijn
Copy link
Contributor

marijn commented Mar 21, 2014

I wish this repo helped its users by allowing them to write testable code.

Sorry mate, that's uncalled for. Quite a few good suggestions have been made here.

@marijn
Copy link
Contributor

marijn commented Mar 21, 2014

Hi Marijn and thanks for your reply.

You're welcome 😄

The Acme/Uuid class contains the non-static method generate() which returns a
random string and not an instance of Rhumsaa\Uuid\Uuid:

If you look more closely to my example you see that it returns an instance of Acme\Uuid

Acme/Uuid works as a wrapper of Rhumsaa\Uuid\Uuid into my project. So how would you write a test that ensures a valid random uuid v4 is returned?

You wanted to test if Rhumsaa\Uuid\Uuid::uuid4() was being called right? In my opininon that's not what you should test. I prefer to test the interface rather than the implementation. That's why I wrote the test generates_UUID4_compatible_values. It's a naive attempt to see if your version 4 UUID is actually randomly generated -- one of the most qualifying attributes of a version 4 UUID hence the name of the test. You could change the implementation of the test (do it a few iterations).

Does this clarify the intent of my tests a bit better? If you have questions I'll be happy to answer but don't get snarky with comments about testability, that's not cool.

@dimsav
Copy link
Author

dimsav commented Mar 21, 2014

The Acme/Uuid class contains the non-static method generate() which returns a
random string and not an instance of Rhumsaa\Uuid\Uuid:

If you look more closely to my example you see that it returns an instance of Acme\Uuid

Still, its not a string.

You wanted to test if Rhumsaa\Uuid\Uuid::uuid4() was being called right?

No, I wanted to test if the result of Rhumsaa\Uuid\Uuid::uuid4() is returned after being casted to a string. Can you send a snippet with the Acme\Uuid class according your design? This might help.

Thanks

@ramsey
Copy link
Owner

ramsey commented Oct 30, 2014

I've merged #25 into the 3.0 branch, which effectively closes out this issue.

@ramsey ramsey closed this as completed Oct 30, 2014
@dimsav
Copy link
Author

dimsav commented Oct 30, 2014

Nice thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants