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/9983] Add redis cache driver tests. #1108

Merged
merged 15 commits into from Dec 3, 2012
Merged

Conversation

p
Copy link
Contributor

@p p commented Dec 1, 2012

In order to not overwrite data in default redis store, at least
one of redis host or post must be explicitly specified.

Redis cache driver constructor has been modified to accept
host and port as parameters. This was not added to public API
as there are more parameters being passed via global constants.

http://tracker.phpbb.com/browse/PHPBB3-9983

In order to not overwrite data in default redis store, at least
one of redis host or post must be explicitly specified.

Redis cache driver constructor has been modified to accept
host and port as parameters. This was not added to public API
as there are more parameters being passed via global constants.

PHPBB3-9983
$db->sql_close();
}

public function test_cache_sql_redis()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those should probably go into different classes/files now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One per driver?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO yes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an abstract base class containing all the tests that all implementation are supposed to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are not actually the same.

@p
Copy link
Contributor Author

p commented Dec 1, 2012

File count increased.

@bantu
Copy link
Collaborator

bantu commented Dec 1, 2012

No common tests in base class? Basic set/get test?

@p
Copy link
Contributor Author

p commented Dec 2, 2012

Added more tests.

@@ -7,11 +7,12 @@
*
*/

require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php';
require_once dirname(__FILE__) . '/common_test_case.php';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autoloading?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have no autoloading for test related classes. But I'm not against adding it, perhaps in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought tests already support it. In that case it's fine.

@bantu
Copy link
Collaborator

bantu commented Dec 2, 2012

Unfortunately my distribution does not come with redis php bindings, so can not test this trivially.

@p
Copy link
Contributor Author

p commented Dec 2, 2012

I don't think many do. I built the thing from source.

or port must be specified in test configuration. This can be done via
test_config.php as follows:

<?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No description provided.

@p
Copy link
Contributor Author

p commented Dec 3, 2012

Fixed and added apc. Tried adding xcache but it was failing too much.

@p
Copy link
Contributor Author

p commented Dec 3, 2012

https://gist.github.com/81423875ef085e6905ee scroll down for my conclusion.

@bantu
Copy link
Collaborator

bantu commented Dec 3, 2012

xcache.admin.user is probably plaintext

@bantu
Copy link
Collaborator

bantu commented Dec 3, 2012

Merge p#34

@bantu bantu merged commit db6b11a into phpbb:develop Dec 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants