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

Integration tests #38

Merged
merged 6 commits into from
Apr 9, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 52 additions & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,20 +1,69 @@
stages:
- name: test
- name: Test PackageGenerator
- name: Run PHPUnit
- name: Build & Post on GitHub
if: branch = master
jobs:
include:
- stage: test
- stage: Test PackageGenerator
language: php
php: '7.1'
os: linux
before_script:
- composer install
- stage: test
- stage: Test PackageGenerator
language: node_js
node_js: '9'
before_install:
- yarn global add grunt-cli
- stage: Run PHPUnit
sudo: required
language: php
php: '7.1'
services:
- docker
env:
- SUGAR_VERSION=7.11 SUGAR_EDITION=Ent
Copy link
Member

Choose a reason for hiding this comment

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

Running tests on ent, pro, and ult seems overkill? The Professor M code doesn't come in different flavors. I assume you're doing this to run the core unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notes from our chat: We're going to continue running on ent, pro, and ult as this tests the install into each version. I'm going to remove the running of the Sugar provided unit tests.

before_script:
- composer install
- cd package
- "./pack.php -v $(date +'%Y%m%d-%H.%M.%S')"
- cd ../scripts
- sudo service mysql stop
script:
- ./SetupEnvAndRunPHPUnitTests.sh $DEV_COMMUNITY_USERNAME $DEV_COMMUNITY_PASSWORD $SUGAR_VERSION $SUGAR_EDITION $GIT_HUB_USERNAME $GIT_HUB_PASSWORD workspace/sugardocker
- stage: Run PHPUnit
sudo: required
language: php
php: '7.1'
services:
- docker
env:
- SUGAR_VERSION=7.11 SUGAR_EDITION=Pro
before_script:
- composer install
- cd package
- "./pack.php -v $(date +'%Y%m%d-%H.%M.%S')"
- cd ../scripts
- sudo service mysql stop
script:
- ./SetupEnvAndRunPHPUnitTests.sh $DEV_COMMUNITY_USERNAME $DEV_COMMUNITY_PASSWORD $SUGAR_VERSION $SUGAR_EDITION $GIT_HUB_USERNAME $GIT_HUB_PASSWORD workspace/sugardocker
- stage: Run PHPUnit
sudo: required
language: php
php: '7.1'
services:
- docker
env:
- SUGAR_VERSION=7.11 SUGAR_EDITION=Ult
before_script:
- composer install
- cd package
- "./pack.php -v $(date +'%Y%m%d-%H.%M.%S')"
- cd ../scripts
- sudo service mysql stop
script:
- ./SetupEnvAndRunPHPUnitTests.sh $DEV_COMMUNITY_USERNAME $DEV_COMMUNITY_PASSWORD $SUGAR_VERSION $SUGAR_EDITION $GIT_HUB_USERNAME $GIT_HUB_PASSWORD workspace/sugardocker
- stage: Build & Post on GitHub
language: php
php: '7.1'
Expand Down
422 changes: 367 additions & 55 deletions README.md

Large diffs are not rendered by default.

Binary file added images/jenkinsbuildpassed.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsfailingbuild.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsjasminefailed.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsjasminepassed.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsphpunit.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsphpunitfailed.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsphpunitfailed2.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/jenkinsphpunitpassed.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified images/nodebuildjob.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified images/phpbuildjob.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/phpunitprofm.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/phpunitsugar.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/travisenvvars.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/travisphpunitfail.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/travisphpunitjob.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added images/travisphpunitresults.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion package/pack.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

$packageID = "ProfessorM";
$packageLabel = "Professor M School for Gifted Coders";
$supportedVersionRegex = '7\\..*$';
$supportedVersionRegex = '(8\..*|7\.(9|10|11)\..*)';

/*
* Determine the version of the zip
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

use Sugarcrm\Sugarcrm\custom\gradebook_fake\RecordManager;

class StudentGradebookJob implements RunnableSchedulerJob {
Copy link

Choose a reason for hiding this comment

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

Our internal styleguide says to put the braces for class declarations on their own line. It's not super important, but if you want to adhere to our standards, then you might want to move the { to the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the detailed review! I've made the style changes you made and opened an issue to track adding a style checker to our build process.


/**
* @var SchedulersJob
*/
protected $job;
/**
* @param SchedulersJob $job
*/
public function setJob(SchedulersJob $job)
{
$this->job = $job;
}

