[Phpmyadmin-devel] Local static variable and unit testing
Ayush Chaudhary
ayushchd at gmail.com
Sun Mar 31 14:44:22 CEST 2013
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 (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
> >
> >
> >
>
>
Should I issue a PR removing checkIsHttps and using settings['is_https'] in isHttps() instead of static $is_https?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.phpmyadmin.net/pipermail/developers/attachments/20130331/37e79253/attachment.html>
More information about the Developers
mailing list