From 98dac9b3148b8eeea243ccb2d47668b1d77d1167 Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Mon, 27 Nov 2017 15:14:16 +1300 Subject: [PATCH] Update unit tests to be rid of Phockito This odd dependency is causing inclusion issues to do with namespacing and the ability to work well with the framework (according to the dependency's readme). The usage of the tool in this case adds no value, as it's performing purely stub type work, rather than mock work. So instead we can use an actual stub (simply return test values depending on input) and have everything work well. After removing the Phockito dependency it was found that the tests that would skip when it isn't present were actually in a failing state. So they had to be repaired too, mostly through the use of fixing the poor code that was causing them to fail (as opposed to being bad tests). This is both configuration and proceedural work. --- _config/injector.yml | 2 +- _config/routes.yml | 8 +- ...nks.php => CMSExternalLinksController.php} | 29 +++---- src/Model/BrokenExternalLink.php | 15 ++-- src/Model/BrokenExternalPageTrackStatus.php | 8 +- src/Reports/BrokenExternalLinksReport.php | 27 ++++--- src/Tasks/CheckExternalLinksTask.php | 24 +++--- src/Tasks/CurlLinkChecker.php | 1 + tests/ExternalLinksTest.php | 75 +++---------------- tests/Stubs/PretendLinkChecker.php | 28 +++++++ 10 files changed, 97 insertions(+), 120 deletions(-) rename src/Controllers/{CMSExternalLinks.php => CMSExternalLinksController.php} (77%) create mode 100644 tests/Stubs/PretendLinkChecker.php diff --git a/_config/injector.yml b/_config/injector.yml index 45b3195..4d890b6 100644 --- a/_config/injector.yml +++ b/_config/injector.yml @@ -2,7 +2,7 @@ Name: externallinksdependencies --- SilverStripe\Core\Injector\Injector: - LinkChecker: SilverStripe\ExternalLinks\Tasks\CurlLinkChecker + SilverStripe\ExternalLinks\Tasks\LinkChecker: SilverStripe\ExternalLinks\Tasks\CurlLinkChecker Psr\SimpleCache\CacheInterface.CurlLinkChecker: factory: SilverStripe\Core\Cache\CacheFactory constructor: diff --git a/_config/routes.yml b/_config/routes.yml index 7320d40..2ac22a9 100644 --- a/_config/routes.yml +++ b/_config/routes.yml @@ -1,7 +1,7 @@ --- -Name: externallink -After: framework/routes +Name: externallinkroutes +Before: '#adminroutes' --- -Director: +SilverStripe\Control\Director: rules: - 'admin/externallinks//$Action': 'CMSExternalLinksController' + 'admin/externallinks//$Action': SilverStripe\ExternalLinks\Controllers\CMSExternalLinksController diff --git a/src/Controllers/CMSExternalLinks.php b/src/Controllers/CMSExternalLinksController.php similarity index 77% rename from src/Controllers/CMSExternalLinks.php rename to src/Controllers/CMSExternalLinksController.php index 2bafb3f..7f35df1 100644 --- a/src/Controllers/CMSExternalLinks.php +++ b/src/Controllers/CMSExternalLinksController.php @@ -12,13 +12,16 @@ use Symbiote\QueuedJobs\Services\QueuedJobService; class CMSExternalLinksController extends Controller { - private static $allowed_actions = array('getJobStatus', 'start'); + private static $allowed_actions = [ + 'getJobStatus', + 'start' + ]; /** - * Respond to Ajax requests for info on a running job - * - * @return string JSON string detailing status of the job - */ + * Respond to Ajax requests for info on a running job + * + * @return string JSON string detailing status of the job + */ public function getJobStatus() { // Set headers @@ -32,19 +35,19 @@ class CMSExternalLinksController extends Controller // Format status $track = BrokenExternalPageTrackStatus::get_latest(); if ($track) { - return json_encode(array( - 'TrackID' => $track->ID, - 'Status' => $track->Status, - 'Completed' => $track->getCompletedPages(), - 'Total' => $track->getTotalPages() - )); + return json_encode([ + 'TrackID' => $track->ID, + 'Status' => $track->Status, + 'Completed' => $track->getCompletedPages(), + 'Total' => $track->getTotalPages() + ]); } } /** - * Starts a broken external link check - */ + * Starts a broken external link check + */ public function start() { // return if the a job is already running diff --git a/src/Model/BrokenExternalLink.php b/src/Model/BrokenExternalLink.php index 5d39f09..0aa2037 100644 --- a/src/Model/BrokenExternalLink.php +++ b/src/Model/BrokenExternalLink.php @@ -69,16 +69,17 @@ class BrokenExternalLink extends DataObject public function getHTTPCodeDescription() { $code = $this->HTTPCode; - if (empty($code)) { + + try { + $response = HTTPResponse::create('', $code); // Assume that $code = 0 means there was no response - $description = _t(__CLASS__ . '.NOTAVAILABLE', 'Server Not Available'); - } elseif (($descriptions = Config::inst()->get(HTTPResponse::class, 'status_codes')) - && isset($descriptions[$code]) - ) { - $description = $descriptions[$code]; - } else { + $description = $code ? + $response->getStatusDescription() : + _t(__CLASS__ . '.NOTAVAILABLE', 'Server Not Available'); + } catch (InvalidArgumentException $e) { $description = _t(__CLASS__ . '.UNKNOWNRESPONSE', 'Unknown Response Code'); } + return sprintf("%d (%s)", $code, $description); } } diff --git a/src/Model/BrokenExternalPageTrackStatus.php b/src/Model/BrokenExternalPageTrackStatus.php index 0a159be..c34596e 100644 --- a/src/Model/BrokenExternalPageTrackStatus.php +++ b/src/Model/BrokenExternalPageTrackStatus.php @@ -111,10 +111,10 @@ class BrokenExternalPageTrackStatus extends DataObject } /** - * Create and prepare a new status - * - * @return BrokenExternalPageTrackStatus - */ + * Create and prepare a new status + * + * @return BrokenExternalPageTrackStatus + */ public static function create_status() { // If the script is to be started create a new status diff --git a/src/Reports/BrokenExternalLinksReport.php b/src/Reports/BrokenExternalLinksReport.php index 8b9ab90..fec065a 100644 --- a/src/Reports/BrokenExternalLinksReport.php +++ b/src/Reports/BrokenExternalLinksReport.php @@ -2,12 +2,13 @@ namespace SilverStripe\ExternalLinks\Reports; -use SilverStripe\Core\Convert; -use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrackStatus; use SilverStripe\ORM\ArrayList; -use SilverStripe\View\Requirements; +use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrackStatus; +use SilverStripe\Core\Convert; +use SilverStripe\View\HTML; use SilverStripe\Forms\LiteralField; use SilverStripe\Reports\Report; +use SilverStripe\View\Requirements; /** * Content side-report listing pages with external broken links @@ -72,7 +73,7 @@ class BrokenExternalLinksReport extends Report if ($track) { return $track->BrokenLinks(); } - return new ArrayList(); + return ArrayList::create(); } public function getCMSFields() @@ -81,17 +82,19 @@ class BrokenExternalLinksReport extends Report $fields = parent::getCMSFields(); $reportResultSpan = '

'; - $reportResult = new LiteralField('ResultTitle', $reportResultSpan); + $reportResult = LiteralField::create('ResultTitle', $reportResultSpan); $fields->push($reportResult); - $button = ''; - $runReportButton = new LiteralField( - 'runReport', - sprintf( - $button, - _t(__CLASS__ . '.RUNREPORT', 'Create new report') - ) + $button = HTML::createTag( + 'button', + [ + 'id' => 'externalLinksReport', + 'type' => 'button', + 'class' => 'btn btn-primary' + ], + _t(__CLASS__ . '.RUNREPORT', 'Create new report') ); + $runReportButton = LiteralField::create('runReport', $button); $fields->push($runReportButton); return $fields; diff --git a/src/Tasks/CheckExternalLinksTask.php b/src/Tasks/CheckExternalLinksTask.php index a588e86..41078a8 100644 --- a/src/Tasks/CheckExternalLinksTask.php +++ b/src/Tasks/CheckExternalLinksTask.php @@ -2,27 +2,23 @@ namespace SilverStripe\ExternalLinks\Tasks; -use DOMNode; - - - - - -use SilverStripe\Dev\Debug; -use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrack; use SilverStripe\ExternalLinks\Model\BrokenExternalLink; -use SilverStripe\Core\Config\Config; +use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrack; use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrackStatus; -use SilverStripe\Core\Injector\Injector; -use SilverStripe\ORM\DB; use SilverStripe\Dev\BuildTask; +use SilverStripe\Core\Config\Config; +use SilverStripe\ORM\DB; +use SilverStripe\Dev\Debug; +use DOMNode; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\ExternalLinks\Tasks\LinkChecker; class CheckExternalLinksTask extends BuildTask { - private static $dependencies = array( - 'LinkChecker' => '%$LinkChecker' - ); + private static $dependencies = [ + 'LinkChecker' => '%$' . LinkChecker::class + ]; /** * @var bool diff --git a/src/Tasks/CurlLinkChecker.php b/src/Tasks/CurlLinkChecker.php index 2e396a8..0a11f3d 100644 --- a/src/Tasks/CurlLinkChecker.php +++ b/src/Tasks/CurlLinkChecker.php @@ -3,6 +3,7 @@ namespace SilverStripe\ExternalLinks\Tasks; use Psr\SimpleCache\CacheInterface; +use SilverStripe\Core\Injector\Injector; /** * Check links using curl diff --git a/tests/ExternalLinksTest.php b/tests/ExternalLinksTest.php index d4de5e5..e1c4bbd 100644 --- a/tests/ExternalLinksTest.php +++ b/tests/ExternalLinksTest.php @@ -2,16 +2,16 @@ namespace SilverStripe\ExternalLinks\Tests; -use SilverStripe\ExternalLinks\Tasks\LinkChecker; -use SilverStripe\Core\Injector\Injector; -use SilverStripe\ExternalLinks\Tasks\CheckExternalLinksTask; -use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrackStatus; -use SilverStripe\i18n\i18n; -use SilverStripe\Reports\Report; use SilverStripe\ExternalLinks\Reports\BrokenExternalLinksReport; -use SilverStripe\Dev\SapphireTest; +use SilverStripe\ExternalLinks\Model\BrokenExternalPageTrackStatus; +use SilverStripe\ExternalLinks\Tasks\CheckExternalLinksTask; use SilverStripe\ExternalLinks\Tests\ExternalLinksTestPage; -use Phockito; +use SilverStripe\i18n\i18n; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\ExternalLinks\Tasks\LinkChecker; +use SilverStripe\ExternalLinks\Tests\Stubs\PretendLinkChecker; +use SilverStripe\Reports\Report; +use SilverStripe\Dev\SapphireTest; class ExternalLinksTest extends SapphireTest { @@ -22,67 +22,12 @@ class ExternalLinksTest extends SapphireTest ExternalLinksTestPage::class ); - public function setUpOnce() - { - if (class_exists(Phockito::class)) { - Phockito::include_hamcrest(false); - } - - parent::setUpOnce(); - } - protected function setUp() { parent::setUp(); - // Check dependencies - if (!class_exists(Phockito::class)) { - $this->skipTest = true; - return $this->markTestSkipped("These tests need the Phockito module installed to run"); - } - - // Mock link checker - $checker = Phockito::mock(LinkChecker::class); - Phockito::when($checker) - ->checkLink('http://www.working.com') - ->return(200); - - Phockito::when($checker) - ->checkLink('http://www.broken.com/url/thing') // 404 on working site - ->return(404); - - Phockito::when($checker) - ->checkLink('http://www.broken.com') // 403 on working site - ->return(403); - - Phockito::when($checker) - ->checkLink('http://www.nodomain.com') // no ping - ->return(0); - - Phockito::when($checker) - ->checkLink('/internal/link') - ->return(null); - - Phockito::when($checker) - ->checkLink('[sitetree_link,id=9999]') - ->return(null); - - Phockito::when($checker) - ->checkLink('home') - ->return(null); - - Phockito::when($checker) - ->checkLink('broken-internal') - ->return(null); - - Phockito::when($checker) - ->checkLink('[sitetree_link,id=1]') - ->return(null); - - Phockito::when($checker) - ->checkLink(Hamcrest_Matchers::anything()) // anything else is 404 - ->return(404); - + // Stub link checker + $checker = new PretendLinkChecker; Injector::inst()->registerService($checker, LinkChecker::class); } diff --git a/tests/Stubs/PretendLinkChecker.php b/tests/Stubs/PretendLinkChecker.php new file mode 100644 index 0000000..8701422 --- /dev/null +++ b/tests/Stubs/PretendLinkChecker.php @@ -0,0 +1,28 @@ +