hoaproject / Dispatcher

The Hoa\Dispatcher library.
https://hoa-project.net/
12 stars 8 forks source link

Implement `Closure` dispatcher. #24

Open shulard opened 9 years ago

shulard commented 9 years ago

Hello,

I've tried to work on the #15 issue. The code is moved from the Basic to a new Closure dispatcher. I've also added a test suite based on the ClassMethod one.

I'm not totally fine because there is a lot of code duplication between the ClassMethod, the Closure and also in the Closure... It's a draft which work, I think ready for review :smile:

Hywan commented 9 years ago

This is a draft and I made a very very quick/high review. Please, tell me when a deeper review is required.

shulard commented 9 years ago

For sure it is a draft, thanks for the review... I'll make it cleaner quickly with a better atoum integration.

shulard commented 9 years ago

@Hywan just one question about function mock... I tried to use it but I don't understand how to... I tried to found some doc in atoum but there is only native function mocks...

Here my code sample :

$this->function->dispatchedMethod = function($test, $foo, $bar) {
  $test
    ->string($foo)
      ->isEqualTo('foo')
    ->string($bar)
      ->isEqualTo('bar');
};

I tried to call the dispatchedMethod but it is not defined. Are the mocks only useful here to generate a closure from a function ? I also tried to add my function definition in the given call but nothing better...

Is there any example of that kind of use ?

shulard commented 8 years ago

Hello @Hywan I need more help about that test cases,

I tried different things with function mocks but can't get it works in my case...

As an example, I write this in a test case :

$this
    ->given(
        $this->function->dispatchedMethod = function($test, $foo, $bar) {
            $test
                ->string($foo)
                    ->isEqualTo('foo')
                ->string($bar)
                    ->isEqualTo('bar');
        },
        $router = new Router\Cli(),
        $router->get(
            'a',
            '(?<_call>dispatchedMethod) ' .
            '(?<foo>foo) ' .
            '(?<bar>bar)'
        ),
        $this->route(
            $router,
            'dispatchedMethod foo bar'
        ),

        $dispatcher = new LUT\Closure(),
        $dispatcher->getParameters()->setParameters([
            'synchronous.call' => '(:call:U:)',
            'synchronous.able' => '(:able:)'
        ])
    )
    ->when($dispatcher->dispatch($router));

I'm sure that this code define the function mock for dispatchedMethod but this function is not present at the root namespace and can't be found by the Dispatcher...

It's more about atoum behaviour than my PR, but I'm sure that you know atoum better than me :smile:

Hywan commented 8 years ago

Sorry for the late reply. Please, can you explain what do you want to test and what your test case does?

Hywan commented 8 years ago

I am assigning @osaris since I have too much work.

shulard commented 8 years ago

Hello @Hywan,

Don't worry for the delay, I was just thinking about this PR yesterday and I've added a comment :smile:

shulard commented 8 years ago

Here the requested details on my test case which is built to test the behaviour of the closure dispatcher :

I've took the logic from the ClassMethod dispatcher which maybe is not the good example to start...

My main problem here is that my registered function is not registered in the place I need :

So I don't know how to use my mocked function to deal with my test needs...

shulard commented 8 years ago

Hello, any news about this PR ? I'm sorry but I'm lost in the "atoum" features...

Hywan commented 8 years ago

ping @osaris :-)

shulard commented 8 years ago

Hello @Jir4, do you know atoum function mock enough to help me here ?

Jir4 commented 8 years ago

I'm not aware about atoum :/ I don't use it at all.

Hywan commented 8 years ago

@shulard $this->function->foo = function () { … } :-).

shulard commented 8 years ago

@Hywan, so my code is right here...

I'm already able to create the mock function but I can't use it through my dispatcher.

In my test case, I try to route to the mocked function with parameters. The router is not aware of the Test\Unit\Closure test mocked functions... Maybe for that case it's not possible to use mocked function ?

Hywan commented 8 years ago

Are you trying to mock a function or a method? If this is a method, then:

$this->calling($object)->$method = function (…) { … }
shulard commented 8 years ago

I try to mock a function to test the Closure dispatcher with a function name. The function can be namespaced but it's a simple function.

Hywan commented 8 years ago

@shulard Show me what piece of code you are trying to test please :-).

shulard commented 8 years ago

@hywan: Of course :smile:

class Closure extends Test\Unit\Suite
{
    public function case_function_name_in_rule_pattern()
    {
        $this
            ->given(
                $this->function->dispatchedMethod = function($test, $foo, $bar) {
                    $test
                        ->string($foo)
                            ->isEqualTo('foo')
                        ->string($bar)
                            ->isEqualTo('bar');
                },
                $router = new Router\Cli(),
                $router->get(
                    'a',
                    '(?<_call>dispatchedMethod) ' .
                    '(?<foo>foo) ' .
                    '(?<bar>bar)'
                ),
                $this->route(
                    $router,
                    'dispatchedMethod foo bar'
                ),

                $dispatcher = new LUT\Closure(),
                $dispatcher->getParameters()->setParameters([
                    'synchronous.call' => '(:call:U:)',
                    'synchronous.able' => '(:able:)'
                ])
            )
            ->when($dispatcher->dispatch($router));
    }

    protected function route(Router $router, $uri, array $extraVariables = [])
    {
        $router->route($uri);
        $theRule                                  = &$router->getTheRule();
        $theRule[$router::RULE_VARIABLES]['test'] = $this;

        foreach ($extraVariables as $name => $value) {
            $theRule[$router::RULE_VARIABLES][$name] = $value;
        }

        return $router;
    }
}

With that code I just get the atoum error: Hoa\Dispatcher\Closure::resolve(): (0) Function dispatchedMethod is not found.

I mock the method and try to use it in the dispatcher. Before using the mocked function, I've created some namespaced functions in my test file... It works but was not really clean...

An example here

Hywan commented 8 years ago

Can you show me the SUT (the method you are trying to test) please? I don't understand why you're mocking your own function.

shulard commented 8 years ago

I'm trying to test the Closure::resolve method: https://github.com/hoaproject/Dispatcher/pull/24/files#diff-d4e09e011fe8d15e008ce4ef76685e74R62

This method must be able to dispatch to a Closure or a function. With this test case I try to validate that the Closure::resolve is able to dispatch to a function.

Am I missing the whole thing ? Maybe my testing strategy is not the good one ?

Hywan commented 8 years ago

So you don't need to mock any thing. You have a function that will be called by the dispatcher.

shulard commented 8 years ago

Yes but I need to create that function somewhere...

Maybe I can use a PHP function instead of creating a useless one. Then validate the result of the call.

Hywan commented 8 years ago

@shulard You can use your own function, of course. To call it, don't forget the namespace (__NAMESPACE__ . '\dispatchedMethod').

shulard commented 8 years ago

Super :+1: I'll work on the test suite quickly !

shulard commented 8 years ago

Hello @Hywan,

I've rebased my branch on master and update the test suite to avoid useless function creation.

Hywan commented 8 years ago

Is it ready for a review?

shulard commented 8 years ago

Yes I think ! I hope that I've used Atoum in a better way :)

I think we'll need a little refactoring because there is duplication between ClassMethod and Closure dispatchers...

Hywan commented 8 years ago

Looks correct, but I am waiting on a first review from @osaris :-)!

osaris commented 8 years ago

Hey guys, I'm here :) It looks good for me too ! Thanks for testing your feature.

shulard commented 8 years ago

Related to #11

Hywan commented 8 years ago

Will merge it as soon as possible.

shulard commented 7 years ago

Hello @Hywan,

Any news about that PR ? I just rebased all the commits on master...