[Phpmyadmin-devel] Local static variable and unit testing

Wed Mar 27 14:52:55 CET 2013

On Wednesday, 27 March 2013 at 5:29 PM, Dieter Adriaenssens wrote:  
> 2013/3/26 Dieter Adriaenssens <dieter.adriaenssens at gmail.com (mailto:dieter.adriaenssens at gmail.com)>:
> > 2013/3/24 Marc Delisle <marc at infomarc.info (mailto: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.
> 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.  
>  
> 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
>  
> ------------------------------------------------------------------------------
> Own the Future-Intel® Level Up Game Demo Contest 2013
> Rise to greatness in Intel's independent game demo contest.
> Compete for recognition, cash, and the chance to get your game  
> on Steam. $5K grand prize plus 10 genre and skill prizes.  
> Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
> _______________________________________________
> Phpmyadmin-devel mailing list
> Phpmyadmin-devel at lists.sourceforge.net (mailto:Phpmyadmin-devel at lists.sourceforge.net)
> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
>  
>  


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.phpmyadmin.net/pipermail/developers/attachments/20130327/6de9e1af/attachment.html>