Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ticket/11620] Improve session test coverage #1496

Merged
merged 48 commits into from Jul 23, 2013
Merged

Conversation

@andychase
Copy link
Contributor

andychase commented Jun 26, 2013

The session test coverage could be improved to help prevent regressions for any factoring work on sessions.php.

PHPBB3-11620

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 26, 2013

I am fine with the fasade approach, although right now it seems to have a bit much overhead.

@andychase

This comment has been minimized.

Copy link
Contributor Author

andychase commented Jun 27, 2013

@bantu It won't seem relatively as much overhead once I start filling in the other tests with data providers and all that.

{
return array (
// [Input] $host, $server_name_config, $cookie_domain_config, [Expected] $output
// If host is ip use that ipv4

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Too many whitespaces?

This comment has been minimized.

Copy link
@andychase

andychase Jul 1, 2013

Author Contributor

Where?

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Between "that" and "ipv4" or is that intentional?

This comment has been minimized.

Copy link
@andychase

andychase Jul 1, 2013

Author Contributor

oh that was intentional.. there was going to be matching ipv6 below but it turns out the code doesn't work with ipv6 (yet?)

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Should work with ipv6 too.

This comment has been minimized.

Copy link
@andychase

andychase Jul 1, 2013

Author Contributor

Oh I forgot that it truncates the last part of ipv6... added test cases

return array (
// [Input] $host, $server_name_config, $cookie_domain_config, [Expected] $output
// If host is ip use that ipv4
array("127.0.0.1", "skipped.org", "skipped.org", "127.0.0.1"),

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Coding guidelines: Single quotes for string whereever possible.

// If no host but server name matches cookie_domain use that
array("", "example.org", "example.org", "example.org"),
// If there is a host uri use that
array("example.org", False, False, "example.org"),

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Coding guidelines: Lower-case false

/** @dataProvider extract_current_page_data */
function test_extract_current_page($root_path, $php_self, $query_string, $request_uri, $expected)
{
$output = phpbb_session_testable_facade::extract_current_page(

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Can the phpbb_session_testable_facade methods be made non-static just like session factory?

This comment has been minimized.

Copy link
@andychase

andychase Jul 1, 2013

Author Contributor

Why would that be preferred? Consistency?

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

It's not so important for tests I guess, but in general http://kore-nordmann.de/blog/0103_static_considered_harmful.html applies.

This comment has been minimized.

Copy link
@andychase

andychase Jul 1, 2013

Author Contributor

Okay, interesting.

'page' => 'index.php',
'forum' => 0
)
) ,

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Spacing

'script_path' => '/phpBB/',
'root_script_path' => '/phpBB/',
'page' => 'index.php',
'forum' => 0

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Missing comma

'root_script_path' => '/phpBB/',
'page' => 'index.php',
'forum' => 0
)

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Missing comma

*/
class phpbb_session_testable_facade
{
public static function extract_current_page($db, $session_factory, $root_path, $php_self, $query_string, $request_uri) {

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

{ on new line

* this exposes those functions through a testable facade that uses testable_factory's
* mock global variables to modify global variables used in the functions.
*
* This is using the facade pattern to provide a testable "front" to the functions in sessions.php.

This comment has been minimized.

Copy link
@bantu

bantu Jul 1, 2013

Member

Limit to max 80 chars per line.

andychase added 19 commits Jun 26, 2013
Since many functions in session.php have global variables inside the function,
this exposes those functions through a testable facade that uses testable_factory's
mock global variables to modify global variables used in the functions.

 This is using the facade pattern to provide a testable "front" to the functions in sessions.php.

 PHPBB3-11620
Renaming this file because it is going to contain a large data provider,
so I'd rather split this test out.

PHPBB3-11620
These test cases were taken from a live session,
more test cases should be added to test specific
functionality in this function.

PHPBB3-11620
Add a tests for extracting the current hostname from session.

PHPBB3-11620
Make the class functions of testable_facade no longer static methods,
but a class based one and expand the methods to be filled in, in later commits.

PHPBB3-11620
There are functions listed in testable facade that don't have a lot of dependencies,
instead mostly just take the input and perform database functions on them.
These can be tested without a testable facade function and so will be removed.

PHPBB3-11620
Add a test for the validate_referrer function.

PHPBB3-11620
indentation is probably more important than 80 characters per line apparently.
Single quotes instead of double per coding guidelines.

PHPBB3-11620
Added a test for the creation of a session with a simple test
for detecting whether a bot is present.

PHPBB3-11620
Due to an auth_refactor, there is a new dependency
in session.php on phpbb_container and a provider.

For purposes of testing, implemented a simple one.

PHPBB3-11620
PHPBB3-11620
The mock provider object now better matches the interface
given for providers.

PHPBB3-11620
try
{
$is_banned =

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

I guess it doesn't make much sense to have two lines here just to enfore the 80 chars per line limit.

// Change the global cache object for this test because
// the mock cache object does not hit the database as is
// needed for this test.
global $cache, $config, $phpbb_root_path, $php_ext;

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

Where is global php_ext used?

This comment has been minimized.

Copy link
@andychase

andychase Jul 22, 2013

Author Contributor

It's might not be used technically, but it's a required part of the phpbb_cache_service constructor so it's best to just include it to avoid confusion.

{
return array(
array('All false values, should not be banned',
false, false, false, false, /* ?: */ false),

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

What do these /* ?: */ mean?

This comment has been minimized.

Copy link
@andychase

andychase Jul 22, 2013

Author Contributor

Changed to /* should be banned? -> */

require_once dirname(__FILE__) . '/../test_framework/phpbb_session_test_case.php';

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

Remove empty line.

try
{
$is_banned = $session->check_ban($user_id, $user_ips, $user_email, $return);
} catch (PHPUnit_Framework_Error_Notice $e)

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

catch on new line.

$is_banned = $session->check_ban($user_id, $user_ips, $user_email, $return);
} catch (PHPUnit_Framework_Error_Notice $e)
{
// User error was triggered, user must have been banned

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

Can you check the type of error?

This comment has been minimized.

Copy link
@andychase

andychase Jul 22, 2013

Author Contributor

It's trigger_error($message) where message is something like '<a href="mailto:' . $config['board_contact'] . '">', '</a> blah blah

}
$this->assertEquals($should_be_banned, $is_banned, $test_msg);
$cache = new phpbb_mock_cache();

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

What does this do?

This comment has been minimized.

Copy link
@andychase

andychase Jul 23, 2013

Author Contributor

I override the global cache object with a real one that hits the database. Without reseting this other tests might fail, I will move these to setUp and tearDown for clarity.

This comment has been minimized.

Copy link
@bantu

bantu Jul 23, 2013

Member

Yes please. But other tests should also setup their dependencies on their own. ;-)

return $session;
}
protected function check_session_equals($expected_sessions, $message)

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

multiple sessions, so sessions with s in the method name?

self::bot('user agent', 13, '127.0.0.1'),
''
);
$this->assertEquals(true, $output->data['is_bot'] , 'should be a bot');

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

$output->data['is_bot'], without space

$this->session = $this->session_factory->get_session($this->db);
}
protected function assert_sessions_equal($expected, $msg)

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

Reusage. Should be in phpbb_session_test_case then.

{
$this->assert_sessions_equal(
array(
array

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

array(

(
'session_id' => 'bar_session000000000000000000000',
'session_user_id' => 4,
)),

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

Use two lines. Indent like L39 and L40, Don't forget the trailing comma.

}
/** @dataProvider referrer_inputs */
function test_referrer_inputs (

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

test_referrer_inputs( without space

);
}
/** @dataProvider referrer_inputs */

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

/** @dataProvider referrer_inputs */ (only one space)

return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/sessions_empty.xml');
}
static function referrer_inputs() {

This comment has been minimized.

Copy link
@bantu

bantu Jul 22, 2013

Member

{ on new line

function login($username, $password)
{
return array(
'status' => "",

This comment has been minimized.

Copy link
@bantu

bantu Jul 23, 2013

Member

There are spaces after the tabs on these lines.

'status' => "",
'error_msg' => "",
'user_row' => "",
);

This comment has been minimized.

Copy link
@bantu

bantu Jul 23, 2013

Member

Indentation seems incorrect (one tab too many).

// Get session here so that config is mocked correctly
$this->session = $this->session_factory->get_session($this->db);
global $cache, $config, $phpbb_root_path, $phpEx;
$this->backup_cache = $cache;

This comment has been minimized.

Copy link
@bantu

bantu Jul 23, 2013

Member

Can the global $cache be empty. Will this throw a warning or so?

This comment has been minimized.

Copy link
@andychase

andychase Jul 23, 2013

Author Contributor

I did unset($GLOBALS['cache']); before global $cache,, no error or warning. $cache is then null.

$this->session_factory = $session_factory;
}
function extract_current_page (

This comment has been minimized.

Copy link
@bantu

bantu Jul 23, 2013

Member

extract_current_page( without space. same below.

new phpbb_session_testable_facade($this->db, $this->session_factory);
}
function check_sessions_equals($expected_sessions, $message)

This comment has been minimized.

Copy link
@bantu

bantu Jul 23, 2013

Member

where did the protected go?

@bantu bantu merged commit 2fe2724 into phpbb:develop Jul 23, 2013
1 check failed
1 check failed
default The Travis CI build failed
Details
@andychase andychase deleted the andychase:ticket/11620 branch Jul 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.