From b4126da9cd00d8732ec071f8a89f6ab5b7910b55 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sun, 20 Sep 2026 22:44:56 +0200 Subject: [PATCH] fix(setupcheck): Print verbose info to allow finding out memory issues Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Joas Schilling --- core/Command/SetupChecks.php | 10 +- lib/private/SetupCheck/SetupCheckManager.php | 31 +++- lib/public/SetupCheck/ISetupCheckManager.php | 14 +- tests/Core/Command/SetupChecksTest.php | 146 ++++++++++++++++ .../lib/SetupCheck/SetupCheckManagerTest.php | 158 ++++++++++++++++++ 5 files changed, 345 insertions(+), 14 deletions(-) create mode 100644 tests/Core/Command/SetupChecksTest.php create mode 100644 tests/lib/SetupCheck/SetupCheckManagerTest.php diff --git a/core/Command/SetupChecks.php b/core/Command/SetupChecks.php index e655222d7d9ba..15aeceef2664d 100644 --- a/core/Command/SetupChecks.php +++ b/core/Command/SetupChecks.php @@ -9,10 +9,12 @@ namespace OC\Core\Command; +use OC\Migration\ConsoleOutput; use OCP\RichObjectStrings\IRichTextFormatter; use OCP\SetupCheck\ISetupCheckManager; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\ConsoleOutputInterface; use Symfony\Component\Console\Output\OutputInterface; class SetupChecks extends Base { @@ -60,12 +62,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int return self::FAILURE; } + $progressOutput = new ConsoleOutput($output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output); + if ($filterByCategory !== '') { - $results = $this->setupCheckManager->runByCategory($filterByCategory); + $results = $this->setupCheckManager->runByCategory($filterByCategory, $progressOutput); } elseif ($filterByClass !== '') { - $results = $this->setupCheckManager->runByClass($filterByClass); + $results = $this->setupCheckManager->runByClass($filterByClass, $progressOutput); } else { - $results = $this->setupCheckManager->runAll(); + $results = $this->setupCheckManager->runAll($progressOutput); } switch ($input->getOption('output')) { diff --git a/lib/private/SetupCheck/SetupCheckManager.php b/lib/private/SetupCheck/SetupCheckManager.php index 27fffa362bc41..adbbfb6629a54 100644 --- a/lib/private/SetupCheck/SetupCheckManager.php +++ b/lib/private/SetupCheck/SetupCheckManager.php @@ -10,10 +10,12 @@ namespace OC\SetupCheck; use OC\AppFramework\Bootstrap\Coordinator; +use OCP\Migration\IOutput; use OCP\Server; use OCP\SetupCheck\ISetupCheck; use OCP\SetupCheck\ISetupCheckManager; use OCP\SetupCheck\SetupResult; +use OCP\Util; use Psr\Log\LoggerInterface; class SetupCheckManager implements ISetupCheckManager { @@ -24,24 +26,24 @@ public function __construct( } #[\Override] - public function runByClass(string $filterByClass): array { + public function runByClass(string $filterByClass, ?IOutput $output = null): array { if (str_starts_with($filterByClass, '\\')) { $filterByClass = substr($filterByClass, 1); } - return $this->run(filterByClass: $filterByClass); + return $this->run(filterByClass: $filterByClass, output: $output); } #[\Override] - public function runByCategory(string $filterByCategory): array { - return $this->run(filterByCategory: $filterByCategory); + public function runByCategory(string $filterByCategory, ?IOutput $output = null): array { + return $this->run(filterByCategory: $filterByCategory, output: $output); } #[\Override] - public function runAll(): array { - return $this->run(); + public function runAll(?IOutput $output = null): array { + return $this->run(output: $output); } - private function run(?string $filterByCategory = null, ?string $filterByClass = null): array { + private function run(?string $filterByCategory = null, ?string $filterByClass = null, ?IOutput $output = null): array { $results = []; $setupChecks = $this->coordinator->getRegistrationContext()->getSetupChecks(); foreach ($setupChecks as $setupCheck) { @@ -55,13 +57,26 @@ private function run(?string $filterByCategory = null, ?string $filterByClass = continue; } - $this->logger->debug('Running check ' . get_class($setupCheckObject)); + $checkDetails = $setupCheckObject->getName() . ' (' . get_class($setupCheckObject) . ')'; + $message = 'Starting check ' . $checkDetails; + $output?->debug($message); + $this->logger->debug($message); + + memory_reset_peak_usage(); + $startTime = microtime(true); try { $setupResult = $setupCheckObject->run(); } catch (\Throwable $t) { $setupResult = SetupResult::error("An exception occurred while running the setup check:\n$t"); $this->logger->error('Exception running check ' . get_class($setupCheckObject) . ': ' . $t->getMessage(), ['exception' => $t]); } + $timeSpent = microtime(true) - $startTime; + $memoryPeak = memory_get_peak_usage(); + + $message = 'Check ' . $checkDetails . ' done in ' . number_format($timeSpent, 2) . ' seconds, peak memory usage: ' . Util::humanFileSize($memoryPeak); + $output?->debug($message); + $this->logger->debug($message); + $setupResult->setName($setupCheckObject->getName()); $category = $setupCheckObject->getCategory(); $results[$category] ??= []; diff --git a/lib/public/SetupCheck/ISetupCheckManager.php b/lib/public/SetupCheck/ISetupCheckManager.php index f0b18ea942e8b..bc88fa48d75ac 100644 --- a/lib/public/SetupCheck/ISetupCheckManager.php +++ b/lib/public/SetupCheck/ISetupCheckManager.php @@ -9,6 +9,8 @@ namespace OCP\SetupCheck; +use OCP\Migration\IOutput; + /** * @since 28.0.0 */ @@ -16,26 +18,32 @@ interface ISetupCheckManager { /** * Run all setup checks and return the results. * + * @param ?IOutput $output - Reports the check that is about to run as debug output, so a check that crashes or runs out of memory can be identified. * @since 28.0.0 + * @since 36.0.0 - parameter $output was added * @return array> Result of each check, first level key is category, second level key is title */ - public function runAll(): array; + public function runAll(?IOutput $output = null): array; /** * Run all tests from one specific category and return the results. * * @param string $filterByCategory - The id of the category to run. + * @param ?IOutput $output - Reports the check that is about to run as debug output, so a check that crashes or runs out of memory can be identified. * @return array> Result of each check, first level key is category, second level key is title * @since 35.0.0 + * @since 36.0.0 - parameter $output was added */ - public function runByCategory(string $filterByCategory): array; + public function runByCategory(string $filterByCategory, ?IOutput $output = null): array; /** * Run all tests from one specific class and return the results. * * @param string $filterByClass - The class to run. + * @param ?IOutput $output - Reports the check that is about to run as debug output, so a check that crashes or runs out of memory can be identified. * @return array> Result of each check, first level key is category, second level key is title * @since 35.0.0 + * @since 36.0.0 - parameter $output was added */ - public function runByClass(string $filterByClass): array; + public function runByClass(string $filterByClass, ?IOutput $output = null): array; } diff --git a/tests/Core/Command/SetupChecksTest.php b/tests/Core/Command/SetupChecksTest.php new file mode 100644 index 0000000000000..16e16f63a5480 --- /dev/null +++ b/tests/Core/Command/SetupChecksTest.php @@ -0,0 +1,146 @@ +setupCheckManager = $this->createMock(ISetupCheckManager::class); + $this->richTextFormatter = $this->createMock(IRichTextFormatter::class); + + $this->commandTester = new CommandTester( + new SetupChecks($this->setupCheckManager, $this->richTextFormatter) + ); + } + + /** + * Report one check through the output the command passes to the manager, and return its result. + */ + private function runOneCheck(?IOutput $output): array { + $this->assertInstanceOf(IOutput::class, $output); + $check = new SetupChecksTestCheck(); + $output->debug('Starting check ' . $check->getName() . ' (' . $check::class . ')'); + $output->debug('Check ' . $check->getName() . ' (' . $check::class . ') done in 0.01 seconds, peak memory usage: 8 MB'); + $result = SetupResult::success('Everything is fine'); + $result->setName($check->getName()); + return ['system' => [$check::class => $result]]; + } + + private function expectRunAll(): void { + $this->setupCheckManager->expects($this->once()) + ->method('runAll') + ->willReturnCallback($this->runOneCheck(...)); + } + + public function testProgressIsReportedOnVerboseOutput(): void { + $this->expectRunAll(); + + $this->assertSame(Command::SUCCESS, $this->commandTester->execute([], [ + 'verbosity' => OutputInterface::VERBOSITY_VERBOSE, + 'capture_stderr_separately' => true, + ])); + + $this->assertStringContainsString('Starting check Test check (' . SetupChecksTestCheck::class . ')', $this->commandTester->getErrorOutput()); + $this->assertStringContainsString('done in 0.01 seconds, peak memory usage: 8 MB', $this->commandTester->getErrorOutput()); + $this->assertStringContainsString('Everything is fine', $this->commandTester->getDisplay()); + $this->assertStringNotContainsString('Starting check Test check', $this->commandTester->getDisplay()); + } + + public function testProgressIsNotReportedOnNormalOutput(): void { + $this->expectRunAll(); + + $this->assertSame(Command::SUCCESS, $this->commandTester->execute([], [ + 'capture_stderr_separately' => true, + ])); + + $this->assertStringNotContainsString('Starting check Test check', $this->commandTester->getErrorOutput()); + $this->assertStringNotContainsString('Starting check Test check', $this->commandTester->getDisplay()); + } + + public function testProgressKeepsJsonOutputParsable(): void { + $this->expectRunAll(); + + $this->assertSame(Command::SUCCESS, $this->commandTester->execute(['--output' => 'json'], [ + 'verbosity' => OutputInterface::VERBOSITY_VERBOSE, + 'capture_stderr_separately' => true, + ])); + + $this->assertStringContainsString('Starting check Test check', $this->commandTester->getErrorOutput()); + $this->assertIsArray(json_decode($this->commandTester->getDisplay(), true, flags: JSON_THROW_ON_ERROR)); + } + + public function testFilterByCategory(): void { + $this->setupCheckManager->expects($this->once()) + ->method('runByCategory') + ->willReturnCallback(function (string $category, ?IOutput $output): array { + $this->assertSame('system', $category); + return $this->runOneCheck($output); + }); + + $this->assertSame(Command::SUCCESS, $this->commandTester->execute(['category' => 'system'])); + } + + public function testFilterByClass(): void { + $this->setupCheckManager->expects($this->once()) + ->method('runByClass') + ->willReturnCallback(function (string $class, ?IOutput $output): array { + $this->assertSame(SetupChecksTestCheck::class, $class); + return $this->runOneCheck($output); + }); + + $this->assertSame(Command::SUCCESS, $this->commandTester->execute(['class' => SetupChecksTestCheck::class])); + } + + public function testFilterByCategoryAndClassIsRejected(): void { + $this->setupCheckManager->expects($this->never()) + ->method($this->anything()); + + $this->assertSame(Command::FAILURE, $this->commandTester->execute([ + 'category' => 'system', + 'class' => SetupChecksTestCheck::class, + ])); + $this->assertStringContainsString('Please specify only one of category or class', $this->commandTester->getDisplay()); + } +} diff --git a/tests/lib/SetupCheck/SetupCheckManagerTest.php b/tests/lib/SetupCheck/SetupCheckManagerTest.php new file mode 100644 index 0000000000000..ed44a240b7db4 --- /dev/null +++ b/tests/lib/SetupCheck/SetupCheckManagerTest.php @@ -0,0 +1,158 @@ + */ + public static array $ran = []; + + #[\Override] + public function getCategory(): string { + return 'security'; + } + + #[\Override] + public function getName(): string { + return 'Security test check'; + } + + #[\Override] + public function run(): SetupResult { + self::$ran[] = static::class; + return SetupResult::success(); + } +} + +class SetupCheckManagerTestSystemCheck extends SetupCheckManagerTestSecurityCheck { + #[\Override] + public function getCategory(): string { + return 'system'; + } + + #[\Override] + public function getName(): string { + return 'System test check'; + } +} + +class SetupCheckManagerTest extends TestCase { + private Coordinator&MockObject $coordinator; + private LoggerInterface&MockObject $logger; + private SetupCheckManager $manager; + + #[\Override] + protected function setUp(): void { + parent::setUp(); + + SetupCheckManagerTestSecurityCheck::$ran = []; + + $registrationContext = $this->createMock(RegistrationContext::class); + $registrationContext->method('getSetupChecks') + ->willReturn([ + new ServiceRegistration('test', SetupCheckManagerTestSecurityCheck::class), + new ServiceRegistration('test', SetupCheckManagerTestSystemCheck::class), + ]); + + $this->coordinator = $this->createMock(Coordinator::class); + $this->coordinator->method('getRegistrationContext') + ->willReturn($registrationContext); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->manager = new SetupCheckManager($this->coordinator, $this->logger); + } + + /** + * Record the reported progress, together with the checks that already ran when it was reported. + * + * @param list}> $reported + */ + private function createOutput(array &$reported): IOutput&MockObject { + $output = $this->createMock(IOutput::class); + $output->method('debug') + ->willReturnCallback(function (string $message) use (&$reported): void { + $reported[] = [$message, SetupCheckManagerTestSecurityCheck::$ran]; + }); + return $output; + } + + public function testRunAllReportsEveryCheckBeforeAndAfterItIsRun(): void { + $reported = []; + + $results = $this->manager->runAll($this->createOutput($reported)); + + $security = preg_quote(SetupCheckManagerTestSecurityCheck::class, '/'); + $system = preg_quote(SetupCheckManagerTestSystemCheck::class, '/'); + + $this->assertCount(4, $reported); + // The check is reported before it gets a chance to run + $this->assertSame('Starting check Security test check (' . SetupCheckManagerTestSecurityCheck::class . ')', $reported[0][0]); + $this->assertSame([], $reported[0][1]); + $this->assertMatchesRegularExpression( + '/^Check Security test check \(' . $security . '\) done in \d+\.\d\d seconds, peak memory usage: /', + $reported[1][0] + ); + $this->assertSame([SetupCheckManagerTestSecurityCheck::class], $reported[1][1]); + $this->assertSame('Starting check System test check (' . SetupCheckManagerTestSystemCheck::class . ')', $reported[2][0]); + $this->assertSame([SetupCheckManagerTestSecurityCheck::class], $reported[2][1]); + $this->assertMatchesRegularExpression( + '/^Check System test check \(' . $system . '\) done in \d+\.\d\d seconds, peak memory usage: /', + $reported[3][0] + ); + + $this->assertEquals( + ['security' => [SetupCheckManagerTestSecurityCheck::class], 'system' => [SetupCheckManagerTestSystemCheck::class]], + array_map(array_keys(...), $results) + ); + } + + public function testRunByCategoryOnlyReportsMatchingChecks(): void { + $reported = []; + + $this->manager->runByCategory('system', $this->createOutput($reported)); + + $this->assertCount(2, $reported); + $this->assertSame('Starting check System test check (' . SetupCheckManagerTestSystemCheck::class . ')', $reported[0][0]); + $this->assertStringStartsWith('Check System test check (' . SetupCheckManagerTestSystemCheck::class . ') done in ', $reported[1][0]); + $this->assertEquals([SetupCheckManagerTestSystemCheck::class], SetupCheckManagerTestSecurityCheck::$ran); + } + + public function testRunByClassOnlyReportsMatchingChecks(): void { + $reported = []; + + $this->manager->runByClass(SetupCheckManagerTestSecurityCheck::class, $this->createOutput($reported)); + + $this->assertCount(2, $reported); + $this->assertStringContainsString(SetupCheckManagerTestSecurityCheck::class, $reported[0][0]); + $this->assertEquals([SetupCheckManagerTestSecurityCheck::class], SetupCheckManagerTestSecurityCheck::$ran); + } + + public function testRunByClassAcceptsALeadingBackslash(): void { + $results = $this->manager->runByClass('\\' . SetupCheckManagerTestSystemCheck::class); + + $this->assertEquals(['system' => [SetupCheckManagerTestSystemCheck::class]], array_map(array_keys(...), $results)); + } + + public function testRunWithoutOutput(): void { + $results = $this->manager->runAll(); + + $this->assertCount(2, $results); + } +}