[Phpmyadmin-devel] Local static variable and unit testing

[Dieter Adriaenssens]

2013/3/24 Marc Delisle <marc at infomarc.info>:
> 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 ;)

I'm trying to figure out what is going on with $is_https in the
PMA_Config class. I'll have another look at the code later and do some
tests. I'll get back to you.

-- 
Kind regards,

Dieter Adriaenssens