Tests refactoring

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Tests refactoring

VANKEISBELCK Remi
It's done, I have refactored the unit tests so that they use a MockServletContext whenever they need to access the configuration.

The current test base was relying on ugly statics, it ain't the case now, we create and stop the StripesFilter (via MockServletContext) when a utest needs to access the configuration.

This StripesFilter.getConfiguration() thing is pretty abscure to me, so again please tell me why it's done like this if you have any idea... I'd quite like to remove all this for a simpler mechanism...

Cheers

Remi
------------------------------------------------------------------------------
The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
is your hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials, tech docs,
whitepapers, evaluation guides, and opinion stories. Check out the most
recent posts - join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Stripes-development mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/stripes-development
Reply | Threaded
Open this post in threaded view
|

Re: Tests refactoring

VANKEISBELCK Remi
I've just done a few tests to check if we could make this Configuration thing just simple.

As I suspected, all tests pass just fine and the examples webapp works fine as well.

I'm attaching the patch for review. I'd vote for applying it (at least to trunk) unless we have a good answer for the current design.

Cheers

Remi





Le 17 févr. 2013 à 21:53, Remi VANKEISBELCK a écrit :

> It's done, I have refactored the unit tests so that they use a MockServletContext whenever they need to access the configuration.
>
> The current test base was relying on ugly statics, it ain't the case now, we create and stop the StripesFilter (via MockServletContext) when a utest needs to access the configuration.
>
> This StripesFilter.getConfiguration() thing is pretty abscure to me, so again please tell me why it's done like this if you have any idea... I'd quite like to remove all this for a simpler mechanism...
>
> Cheers
>
> Remi


------------------------------------------------------------------------------
The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
is your hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials, tech docs,
whitepapers, evaluation guides, and opinion stories. Check out the most
recent posts - join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Stripes-development mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/stripes-development

configcleanup.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Tests refactoring

Marcus Kraßmann-2
In reply to this post by VANKEISBELCK Remi
Hi Remi,

The static StripesFilter.getConfiguration() makes access to the current
runtime configuration very convenient (and it is heavily used within
many Stripes internal classes). On the other hand, statics in general
make unit testing a mess. And Stripes already offers Dependency
Injection with the ConfigurableComponent interface for custom Stripes
components.

You wrote about a simpler mechanism. I'm curious what you are thinking
about.

Kind regards,
Marcus


Am 17.02.2013 21:53, schrieb Remi VANKEISBELCK:

> It's done, I have refactored the unit tests so that they use a MockServletContext whenever they need to access the configuration.
>
> The current test base was relying on ugly statics, it ain't the case now, we create and stop the StripesFilter (via MockServletContext) when a utest needs to access the configuration.
>
> This StripesFilter.getConfiguration() thing is pretty abscure to me, so again please tell me why it's done like this if you have any idea... I'd quite like to remove all this for a simpler mechanism...
>
> Cheers
>
> Remi
> ------------------------------------------------------------------------------
> The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
> is your hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials, tech docs,
> whitepapers, evaluation guides, and opinion stories. Check out the most
> recent posts - join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> Stripes-development mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/stripes-development


------------------------------------------------------------------------------
The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
is your hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials, tech docs,
whitepapers, evaluation guides, and opinion stories. Check out the most
recent posts - join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Stripes-development mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/stripes-development
Reply | Threaded
Open this post in threaded view
|

Re: Tests refactoring

VANKEISBELCK Remi
Yeah I know it's used as a "global accessor", and I don't plan to change this now, StripesFilter.getConfiguration() is called from everywhere... Of course it would be better to have this as an instance field or method.

The unit tests were actually relying on a very crappy mechanism that initialized the whole Configuration thing once for all tests. I noticed that when I tried to write a test that closed the MockServletContext (thereby destroying StripesFilter) that passed when ran alone, but had side effects on other unit tests. I rewrote all this to actually open/close the context (and thereby StripesFilter and config stuff) correctly in @Before/AfterClass.

We could stop here, it works :

MockServletContext ctx = createCtx(); // create ctx and init StripesFilter
try {
        // test code
} finally {
        ctx.close(); // close ctx, destroy StripesFilter, releases configurations
}

The unit tests now reproduce the "real webapp" behavior for the context lifecycle.  

Now my concern is about the management of Configuration objects. For the moment StripesFilter uses several instance fields, weak refs and the like in order to "allow several Configuration objects per Class Loader". The overall routine is quite complex IMHO and I don't understand why this is needed. I've tried to replace all this stuff by a static ref to the Configuration object, and everything seems to work...

I know Tim, he ain't the type of guy that writes over-engineered code, so I'm guessing the reason for all that stuff.

Cheers

Remi


Le 18 févr. 2013 à 10:20, Marcus Kraßmann a écrit :

> Hi Remi,
>
> The static StripesFilter.getConfiguration() makes access to the current
> runtime configuration very convenient (and it is heavily used within
> many Stripes internal classes). On the other hand, statics in general
> make unit testing a mess. And Stripes already offers Dependency
> Injection with the ConfigurableComponent interface for custom Stripes
> components.
>
> You wrote about a simpler mechanism. I'm curious what you are thinking
> about.
>
> Kind regards,
> Marcus
>
>
> Am 17.02.2013 21:53, schrieb Remi VANKEISBELCK:
>> It's done, I have refactored the unit tests so that they use a MockServletContext whenever they need to access the configuration.
>>
>> The current test base was relying on ugly statics, it ain't the case now, we create and stop the StripesFilter (via MockServletContext) when a utest needs to access the configuration.
>>
>> This StripesFilter.getConfiguration() thing is pretty abscure to me, so again please tell me why it's done like this if you have any idea... I'd quite like to remove all this for a simpler mechanism...
>>
>> Cheers
>>
>> Remi
>> ------------------------------------------------------------------------------
>> The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
>> is your hub for all things parallel software development, from weekly thought
>> leadership blogs to news, videos, case studies, tutorials, tech docs,
>> whitepapers, evaluation guides, and opinion stories. Check out the most
>> recent posts - join the conversation now. http://goparallel.sourceforge.net/
>> _______________________________________________
>> Stripes-development mailing list
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/stripes-development
>
>
> ------------------------------------------------------------------------------
> The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
> is your hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials, tech docs,
> whitepapers, evaluation guides, and opinion stories. Check out the most
> recent posts - join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> Stripes-development mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/stripes-development


------------------------------------------------------------------------------
The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
is your hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials, tech docs,
whitepapers, evaluation guides, and opinion stories. Check out the most
recent posts - join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Stripes-development mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/stripes-development