[Phpmyadmin-devel] Local static variable and unit testing

[Dieter Adriaenssens]

2013/3/26 Dieter Adriaenssens <dieter.adriaenssens at gmail.com>:
> 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.

is_https isn't a member variable of the PMA_Config class, and I'm
wondering if it should be.

In detectHttps() $is_https is a local variable. (only defined in the
scope of this method) and this works.
In isHttps() $is_https is made static, forcing it to be remembered,
probably to avoid the behaviour as described in [0].

As I can see now, there are three things to be considered

1) use of static $is_https in isHttps() is not necessary -> it can be
replace by something else that makes more sense in an instanciated
class (like a class member variable, but because
$this->settings["is_https"] already exists, it makes sense to use this
one)
2) the reason why $is_https is cached/stored in isHttps()?
3) the reason why isHttps() doesn't parse PmaAbsoluteUri when it is
called a second time. (this is why the test returns a false when the
method is called a second time with a url that contains https, if it
would be a https url first and a http url second, it would return true
two times)

In case 2) and 3) I'm wondering why it is done this way.

Michal, do you have any idea why this is done this way. It would make
sense that isHttps() would not cache $is_https and would parse
PmaAbsoluteUri every time, the execution is not expensive. That said,
PmAbsoluteUri is not supposed to change because it is set in the
configfile, so in a way caching does make sense.

[0] http://sourceforge.net/p/phpmyadmin/bugs/2965/


-- 
Kind regards,

Dieter Adriaenssens