Make AjaxRequestTarget extendable/mockable

[Martin Grigorov <mg...@apache.org>]

Hi,

Currently AjaxRequestTarget (ART) is not very extendable because some
of its methods are final. The same reason makes it impossible to use a
mock instance for unit tests (pure unit test, not wicket tester test).
Trying to instantiate ART is also not possible because it uses
"RequestCycle.get()" in its constructor and
ThreadContext.setRequestCycle() accepts RequestCycle, the class, not
IRequestCycle - the interface, and this leads to creating mocks of
several transitive dependencies to be able to use ART.

In long term (maybe when we fix the packages split problem) I think we
have to introduce IAjaxRequestHandler and change all signatures
(onClick, onEvent, ...) to receive it instead of ART. This will also
allow passing of NoopART as some people asked for. And maybe
Optional<ART> wont be needed for AjaxFallback** components ?!

In short term removing "final" from method signatures would allow to
mock it for tests and return customized impl from
org.apache.wicket.protocol.http.WebApplication.getAjaxRequestTargetProvider().

I know 'final's helps to keep the framework stable by not allowing the
user to override critical pieces of the functionality but I don't see
better solution.

What do you think ?

-- 
Martin Grigorov
jWeekend
Training, Consulting, Development
http://jWeekend.com

Re: Make AjaxRequestTarget extendable/mockable

[Martin Grigorov <mg...@apache.org>]

On Sat, Sep 17, 2011 at 2:57 AM, Igor Vaynberg <ig...@gmail.com> wrote:
> On Fri, Sep 16, 2011 at 3:26 AM, Martin Grigorov <mg...@apache.org> wrote:
>> Hi,
>>
>> Currently AjaxRequestTarget (ART) is not very extendable because some
>> of its methods are final. The same reason makes it impossible to use a
>> mock instance for unit tests (pure unit test, not wicket tester test).
>> Trying to instantiate ART is also not possible because it uses
>> "RequestCycle.get()" in its constructor and
>> ThreadContext.setRequestCycle() accepts RequestCycle, the class, not
>> IRequestCycle - the interface, and this leads to creating mocks of
>> several transitive dependencies to be able to use ART.
>
> so pass the RC into the constructor and keep it as a field...
Actually just Response object is needed, but this is an API break...
>
>> In long term (maybe when we fix the packages split problem) I think we
>> have to introduce IAjaxRequestHandler and change all signatures
>> (onClick, onEvent, ...) to receive it instead of ART.
>
> this will basically be the public interface extracted from ART?
Yes. Will extend IPageRequestHandler and will define the public methods from ART
>
>> This will also
>> allow passing of NoopART as some people asked for.
>
> -1 on the noop thing. it seems pretty stupid.
In my previous job we used custom noop impl from 2+ years and it
served us well, but yes, maybe it shouldn't be part of the framework.
>
>> And maybe
>> Optional<ART> wont be needed for AjaxFallback** components ?!
>
> introducing optional<art> uncovered a ton of NPEs in the tree
> components. peter fixed some of these in a recent commit. some
> components need to know if they are responding to an ajax request or
> not, a noop target doesnt give them this option.
Not sure when the code changed but in 1.4 it was more safe. See
org.apache.wicket.markup.html.tree.AbstractTree.updateTree(AjaxRequestTarget)
in 1.4.
#updateTree(ART) returns early if the passed ART is null.
In 1.5 we do the check in the callers which is worse than 1.4. That's
why the bugs appeared.
With Optional<ART> it will be the same as in 1.4, just the code that
checks the parameter will be slightly different.
>
>> In short term removing "final" from method signatures would allow to
>> mock it for tests and return customized impl from
>> org.apache.wicket.protocol.http.WebApplication.getAjaxRequestTargetProvider().
>
> i thought some newer libs could mock final methods, rg
>
> http://code.google.com/p/powermock/wiki/MockFinal
Maybe this is the way to go. We use Mockito and it seems PowerMock
supports the Mockito API and it will be easier to migrate to this more
powerful library. I'll experiment.
>
>> I know 'final's helps to keep the framework stable by not allowing the
>> user to override critical pieces of the functionality but I don't see
>> better solution.
>>
>> What do you think ?
>
> its not as simple as removing the final keyword. we also need to
> rewrite whatever code is written with the assumption that methods its
> calling is final inside the ART.
Yes, this is true.
>
> -igor
>
>>
>> --
>> Martin Grigorov
>> jWeekend
>> Training, Consulting, Development
>> http://jWeekend.com
>>
>



-- 
Martin Grigorov
jWeekend
Training, Consulting, Development
http://jWeekend.com

Re: Make AjaxRequestTarget extendable/mockable

[Igor Vaynberg <ig...@gmail.com>]

On Fri, Sep 16, 2011 at 3:26 AM, Martin Grigorov <mg...@apache.org> wrote:
> Hi,
>
> Currently AjaxRequestTarget (ART) is not very extendable because some
> of its methods are final. The same reason makes it impossible to use a
> mock instance for unit tests (pure unit test, not wicket tester test).
> Trying to instantiate ART is also not possible because it uses
> "RequestCycle.get()" in its constructor and
> ThreadContext.setRequestCycle() accepts RequestCycle, the class, not
> IRequestCycle - the interface, and this leads to creating mocks of
> several transitive dependencies to be able to use ART.

so pass the RC into the constructor and keep it as a field...

> In long term (maybe when we fix the packages split problem) I think we
> have to introduce IAjaxRequestHandler and change all signatures
> (onClick, onEvent, ...) to receive it instead of ART.

this will basically be the public interface extracted from ART?

> This will also
> allow passing of NoopART as some people asked for.

-1 on the noop thing. it seems pretty stupid.

> And maybe
> Optional<ART> wont be needed for AjaxFallback** components ?!

introducing optional<art> uncovered a ton of NPEs in the tree
components. peter fixed some of these in a recent commit. some
components need to know if they are responding to an ajax request or
not, a noop target doesnt give them this option.

> In short term removing "final" from method signatures would allow to
> mock it for tests and return customized impl from
> org.apache.wicket.protocol.http.WebApplication.getAjaxRequestTargetProvider().

i thought some newer libs could mock final methods, rg

http://code.google.com/p/powermock/wiki/MockFinal

> I know 'final's helps to keep the framework stable by not allowing the
> user to override critical pieces of the functionality but I don't see
> better solution.
>
> What do you think ?

its not as simple as removing the final keyword. we also need to
rewrite whatever code is written with the assumption that methods its
calling is final inside the ART.

-igor

>
> --
> Martin Grigorov
> jWeekend
> Training, Consulting, Development
> http://jWeekend.com
>

Re: Make AjaxRequestTarget extendable/mockable

[tetsuo <ro...@gmail.com>]

> In short term removing "final" from method signatures would allow to
> mock it for tests and return customized impl from
>
org.apache.wicket.protocol.http.WebApplication.getAjaxRequestTargetProvider().

setAjaxRequestTargetProvider()?