[Phpmyadmin-devel] Local static variable and unit testing

Wed Mar 27 15:14:34 CET 2013

On Wednesday, 27 March 2013 at 7:31 PM, Dieter Adriaenssens wrote:  
> 2013/3/27 Ayush Chaudhary <ayushchd at gmail.com (mailto: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 (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.
> >  
>  
>  
> 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/
> >  
> >  
> > --
> > Kind regards,
> >  
> > Dieter Adriaenssens
>  
> --  
> 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/bce6170f/attachment.html>