cron: refactor and add unit tests

Move the job execution into manager so it can be tested

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;
     }

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());

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]);
+            }
+        }
     }
 }

tests/Cron/CronJob.php

+<?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;
+    }
+}

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));
+    }
 }