/**
* This function defines the job that adds a new student to the GradebookFake app
* @param $job Information about the job. $job->data should be the id for the new Student (contact) record
Copy link

Choose a reason for hiding this comment

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

Your parameter is actually called $data. You're also missing a type. It looks like it's an ID, so string would really be the only supported type.

* @return bool true if a record was successfully created in the GradebookFake app
*/
public function run($data)
{
if (!empty($data))
Copy link

Choose a reason for hiding this comment

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

PSR-2 says that the brace should be on the same line as the if. "Opening braces for control structures MUST go on the same line, and closing braces MUST go on the next line after the body."

{
$bean = $this->getContactBean($data);

try{
Copy link

Choose a reason for hiding this comment

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

PSR-2 says "Control structure keywords MUST have one space after them; method and function calls MUST NOT." So try { and if ($success) { and } else {.

//Call the external GradebookFake app to create a new record in it
$rm = $this->getRecordManager();
$success = $rm->createStudentRecord($bean->email1, $bean->first_name, $bean->last_name);
Copy link

Choose a reason for hiding this comment

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

email1 shouldn't be used anymore. SugarEmailAddress::getPrimaryAddress() can be used to get the string of the primary email address. You don't have to change this now, but you'll want to create an issue to remind you to do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I was messing with the file anyway, I went ahead and fixed this.

if ($success){
Copy link

Choose a reason for hiding this comment

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

You should have a space before the {.

$this->job->succeedJob();
return true;
} else{
Copy link

Choose a reason for hiding this comment

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

You should have a space before the {.

$this->job->failJob("Record not successfully created in GradebookFake");
return false;
}
} catch (Exception $e) {
$this->job->failJob($e->getMessage());

return false;
}

}

$this->job->failJob("Job had no data");
Copy link

Choose a reason for hiding this comment

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

We tend to use single quotes for strings unless using string interpolation. It's not a PSR rule, however.

return false;
}

protected function getContactBean($id){
Copy link

Choose a reason for hiding this comment

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

Braces for functions go on their own line.

Copy link

Choose a reason for hiding this comment

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

I would also suggest having doc blocks for all methods since people will read the code and ask questions for every method, no matter how minor and seemingly obvious.

return BeanFactory::getBean('Contacts', $id);
Copy link

Choose a reason for hiding this comment

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

BeanFactory::getBean() always returns a bean. If it can't find the right one by the $id then it just returns an empty bean of that type (Contacts). BeanFactory::retrieveBean() will return null if the record can't be found, which is what I think you want.

}

protected function getRecordManager()
{
return new RecordManager();
}

}
75 changes: 54 additions & 21 deletions package/src/custom/modules/Contacts/Students_Gradebook.php
100755 → 100644
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<?php

require_once 'include/SugarQueue/SugarJobQueue.php';
Copy link

Choose a reason for hiding this comment

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

I'm shocked that you have to include SugarJobQueue. It's actually in composer's autoload_classmap.php, so you shouldn't have to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right!


if (!defined('sugarEntry') || !sugarEntry) die('Not A Valid Entry Point');
Copy link

Choose a reason for hiding this comment

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

You don't need to do this check at the top of files anymore.


/**
Expand All @@ -15,30 +17,61 @@ class Students_Gradebook
* @param $event The current event
* @param $arguments Additional information related to the event
*/
function AddStudentToGradebook(&$bean, $event, $arguments)
public function addStudentToGradebook(&$bean, $event, $arguments)
{
if ($event !== 'after_save') {
return;
}

//Check if this is a new student record or just an update to an existing record
if(!$arguments['isUpdate']){

require_once('include/SugarQueue/SugarJobQueue.php');

//create the new job
$job = new SchedulersJob();
//job name
$job->name = "Add New Student to Gradebook Job";
//data we are passing to the job
$job->data = $bean->id;
//function to call
$job->target = "function::AddStudentToGradebookJob";

global $current_user;
//set the user the job runs as
$job->assigned_user_id = $current_user->id;

//push into the queue to run
$jq = new SugarJobQueue();
$jobid = $jq->submitJob($job);
if($arguments['isUpdate']){
Copy link

Choose a reason for hiding this comment

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

You should have a space after if and another space before {.

return;
}

$job = $this->defineJob($bean);

$this->scheduleJob($job);
}

/**
* Define the job that adds a new student to the GradebookFake app
* @param SugarBean $bean The Student (Contact) bean
* @return SchedulersJob The job that was defined
*/
protected function defineJob(\SugarBean $bean)
{
//create the new job
$job = $this->getSchedulersJob();
//job name
$job->name = "Add New Student to Gradebook Job";
//data we are passing to the job
$job->data = $bean->id;
//function to call
$job->target = "class::StudentGradebookJob";
//set the user the job runs as
$job->assigned_user_id = $GLOBALS['current_user']->id;

return $job;
}

/**
* Schedule the job to run by submitting it to the Sugar Job Queue
* @param SchedulersJob $job The job to submit
* @return string Response from submitting the job
*/
protected function scheduleJob(\SchedulersJob $job)
{
$jq = $this->getSugarJobQueue();
return $jq->submitJob($job);
}

protected function getSchedulersJob()
Copy link

Choose a reason for hiding this comment

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

Missing doc block.

{
return new SchedulersJob();
}

protected function getSugarJobQueue()
Copy link

Choose a reason for hiding this comment

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

Missing doc block.

{
return new SugarJobQueue();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
<?php

Copy link

Choose a reason for hiding this comment

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

You should add a namespace for this test.

namespace Sugarcrm\SugarcrmTestsUnit\modules\Schedulers\Ext\ScheduledTasks;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. So I know for the future...what is the benefit of namespacing unit tests?

use Sugarcrm\SugarcrmTestsUnit\TestMockHelper;
use Sugarcrm\Sugarcrm\Util\Uuid;

/**
* @coversDefaultClass \StudentGradebookJob
*/
class StudentGradebookJobTest extends \PHPUnit_Framework_TestCase
Copy link

Choose a reason for hiding this comment

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

This just recently changed to extends \PHPUnit\Framework\TestCase. I'm not sure if that will affect your or not.

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 verified that updating to \PHPUnit\Framework\TestCase works when running the tests against Sugar 7.10 and 7.11.

{
/**
* @var a partial mock of a Student (Contact) with an id, first name, last name, and email
*/
private $student;

/**
* @var a partial mock of SchedulersJob
*/
private $job;

/**
* @var a partial mock of RecordManager
*/
private $rm;

/**
* @var a partial mock of StudentGradebookJob
*/
private $sgJob;

public function setup()
Copy link

Choose a reason for hiding this comment

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

setUp and tearDown are actually protected. And they are camel cased.

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 surprised that the lower case version worked. Updated.

{
\SugarAutoLoader::load('../../../Extension/modules/Schedulers/Ext/ScheduledTasks/StudentGradebookJob.php');
Copy link

Choose a reason for hiding this comment

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

This will incur a require_once call every time a single test case is run. You have 4 tests cases, so you're calling it 4 times. You know you always need it, so you might as well just add this above the class definition...

require_once '../../../Extension/modules/Schedulers/Ext/ScheduledTasks/StudentGradebookJob.php';

class StudentGradebookJobTest extends \PHPUnit\Framework\TestCase
{
    // ...
}

Copy link

Choose a reason for hiding this comment

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

I'm also curious if you can remove the ../../../ in your path. If Extension/ is in the SugarCRM root, then I don't think the extra traversals are necessary.

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 was able to change the path to 'custom/Extension/modules....' Much easier to read!

parent::setup();

// Create a new Student (Contact) with an id, first name, last name, and email
$this->student = TestMockHelper::createPartialMock($this, '\\Contact', []);
$this->student->id = Uuid::uuid1();
$this->student->first_name = 'John';
$this->student->last_name = 'Doe';
$this->student->email1 = 'jdoe@example.com';

// Create a new SchedulersJob and set the job's data to the student's id
$this->job = TestMockHelper::createPartialMock($this, '\\SchedulersJob', ['succeedJob', 'failJob']);
$this->job->data = $this->student->id;

// Create a mock of the Gradebook's RecordManager
$this->rm = TestMockHelper::createPartialMock($this, '\\Sugarcrm\\Sugarcrm\\custom\\gradebook_fake\\RecordManager', ['createStudentRecord']);

// Create a mock of the StudentGradebookJob. getContactBean will return the mock student we created.
// getRecordManager returns the mock RecordManager we created
$this->sgJob = TestMockHelper::createPartialMock($this, '\\StudentGradebookJob', ['getContactBean', 'getRecordManager']);
$this->sgJob->method('getContactBean')->willReturn($this->student);
$this->sgJob->method('getRecordManager')->willReturn($this->rm);

// Set the job to be our mock job
$this->sgJob->setJob($this->job);
}

/**
* @covers ::run
*/
public function testRun_JobSucceeds()
{
$this->job->expects($this->once())->method('succeedJob');
$this->job->expects($this->never())->method('failJob');

$this->rm->expects($this->once())->method('createStudentRecord')->willReturn(true);

$actual = $this->sgJob->run($this->job->data);

$this->assertTrue($actual);
}

/**
* @covers ::run
*/
public function testRun_RecordManagerReturnsFalse_JobFails()
{
$this->job->expects($this->never())->method('succeedJob');
$this->job->expects($this->once())->method('failJob')->with("Record not successfully created in GradebookFake");

$this->rm->expects($this->once())->method('createStudentRecord')->willReturn(false);

$actual = $this->sgJob->run($this->job->data);

$this->assertFalse($actual);
}

/**
* @covers ::run
*/
public function testRun_RecordManagerThrowsException_JobFails()
{
$exceptionMessage = "An exception occurred";

$this->job->expects($this->never())->method('succeedJob');
$this->job->expects($this->once())->method('failJob')->with($exceptionMessage);

$this->rm->expects($this->once())->method('createStudentRecord')->willThrowException(new \Exception($exceptionMessage));

$actual = $this->sgJob->run($this->job->data);

$this->assertFalse($actual);
}

/**
* @covers ::run
*/
public function testRun_NoData_JobFails()
{
$this->job->expects($this->never())->method('succeedJob');
$this->job->expects($this->once())->method('failJob')->with("Job had no data");

$this->rm->expects($this->never())->method('createStudentRecord');

$actual = $this->sgJob->run('');

$this->assertFalse($actual);
}

}
Loading