Skip to content

Commit

Permalink
Dev: Finish first remote API unit test
Browse files Browse the repository at this point in the history
  • Loading branch information
@olleharstedt
olleharstedt committed Apr 26, 2019
1 parent b478314 commit 9dcffbb
Showing 1 changed file with 30 additions and 4 deletions.
34 changes: 30 additions & 4 deletions tests/helpers/RemoteControlTest.php
View file
Expand Up @@ -38,25 +38,51 @@ public static function setupBeforeClass()
*/
public function testAddResponse()
{
\Yii::import('application.helpers.remotecontrol.remotecontrol_handle');
\Yii::import('application.helpers.remotecontrol.remotecontrol_handle', true);
\Yii::import('application.helpers.viewHelper', true);
\Yii::import('application.libraries.BigData', true);

// Import survey
$filename = self::$surveysFolder . '/limesurvey_survey_666368.lss';
self::importSurvey($filename);
self::$testHelper->activateSurvey(self::$surveyId);

// Create handler.
$admin = new \AdminController('dummyid');
$handler = new \remotecontrol_handle($admin);

// Get session key
// Get session key.
$sessionKey = $handler->get_session_key(
self::$username,
self::$password
);

//var_dump($sessionKey);
// Get sgqa.
$survey = \Survey::model()->findByPk(self::$surveyId);
$question = $survey->groups[0]->questions[0];
$sgqa = self::$surveyId . 'X' . $survey->groups[0]->gid . 'X' . $question->qid;

// Add response
// Check result
$response = [
$sgqa => 'One answer'
];
$result = $handler->add_response($sessionKey, self::$surveyId, $response);

// Check result via database.
$dbo = \Yii::app()->getDb();
$query = sprintf('SELECT * FROM {{survey_%d}}', self::$surveyId);
$result = $dbo->createCommand($query)->queryAll();
$this->assertCount(1, $result, 'Exactly one response');
$this->assertEquals('One answer', $result[0][$sgqa], '"One answer" response');

// Check result via API.
$result = $handler->export_responses($sessionKey, self::$surveyId, 'json');
$this->assertNotNull($result);
$responses = json_decode(file_get_contents($result->fileName));
$this->assertTrue(count($responses->responses) === 1);

// Cleanup
self::$testSurvey->delete();
self::$testSurvey = null;
}
}

12 comments on commit 9dcffbb

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how you can test remopte control inside LimeSurvey APP ?

Dodn't we need a script more external ? and here TestBaseClass is inside LS .

the biggest issue in RC is get_session_key : i don't see how to try get_session_key inside LimeSurvey ?

@olleharstedt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm testing the remote API helper class, not the remote API controller.

@olleharstedt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shnoulle Just a note: To make the code easier to test, only the CONTROLLER should contain interaction with the outside world, like the request object. Really important to separate concerns for test-driven development.

@olleharstedt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can actually be taken one step further, to clearly separate functions with side-effects (writing/reading to file or database) from functions without.

@olleharstedt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another technique that has become very popular the last years is something called dependency injection. This means that if class A depends on class DB, A will not ask for DB, but DB will be injected into A, e.g. in the constructor (there are frameworks that deal with this in a systematic matter). This makes it easier to create a dummy class DB_dummy when we're trying to test A.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make the code easier to test, only the CONTROLLER should contain interaction with the outside world, like the request object.

Here it must be https://github.com/LimeSurvey/LimeSurvey/blob/master/application/controllers/admin/remotecontrol.php right ?

Or do you write this about a specific commit in done ?

@olleharstedt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a general remark.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, i try then :).

For example, i have the bad habit to add Permission::model check inside model. But this must be a Class variable or a function param : right ?

@olleharstedt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not sure. What's the context?

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

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

LimeSurvey/application/models/Survey.php

Line 223 in 4f704d3

if (!Permission::model()->hasSurveyPermission($this->sid, 'survey', 'delete')) {

@olleharstedt
Copy link
Contributor Author

@olleharstedt olleharstedt commented on 9dcffbb Apr 29, 2019 •
edited

Choose a reason for hiding this comment

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

Right. So, in a system with dependency injection you would instead have

class Survey
{
  public function __constructor(PermissionServiceInterface $permissionService)
  {
    $this->permissionService = $permissionService;
  }
  public function delete()
  {
    if ($this->permissionService->hasPermission(...)) {
      ...
    }
  }
}

This way, when doing tests, you can insert a dummy permission class:

class DummyPermission implements PermissionServiceInterface
{
  public function hasPermission()
  {
    return true;  // Always, we don't care in this test.
  }
}

$contr = new Survey(new DummyPermission);
$this->assertTrue($contr->delete());  // Or something

Instead, now I have to hack the session:

LimeSurvey/tests/TestBaseClass.php

Line 66 in 4f704d3

\Yii::app()->session['loginID'] = 1;

@olleharstedt
Copy link
Contributor Author

@olleharstedt olleharstedt commented on 9dcffbb Apr 29, 2019 •
edited

Choose a reason for hiding this comment

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

The question is how to handle the case when number of dependencies increases a lot. For models, we also have a dependency on the database, for example. Often the request is required. The session. And so on. When testing, all these objects have to be hacked around in different ways, making the code pretty fragile.

Please sign in to comment.