[Phpmyadmin-devel] Local static variable and unit testing

[Marc Delisle]

Le 2013-03-23 11:07, Ayush Chaudhary a écrit :
> Hi
> 
> 
> On Saturday, 23 March 2013 at 8:24 PM, Marc Delisle wrote:
>> Le 2013-03-23 08:47, Ayush Chaudhary a écrit :
>>> Hi,
>> 
>> 
>> Hello,
>> 
>>> 
>>> I was going through the existing tests. The test 'testIsHttps'
>>> under test/classes/PMA_Config_test.php looks wrong as isHttps()
>>> should be returning true for the second test case. However, the
>>> function isHttps() uses a local static variable, which is why the
>>> test is passing as in the construct, the PmaAbsoluteUri is
>>> initialised with null and isHttps() is hence returning false in
>>> all subsequent calls.
>>> 
>>> I am not sure why a local static variable was used here.
>> 
>> To avoid retesting the condition for nothing?
>> 
>>> A better test for this method would be if we could reset the
>>> static variable before each assertion. Since its a local static
>>> variable, I couldn't find a proper way to reset the static
>>> variable. Should we not move the static variable in the scope of
>>> the class? In fact, even if we don't use a static variable, we
>>> could just make is_https a class member and initialise it to NULL
>>> in the constructor. This would eventually achieve the same result
>>> in a neater way.
>>> 
>> 
>> 
>> Makes sense, but watch out: isHttps() and detectHttps() are both
>> using $is_https, for different goals.
>>> 
>>> Also, what approach do we take for resetting static variables if 
>>> needed for unit testing? One thing I came across was using 
>>> Reflection_Helper::set_static_variable($class, $name,
>>> $new_value)?
>>> 
>> 
> 
>> 
>> Yes, this approach seems interesting : 
>> http://tech.blog.box.com/2012/07/unit-testing-with-static-variables/
> 
> I think the purpose of the static variable is to only let isHttps()
> execute once in the lifecycle of a PMA_Config object. Regarding, the
> use of $is_https in detectHttps(), the copy of $is_https in
> detectHttps() is completely independent of the one in isHttps()
> because isHttps() creates a local copy of the variable. So
> detectHttps() would not need to be changed at all.

Yes, but on a maintenance perspective, it's confusing to be using the
same variable in one file, with two different purposes.
> 
> Making $is_https a member variable and using the member variable in
> isHttps() will help to make tests more exhaustive as we will be able
> to test for 1. non-https input 2. https input 3. input when is_https
> is already set (this is the only case that is being tested right
> now)
> 
> Also, regarding that approach, the issue here is that the static copy
> has scope only within the function isHttps(), so it does not seem
> possible to reset it using Reflection_Helper.
> 
> Please let me know, if this would be fine. I can then write a quick
> patch to fix this, and also improve testIsHttps()

Please do; I am not the OOP expert of the team ;)

> 
> Thanks, Ayush
> 
> 
> 
> ------------------------------------------------------------------------------
>
> 
Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics Download AppDynamics Lite
> for free today: http://p.sf.net/sfu/appdyn_d2d_mar
> 
> 
> 
> _______________________________________________ Phpmyadmin-devel
> mailing list Phpmyadmin-devel at lists.sourceforge.net 
> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
> 


-- 
Marc Delisle
http://infomarc.info