From f05c05fd0b1c55b058dcf9bc9fd10642541ec48e Mon Sep 17 00:00:00 2001
From: Christoph Reiter <reiter.christoph@gmail.com>
Date: Tue, 14 Feb 2023 15:15:58 +0100
Subject: [PATCH] cron: refactor and add unit tests
Move the job execution into manager so it can be tested
---
src/Cron/CronCommand.php | 16 +-------
src/Cron/CronListCommand.php | 2 +-
src/Cron/CronManager.php | 71 +++++++++++++++++++++------------
tests/Cron/CronJob.php | 48 ++++++++++++++++++++++
tests/Cron/CronTest.php | 77 +++++++++++++++++++++++++++++++++---
5 files changed, 168 insertions(+), 46 deletions(-)
create mode 100644 tests/Cron/CronJob.php
diff --git a/src/Cron/CronCommand.php b/src/Cron/CronCommand.php
index 521484d..9828c48 100644
--- a/src/Cron/CronCommand.php
+++ b/src/Cron/CronCommand.php
@@ -47,21 +47,7 @@ final class CronCommand extends Command implements LoggerAwareInterface
$command = $app->find('cache:pool:prune');
CachePrune::setPruneCommand($command);
- // Now run all jobs
- if ($force) {
- $jobsToRun = $this->manager->getAllJobs();
- } else {
- $jobsToRun = $this->manager->getDueJobs();
- }
- foreach ($jobsToRun as $job) {
- $name = $job->getName();
- $this->logger->info("cron: Running '$name'");
- try {
- $job->run(new CronOptions());
- } catch (\Throwable $e) {
- $this->logger->error("cron: '$name' failed", ['exception' => $e]);
- }
- }
+ $this->manager->runDueJobs($force);
return 0;
}
diff --git a/src/Cron/CronListCommand.php b/src/Cron/CronListCommand.php
index 5dfed2f..f9cb3b6 100644
--- a/src/Cron/CronListCommand.php
+++ b/src/Cron/CronListCommand.php
@@ -38,7 +38,7 @@ final class CronListCommand extends Command implements LoggerAwareInterface
protected function execute(InputInterface $input, OutputInterface $output): int
{
$currentTime = new \DateTimeImmutable('now', new \DateTimeZone('UTC'));
- $jobs = $this->manager->getAllJobs();
+ $jobs = $this->manager->getJobs();
foreach ($jobs as $job) {
$output->writeln('<fg=green;options=bold>['.get_class($job).']</>');
$output->writeln('<fg=blue;options=bold>Name:</> '.$job->getName());
diff --git a/src/Cron/CronManager.php b/src/Cron/CronManager.php
index 7d8bf69..0d937be 100644
--- a/src/Cron/CronManager.php
+++ b/src/Cron/CronManager.php
@@ -42,13 +42,11 @@ final class CronManager implements LoggerAwareInterface
* Returns if a job should run or not. Note that there is no feedback channel, so if you skip
* this run you will only be notified the next time the cron job should run.
*
- * @param string $cronExpression A cron expression
- *
* @return bool If the job should run
*/
- public static function isDue(?\DateTimeInterface $previousRun, \DateTimeInterface $currentRun, string $cronExpression): bool
+ public static function isDue(CronJobInterface $job, ?\DateTimeInterface $previousRun, \DateTimeInterface $currentRun): bool
{
- $cron = new CronExpression($cronExpression);
+ $cron = new CronExpression($job->getInterval());
$previousExpectedRun = $cron->getPreviousRunDate($currentRun, 0, true);
$previousExpectedRun->setTimezone(new \DateTimeZone('UTC'));
@@ -58,8 +56,9 @@ final class CronManager implements LoggerAwareInterface
// This can happen on re-deployments, and we don't want a cron-storm there, or jobs that run
// way off their schedule
$shouldRun = false;
- } elseif ($previousExpectedRun > $previousRun && $previousExpectedRun <= $currentRun) {
+ } elseif ($previousExpectedRun->getTimestamp() > $previousRun->getTimestamp() && $previousExpectedRun->getTimestamp() <= $currentRun->getTimestamp()) {
// If we were scheduled to run between now and right the previous run then we should run
+ // XXX: We compare the timestamps, since that is what we use to serialize the last execution time (so we get the same rounding)
$shouldRun = true;
}
@@ -79,59 +78,81 @@ final class CronManager implements LoggerAwareInterface
return \DateTimeImmutable::createFromMutable($nextDate);
}
- public function getPreviousRun(\DateTimeInterface $currentTime): ?\DateTimeInterface
+ /**
+ * Returns the last time cron was executed.
+ */
+ public function getLastExecutionDate(): ?\DateTimeInterface
{
$cachePool = $this->cachePool;
- // Store the previous run time in the cache and fetch from there
assert($cachePool instanceof CacheItemPoolInterface);
$item = $cachePool->getItem('cron-previous-run');
$value = $item->get();
$previousRun = null;
if ($value !== null) {
$previousRun = (new \DateTimeImmutable())->setTimezone(new \DateTimeZone('UTC'))->setTimestamp($value);
- if ($previousRun > $currentTime) {
- // Something is wrong, cap at the current time
- $previousRun = $currentTime;
- }
}
+
+ return $previousRun;
+ }
+
+ /**
+ * Stores the given time as the new last cron execution time.
+ */
+ public function setLastExecutionDate(\DateTimeInterface $currentTime): void
+ {
+ $cachePool = $this->cachePool;
+ assert($cachePool instanceof CacheItemPoolInterface);
+ $item = $cachePool->getItem('cron-previous-run');
$item->set($currentTime->getTimestamp());
if ($cachePool->save($item) === false) {
throw new \RuntimeException('Saving cron timestamp failed');
}
-
- return $previousRun;
}
/**
* @return CronJobInterface[]
*/
- public function getAllJobs(): array
+ public function getJobs(): array
{
return $this->jobs;
}
- /**
- * @return CronJobInterface[]
- */
- public function getDueJobs(): array
+ public function runDueJobs(bool $force = false, \DateTimeInterface $currentTime = null)
{
// Get all jobs that should have been run between the last time we were called and now
- $currentTime = new \DateTimeImmutable('now', new \DateTimeZone('UTC'));
- // round to full seconds, so we have the same resolution for both date times
- $currentTime = $currentTime->setTimestamp($currentTime->getTimestamp());
- $previousRunTime = $this->getPreviousRun($currentTime);
+ if ($currentTime === null) {
+ $currentTime = new \DateTimeImmutable('now', new \DateTimeZone('UTC'));
+ }
+ $lastDate = $this->getLastExecutionDate();
+ $this->setLastExecutionDate($currentTime);
+
+ if ($lastDate === null) {
+ $this->logger->info('cron: No last execution time available, will no run anything');
+ }
$toRun = [];
foreach ($this->jobs as $job) {
$interval = $job->getInterval();
$name = $job->getName();
$this->logger->info("cron: Checking '$name' ($interval)");
- $isDue = self::isDue($previousRunTime, $currentTime, $interval);
- if ($isDue) {
+ $isDue = self::isDue($job, $lastDate, $currentTime);
+ if ($isDue || $force) {
$toRun[] = $job;
}
}
- return $toRun;
+ if (count($toRun) === 0) {
+ $this->logger->info('cron: No jobs to run');
+ }
+
+ foreach ($toRun as $job) {
+ $name = $job->getName();
+ $this->logger->info("cron: Running '$name'");
+ try {
+ $job->run(new CronOptions());
+ } catch (\Throwable $e) {
+ $this->logger->error("cron: '$name' failed", ['exception' => $e]);
+ }
+ }
}
}
diff --git a/tests/Cron/CronJob.php b/tests/Cron/CronJob.php
new file mode 100644
index 0000000..6350caf
--- /dev/null
+++ b/tests/Cron/CronJob.php
@@ -0,0 +1,48 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Dbp\Relay\CoreBundle\Tests\Cron;
+
+use Dbp\Relay\CoreBundle\Cron\CronJobInterface;
+use Dbp\Relay\CoreBundle\Cron\CronOptions;
+
+class CronJob implements CronJobInterface
+{
+ /**
+ * @var string
+ */
+ private $name;
+
+ /**
+ * @var string
+ */
+ private $interval;
+
+ /**
+ * @var bool
+ */
+ public $ran;
+
+ public function __construct(string $interval, string $name = '')
+ {
+ $this->name = $name;
+ $this->interval = $interval;
+ $this->ran = false;
+ }
+
+ public function getName(): string
+ {
+ return $this->name;
+ }
+
+ public function getInterval(): string
+ {
+ return $this->interval;
+ }
+
+ public function run(CronOptions $options): void
+ {
+ $this->ran = true;
+ }
+}
diff --git a/tests/Cron/CronTest.php b/tests/Cron/CronTest.php
index 7c951cf..a648238 100644
--- a/tests/Cron/CronTest.php
+++ b/tests/Cron/CronTest.php
@@ -4,20 +4,87 @@ declare(strict_types=1);
namespace Dbp\Relay\CoreBundle\Tests\Cron;
+use Dbp\Relay\CoreBundle\Cron\CronCommand;
+use Dbp\Relay\CoreBundle\Cron\CronListCommand;
use Dbp\Relay\CoreBundle\Cron\CronManager;
use PHPUnit\Framework\TestCase;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\Console\Input\ArrayInput;
+use Symfony\Component\Console\Output\BufferedOutput;
class CronTest extends TestCase
{
- public function testCronisDue()
+ public function testCronIsDue()
{
- $isDue = CronManager::isDue(new \DateTimeImmutable('2021-09-07T09:36:26Z'), new \DateTimeImmutable('2021-09-07T09:36:26Z'), '* * * * *');
+ $isDue = CronManager::isDue(new CronJob('* * * * *'), new \DateTimeImmutable('2021-09-07T09:36:26Z'), new \DateTimeImmutable('2021-09-07T09:36:26Z'));
$this->assertFalse($isDue);
- $isDue = CronManager::isDue(new \DateTimeImmutable('2021-09-07T09:35:59Z'), new \DateTimeImmutable('2021-09-07T09:36:00Z'), '* * * * *');
+ $isDue = CronManager::isDue(new CronJob('* * * * *'), new \DateTimeImmutable('2021-09-07T09:35:59Z'), new \DateTimeImmutable('2021-09-07T09:36:00Z'));
$this->assertTrue($isDue);
- $isDue = CronManager::isDue(null, new \DateTimeImmutable('2021-09-07T09:36:00Z'), '0 0 1 1 *');
+ $isDue = CronManager::isDue(new CronJob('0 0 1 1 *'), null, new \DateTimeImmutable('2021-09-07T09:36:00Z'));
$this->assertFalse($isDue);
- $isDue = CronManager::isDue(null, new \DateTimeImmutable('2021-09-07T09:36:00Z'), '* * * * *');
+ $isDue = CronManager::isDue(new CronJob('* * * * *'), null, new \DateTimeImmutable('2021-09-07T09:36:00Z'));
$this->assertFalse($isDue);
}
+
+ public function testExecutionDate()
+ {
+ $man = new CronManager();
+ $man->setCache(new ArrayAdapter());
+ $this->assertNull($man->getLastExecutionDate());
+ $date = (new \DateTimeImmutable())->setTimestamp(42);
+ $man->setLastExecutionDate($date);
+ $this->assertSame(42, $man->getLastExecutionDate()->getTimestamp());
+ }
+
+ public function testGetNextDate()
+ {
+ $current = (new \DateTimeImmutable())->setTimestamp(1676383110);
+ $next = CronManager::getNextDate(new CronJob('*/5 * * * *'), $current);
+ $this->assertSame(1676383200, $next->getTimestamp());
+ }
+
+ public function testManager()
+ {
+ $last = (new \DateTimeImmutable())->setTimestamp(1676383110);
+ $current = $last->setTimestamp($last->getTimestamp() + 30);
+ $man = new CronManager();
+ $man->setCache(new ArrayAdapter());
+ $man->setLastExecutionDate($last);
+ $job = new CronJob('* * * * *');
+
+ // add a job
+ $man->addJob($job);
+ $this->assertSame($job, $man->getJobs()[0]);
+
+ // run right away
+ $man->runDueJobs(false, $last);
+ $this->assertFalse($job->ran);
+
+ // now a bit later, the job gets run
+ $man->runDueJobs(false, $current);
+ $this->assertTrue($job->ran);
+
+ // now right away again, but force
+ $job->ran = false;
+ $man->runDueJobs(false, $current);
+ $this->assertFalse($job->ran);
+ $man->runDueJobs(true, $current);
+ $this->assertTrue($job->ran);
+ }
+
+ public function testCommands()
+ {
+ $man = new CronManager();
+ $job = new CronJob('* * * * *');
+ $man->addJob($job);
+
+ // cron command
+ new CronCommand($man);
+
+ // list command
+ $input = new ArrayInput([]);
+ $output = new BufferedOutput();
+ $cmd = new CronListCommand($man);
+ $this->assertSame(0, $cmd->run($input, $output));
+ }
}
--
GitLab