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

Unit tests for User #11

Closed
wants to merge 6 commits into from
Closed

Unit tests for User #11

wants to merge 6 commits into from

Conversation

agr0chal
Copy link
Member

I've created unit tests for UserManager.php and UserAuthenticator.php

Added getSessionKeys function to allow testing for setSessionKeysToNullify

Added tests for UserAuthenticator functions
*/
public function testSetPluginId() {
$this->assertEquals(NULL, $this->userManager->getPluginId());
$this->userManager->setPluginId('social_auth_mixer');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change it to 'social_auth_test'

Comment on lines 74 to 86
$this->assertTrue(
method_exists($this->userManager, 'getPluginId'),
'UserManager class does not implements getPluginId function/method'
);
$this->assertTrue(
method_exists($this->userManager, 'setPluginId'),
'UserManager class does not implements setPluginId function/method'
);
$this->assertTrue(
method_exists($this->userManager, 'getDrupalUserId'),
'UserManager class does not implements getDrupalUserId function/method'
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless the class is inheriting from an abstract class, we don't need to check this. I don't know if that's the case off the top of my head

*/
public function testGetPluginId() {
$this->userManager->setPluginId('social_auth_mixer');
$this->assertNotEquals('social_auth_reddit', $this->userManager->getPluginId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Just test that you're getting the correct value (again)

* @covers Drupal\social_api\User\UserManager::getPluginId
*/
public function testGetPluginId() {
$this->userManager->setPluginId('social_auth_mixer');
Copy link
Contributor

Choose a reason for hiding this comment

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

'social_auth_test2'

Comment on lines 109 to 128
$this->assertTrue(
method_exists($this->userAuthenticator, 'getPluginId'),
'UserAuthenticator class does not implements getPluginId function/method'
);
$this->assertTrue(
method_exists($this->userAuthenticator, 'setPluginId'),
'UserAuthenticator class does not implements setPluginId function/method'
);
$this->assertTrue(
method_exists($this->userAuthenticator, 'getSessionKeys'),
'UserAuthenticator class does not implements getSessionKeys function/method'
);
$this->assertTrue(
method_exists($this->userAuthenticator, 'setSessionKeysToNullify'),
'UserAuthenticator class does not implements setSessionKeysToNullify function/method'
);
$this->assertTrue(
method_exists($this->userAuthenticator, 'nullifySessionKeys'),
'UserAuthenticator class does not implements nullifySessionKeys function/method'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as what I mentioned before

$another_session = ['mpau82ass' => 'lplaw2b21', 'caholaaw' => '018y23czs'];

$this->userAuthenticator->setSessionKeysToNullify($sample_session);
$this->assertNotEquals($another_session, $this->userAuthenticator->getSessionKeys());
Copy link
Contributor

Choose a reason for hiding this comment

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

You're better off checking that what you set is what you get in this cases. assertNotEquals is not convenient since the session keys might be NULL and that's still no equal.

$this->assertEquals($sample_session, $this->userAuthenticator->getSessionKeys());
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

You might wanna add a test for nullifySessionKeys. That would probably require mocking the set and get methods of the data handler to use an arrray/map that you define in your test file.

@agr0chal
Copy link
Member Author

agr0chal commented Dec 27, 2019

While debugging I noticed that $session_key was a value not the key, after adding second argument in the callback, $session_key is a key, also I removed SessionPrefix because dataHandler->set adds it itself.

@@ -142,8 +152,8 @@ public function setSessionKeysToNullify(array $session_keys) {
*/
public function nullifySessionKeys() {
if (!empty($this->sessionKeys)) {
array_walk($this->sessionKeys, function ($session_key) {
$this->dataHandler->set($this->dataHandler->getSessionPrefix() . $session_key, NULL);
array_walk($this->sessionKeys, function ($value, $session_key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't pass a key => value pair to this function. $sessionKeys is supposed to be a simple array such as ['key1', 'key2']. Thus, this change has to be undone.

$sample_session = ['h78323' => '78t2gq2g7q', 'pawdwadawd' => 'cbzhzxc'];

$this->assertNotEquals($sample_session, $this->userAuthenticator->getSessionKeys());
$this->userAuthenticator->setSessionKeysToNullify($sample_session);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that only the keys of $sample_session should be passed to setSessionKeysToNullify. See the first comment about reverting this function

@gvso
Copy link
Contributor

gvso commented Dec 29, 2019

I have merged this branch with #10 . Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants