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 :
--
Marc Delisle
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
_______________________________________________
Phpmyadmin-devel mailing list
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.
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()
Thanks,
Ayush