[Phpmyadmin-devel] Local static variable and unit testing

[Dieter Adriaenssens]

2013/3/31 Ayush Chaudhary <ayushchd at gmail.com>:
> On Wednesday, 27 March 2013 at 7:44 PM, Ayush Chaudhary wrote:
>
>
> On Wednesday, 27 March 2013 at 7:31 PM, Dieter Adriaenssens wrote:
>
> 2013/3/27 Ayush Chaudhary <ayushchd at gmail.com>:
>
>
> On Wednesday, 27 March 2013 at 5:29 PM, Dieter Adriaenssens wrote:
>
> 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)
>
> The issue with using $this->settings['is_https'] would be that
> $this->settings['is_https'] is set by checkIsHttps() which in turn calls
> isHttps() So if in any case, isHttps() is called independently, it might
> cause issues. Since the purpose of static $is_https in isHttps() is
> precisely to make sure that isHttps() only sets $is_https to true or false
> only once in the life cycle of an object, we can have a class member like
> 'check_for_https' which is set to true in the constructor and then isHttps()
> checks if its true or false and if its true, isHttps() executes further and
> sets it to false.
>
>
> Adding the check_for_https will overcomplicate things.
> If $this->settings['is_https'] would be used by isHttps() there would
> be no use for checkIsHttps().
>
> That would be ideal if removing checkIsHttps isn't a problem. And I just
> checked, checkIsHttps is only used on line Line 94 of Config.class.php
>
>
> 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.
>
> When it runs a second time, since $is_https already has a boolean value
> (true or false) and $is_https is a static variable in the scope of isHttps()
> and isHttps() returns $is_https if it already has a boolean value, the tests
> are returning the same results every time.
>
>
> I understand the code in isHttps() and understand why it is behaving
> that way. ;)
> What I was wondering about, is why it was done that way. (see reasoning
> below)
>
> Oh, sorry I misunderstood :)
>
>
> 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/
>
>

> Should I issue a PR removing checkIsHttps and using settings['is_https'] in
> isHttps() instead of static $is_https?

Please do. Also test if this doesn't affect the current behaviour o
phpmyadmin regarding the detection of https.

--
Kind regards,

Dieter Adriaenssens