Adjust tests, use timeFactory and add conditional logging

owncloud · Apr 8, 2019 · ca22476 · ca22476
1 parent b07bce8
commit ca22476
Show file tree

Hide file tree

Showing 4 changed files with 152 additions and 94 deletions.
diff --git a/core/Command/Background/Queue/Execute.php b/core/Command/Background/Queue/Execute.php
@@ -24,6 +24,7 @@
 use OC\BackgroundJob\TimedJob;
 use OC\Log\CommandLogger;
 use OCP\BackgroundJob\IJobList;
+use OCP\AppFramework\Utility\ITimeFactory;
 
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Helper\QuestionHelper;
@@ -39,13 +40,16 @@ class Execute extends Command {
 	 * @var IJobList
 	 */
 	protected $jobList;
+	/** @var ITimeFactory */
+	protected $timeFactory;
 
 	/**
 	 * @param IJobList $jobList
 	 */
-	public function __construct(IJobList $jobList) {
-		$this->jobList = $jobList;
+	public function __construct(IJobList $jobList, ITimeFactory $timeFactory) {
 		parent::__construct();
+		$this->jobList = $jobList;
+		$this->timeFactory = $timeFactory;
 	}
 
 	protected function configure() {
@@ -88,7 +92,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		$jobClass = \get_class($job);
 		$output->writeln("<info>Found job: $jobClass with ID $jobId</info>");
 
-		$start = \time();
+		$start = $this->timeFactory->getTime();
 
 		// Run the job if not reserved
 		$logger = new \OC\Log(new CommandLogger($output), \OC::$server->getSystemConfig());
@@ -103,7 +107,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 
 		$job->execute($this->jobList, $logger);
 
-		$duration = \time() - $start;
+		$duration = $this->timeFactory->getTime() - $start;
 
 		$output->writeln("<info>Finished in $duration seconds</info>");
 		$this->jobList->setLastJob($job);

diff --git a/core/register_command.php b/core/register_command.php
@@ -73,7 +73,7 @@
 	$application->add(new OC\Core\Command\Background\Ajax(\OC::$server->getConfig()));
 	$application->add(new OC\Core\Command\Background\Queue\Status(\OC::$server->getJobList()));
 	$application->add(new OC\Core\Command\Background\Queue\Delete(\OC::$server->getJobList()));
-	$application->add(new OC\Core\Command\Background\Queue\Execute(\OC::$server->getJobList()));
+	$application->add(new OC\Core\Command\Background\Queue\Execute(\OC::$server->getJobList(), \OC::$server->getTimeFactory()));
 
 	$application->add(new OC\Core\Command\Config\App\DeleteConfig(\OC::$server->getConfig()));
 	$application->add(new OC\Core\Command\Config\App\GetConfig(\OC::$server->getConfig()));

diff --git a/lib/private/BackgroundJob/Job.php b/lib/private/BackgroundJob/Job.php
@@ -47,39 +47,39 @@ abstract class Job implements IJob {
 	 * @param ILogger $logger
 	 */
 	public function execute($jobList, ILogger $logger = null) {
-		if ($logger === null) {
-			$logger = \OC::$server->getLogger();
-		}
-
 		$jobList->setLastRun($this);
 		try {
 			//storing job start time
 			$jobStartTime = \time();
 
-			$logger->debug(
-				'Started background job of class : {class} with arguments : {arguments}',
-				[
-					'app' => 'cron',
-					'class' => \get_class($this),
-					'arguments' => \print_r($this->argument, true)
-				]
-			);
+			if ($logger) {
+				$logger->debug(
+					'Started background job of class : {class} with arguments : {arguments}',
+					[
+						'app' => 'cron',
+						'class' => \get_class($this),
+						'arguments' => \print_r($this->argument, true)
+					]
+				);
+			}
 
 			$this->run($this->argument);
 
 			//storing job end time
 			$jobEndTime = \time();
 			$timeTaken = $jobEndTime - $jobStartTime;
 
-			$logger->debug(
-				'Finished background job, the job took : {timeTaken} seconds, this job is an instance of class : {class} with arguments : {arguments}',
-				[
-					'app' => 'cron',
-					'timeTaken' => $timeTaken,
-					'class' => \get_class($this),
-					'arguments' => \print_r($this->argument, true)
-				]
-			);
+			if ($logger) {
+				$logger->debug(
+					'Finished background job, the job took : {timeTaken} seconds, this job is an instance of class : {class} with arguments : {arguments}',
+					[
+						'app' => 'cron',
+						'timeTaken' => $timeTaken,
+						'class' => \get_class($this),
+						'arguments' => \print_r($this->argument, true)
+					]
+				);
+			}
 
 			$jobList->setExecutionTime($this, $timeTaken);
 		} catch (\Exception $e) {

diff --git a/tests/Core/Command/Background/Queue/ExecuteTest.php b/tests/Core/Command/Background/Queue/ExecuteTest.php
@@ -21,22 +21,14 @@
 
 namespace Tests\Core\Command\Background\Queue;
 
-use OC\BackgroundJob\Legacy\RegularJob;
 use OC\BackgroundJob\TimedJob;
+use OCP\BackgroundJob\IJob;
 use OC\Core\Command\Background\Queue\Execute;
 use OCP\BackgroundJob\IJobList;
+use OCP\AppFramework\Utility\ITimeFactory;
 use Symfony\Component\Console\Tester\CommandTester;
 use Test\TestCase;
 
-class TimedJobForExecuteTest extends TimedJob {
-	public function __construct() {
-		$this->setInterval(10);
-	}
-
-	public function run($argument) {
-	}
-}
-
 /**
  * Class ExecuteTest
  *
@@ -48,98 +40,160 @@ class ExecuteTest extends TestCase {
 	private $commandTester;
 	/** @var IJobList */
 	private $jobList;
-	/** @var string */
-	private $confirmPromptMessage = 'If you still want to use this command please confirm the usage by entering: yes';
+	/** @var ITimeFactory */
+	private $timeFactory;
+	/** @var IJob[] */
+	private $jobById = [];
 
 	public function setUp() {
 		parent::setUp();
 
+		$normalJob = $this->createMock(IJob::class);
+		$timedJob = $this->createMock(TimedJob::class);
+		$this->jobById = [];
+		$this->jobById[42] = $timedJob;
+		$this->jobById[48] = $normalJob;
+
+		$this->timeFactory = $this->createMock(ITimeFactory::class);
+
 		$this->jobList = $this->createMock(IJobList::class);
 		$this->jobList->expects($this->any())->method('getById')
 			->willReturnCallback(function ($id) {
-				if ($id === '666') {
-					return null;
-				}
-				if ($id === '42') {
-					$job = new TimedJobForExecuteTest();
+				if (isset($this->jobById[$id])) {
+					return $this->jobById[$id];
 				} else {
-					$job = new RegularJob();
+					return null;
 				}
-				$job->setId($id);
-				return $job;
 			});
 
-		$command = new Execute($this->jobList);
+		$command = new Execute($this->jobList, $this->timeFactory);
 		$this->commandTester = new CommandTester($command);
 	}
 
+
+	public function executeJobIdProvider() {
+		return [
+			[48],
+			[42],
+		];
+	}
+
+	public function testExecuteJobNotFoundWithAcceptWarning() {
+		$input = ['Job ID' => '666', '--accept-warning' => null];
+		foreach ($this->jobById as $id => $job) {
+			$job->expects($this->never())
+				->method('execute');
+		}
+
+		$this->assertSame(1, $this->commandTester->execute($input));
+	}
+
+	public function testExecuteJobNotFoundWithManualConfirmation() {
+		$this->commandTester->setInputs(['yes']);
+		$input = ['Job ID' => '666'];
+		foreach ($this->jobById as $id => $job) {
+			$job->expects($this->never())
+				->method('execute');
+		}
+
+		$this->assertSame(1, $this->commandTester->execute($input));
+	}
+
 	/**
-	 * @dataProvider providesJobIds
-	 * @param $jobId
-	 * @param $expectedOutputs
+	 * @dataProvider executeJobIdProvider
 	 */
-	public function testCommandWithAcceptWarningOption($jobId, $expectedOutputs) {
-		$input = ['Job ID' => $jobId, '--accept-warning' => null];
-		$this->commandTester->execute($input);
-		$output = $this->commandTester->getDisplay();
-		$this->assertNotContains($this->confirmPromptMessage, $output);
-		foreach ($expectedOutputs as $expectedOutput) {
-			$this->assertContains($expectedOutput, $output);
+	public function testExecuteJobWithAcceptWarning($jobId) {
+		$input = ['Job ID' => $jobId, '--accept-warning' => null];  // normal job
+		foreach ($this->jobById as $id => $job) {
+			if ($id === \intval($jobId)) {
+				// only the target job should be executed
+				$job->expects($this->once())
+					->method('execute');
+				$this->jobList->expects($this->once())
+					->method('setLastJob')
+					->with($job);
+				$this->jobList->expects($this->once())
+					->method('setExecutionTime')
+					->with($job, $this->anything());
+			} else {
+				$job->expects($this->never())
+					->method('execute');
+			}
 		}
+
+		$this->assertSame(0, $this->commandTester->execute($input));
 	}
 
 	/**
-	 * @dataProvider providesJobIds
-	 * @param $jobId
-	 * @param $expectedOutputs
+	 * @dataProvider executeJobIdProvider
 	 */
-	public function testCommandWithManualConfirmation($jobId, $expectedOutputs) {
+	public function testExecuteJobWithManualConfirmation($jobId) {
 		$this->commandTester->setInputs(['yes']);
-		$input = ['Job ID' => $jobId];
-		$this->commandTester->execute($input);
-		$output = $this->commandTester->getDisplay();
-		$this->assertContains($this->confirmPromptMessage, $output);
-		foreach ($expectedOutputs as $expectedOutput) {
-			$this->assertContains($expectedOutput, $output);
+		$input = ['Job ID' => $jobId];  // normal job
+		foreach ($this->jobById as $id => $job) {
+			if ($id === \intval($jobId)) {
+				// only the target job should be executed
+				$job->expects($this->once())
+					->method('execute');
+				$this->jobList->expects($this->once())
+					->method('setLastJob')
+					->with($job);
+				$this->jobList->expects($this->once())
+					->method('setExecutionTime')
+					->with($job, $this->anything());
+			} else {
+				$job->expects($this->never())
+					->method('execute');
+			}
 		}
-	}
 
-	public function providesJobIds() {
-		return [
-			['666', ['Job not found']],
-			['1', ['Found job: OC\BackgroundJob\Legacy\RegularJob with ID 1', 'Running job...', 'Finished in', 'seconds']],
-		];
+		$this->assertSame(0, $this->commandTester->execute($input));
 	}
 
-	public function testCommandWithManualAbort() {
+	/**
+	 * @dataProvider executeJobIdProvider
+	 */
+	public function testExecuteJobWithManualAbort($jobId) {
 		$this->commandTester->setInputs(['no']);
-		$input = ['Job ID' => 1];
-		$this->commandTester->execute($input);
-		$output = $this->commandTester->getDisplay();
-		$this->assertContains($this->confirmPromptMessage, $output);
-		$this->assertNotContains('Running job', $output);
+		$input = ['Job ID' => $jobId];
+		foreach ($this->jobById as $id => $job) {
+			$job->expects($this->never())
+				->method('execute');
+		}
+
+		$this->assertSame(1, $this->commandTester->execute($input));
 	}
 
 	/**
-	 * @dataProvider providesJobIdsForForceOption
-	 * @param $jobId
-	 * @param $expectedOutputs
+	 * @dataProvider executeJobIdProvider
 	 */
-	public function testCommandWithForceOption($jobId, $expectedOutputs) {
+	public function testExecuteJobWithForceOption($jobId) {
 		$this->commandTester->setInputs(['yes']);
 		$input = ['Job ID' => $jobId, '--force' => null];
-		$this->commandTester->execute($input);
-		$output = $this->commandTester->getDisplay();
-		$this->assertContains($this->confirmPromptMessage, $output);
-		foreach ($expectedOutputs as $expectedOutput) {
-			$this->assertContains($expectedOutput, $output);
+		foreach ($this->jobById as $id => $job) {
+			if ($id === \intval($jobId)) {
+				// only the target job should be executed
+				$job->expects($this->once())
+					->method('execute');
+				$this->jobList->expects($this->once())
+					->method('setLastJob')
+					->with($job);
+				$this->jobList->expects($this->once())
+					->method('setExecutionTime')
+					->with($job, $this->anything());
+
+				if ($job instanceof TimedJob) {
+					// if it's a timed job, reset the last run, otherwise just run it normally
+					$job->expects($this->once())
+						->method('setLastRun')
+						->with(0);
+				}
+			} else {
+				$job->expects($this->never())
+					->method('execute');
+			}
 		}
-	}
 
-	public function providesJobIdsForForceOption() {
-		return [
-			['666', ['Job not found']],
-			['42', ['Found job: Tests\Core\Command\Background\Queue\TimedJobForExecuteTest with ID 42', 'Forcing run, resetting last run value to 0', 'Running job...', 'Finished in', 'seconds']],
-		];
+		$this->assertSame(0, $this->commandTester->execute($input));
 	}
 }