magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Let background commands actually run in the background

Open fredden opened this issue 1 year ago • 94 comments

Update 2023-05-30

After discussion with @engcom-Charlie (thanks!), I set up the Magento Functional Testing Framework locally to investigate the specifics of this test. The report was that the test worked fine on the 2.4-develop branch, but was failing on #32690, so it was suggested that the changes there were breaking this test, and that the test was fine as-is on 2.4-develop. I thought this was a race condition, but it seems it's more complicated than a simple race condition.

The reason the test isn't failing on 2.4-develop is because STDERR isn't being redirected for the child processes, so the testing framework wasn't actually generating background processes when it should have been. https://github.com/magento/magento2/pull/37511/commits/7d56fe499617d1ad03c77ecace4dfd7c60a4c3dd fixes that bug, which will mean that all other unreliable tests will now be failing here. Many of the test failures here will be fixed by https://github.com/magento/magento2-functional-testing-framework/pull/904

Description

While working on https://github.com/magento/magento2/pull/32690#discussion_r1196637387, it was discovered that the test StorefrontVerifyProductAfterPartialReindexOnSeveralWebsitesTest (from Magento\AcceptanceTest\_default\Backend\StorefrontVerifyProductAfterPartialReindexOnSeveralWebsitesTest) is unreliable. This pull request aims to improve the reliability of this test. While this is not a complete fix, it will improve the reliability.

Why is the test unreliable? The test aims to ensure that indexers are correctly triggered / rebuilt when making changes in the admin. The general process goes: log into admin, make a change, run cron, witness result. It's the "run cron" step that is causing issues. The internals of that step spawns separate processes, and then returns. These separate processes will run in the background and will work, however there is a race condition between these background processes running to completion and the next step of the test being carried out. When the indexer process is quick, all is well. When the test framework gets to the next step before the background process complete, the test fails.

How does this change help?

This pull request turns the background process into a foreground process, so the test framework won't ever reach the next step until the "background" process finishes.

Why is this change incomplete?

There is no guarantee that a job has actually been scheduled. There is still an expectation that there is a indexer_update_all_views job in the queue, ready to be executed. Usually this will be the case, however it's not guaranteed. Further test improvements are welcome in this area.

Manual testing scenarios

Run the following PHP script. It should report success.

#!/usr/bin/env php
<?php

use Magento\Framework\App\ObjectManager;
use Magento\Framework\Console\Cli;
use Magento\Framework\Shell\CommandRendererBackground;

$command = 'sleep 10';

require __DIR__ . '/app/bootstrap.php';
new Cli('Magento CLI');
$objectManager = ObjectManager::getInstance();
$commandRenderer = $objectManager->get(CommandRendererBackground::class);

$command = $commandRenderer->render($command);
$startTime = microtime(true);

echo "DEBUG: running `$command` ...\n";
$process = proc_open($command, [
    0 => ['pipe', 'r'],
    1 => ['pipe', 'w'],
    2 => ['pipe', 'w'],
], $pipes, getcwd());
fclose($pipes[0]); // STDIN
echo stream_get_contents($pipes[1]); // STDOUT
echo stream_get_contents($pipes[2]); // STDERR
proc_close($process);

$endTime = microtime(true);
$duration = $endTime - $startTime;
$result = $duration >= 5 ? 'fail' : 'pass';
printf('Test %sed. It took %.1f seconds to start a long-running background process.', $result, $duration);
echo "\n";

Contribution checklist

  • [x] Pull request has a meaningful description of its purpose
  • [x] All commits are accompanied by meaningful commit messages
  • [x] All new or changed code is covered with unit/integration tests (if applicable)
  • [x] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • [x] All automated tests passed successfully (all builds are green)

fredden avatar May 17 '23 15:05 fredden