Merge pull request #613 from tienvx/prevent-updating-bug-steps-after-…

…running Prevent updating bug's steps after running
tienvx · Apr 23, 2022 · 329b312 · 329b312
2 parents be28c20 + acc68e4
commit 329b312
Show file tree

Hide file tree

Showing 14 changed files with 172 additions and 111 deletions.
diff --git a/src/Reducer/HandlerTemplate.php b/src/Reducer/HandlerTemplate.php
@@ -9,24 +9,28 @@
 use Tienvx\Bundle\MbtBundle\Repository\BugRepositoryInterface;
 use Tienvx\Bundle\MbtBundle\Service\Step\Builder\StepsBuilderInterface;
 use Tienvx\Bundle\MbtBundle\Service\Step\Runner\BugStepsRunner;
+use Tienvx\Bundle\MbtBundle\Service\Step\StepHelperInterface;
 
 abstract class HandlerTemplate implements HandlerInterface
 {
     protected BugRepositoryInterface $bugRepository;
     protected MessageBusInterface $messageBus;
     protected BugStepsRunner $stepsRunner;
     protected StepsBuilderInterface $stepsBuilder;
+    protected StepHelperInterface $stepHelper;
 
     public function __construct(
         BugRepositoryInterface $bugRepository,
         MessageBusInterface $messageBus,
         BugStepsRunner $stepsRunner,
-        StepsBuilderInterface $stepsBuilder
+        StepsBuilderInterface $stepsBuilder,
+        StepHelperInterface $stepHelper
     ) {
         $this->bugRepository = $bugRepository;
         $this->messageBus = $messageBus;
         $this->stepsRunner = $stepsRunner;
         $this->stepsBuilder = $stepsBuilder;
+        $this->stepHelper = $stepHelper;
     }
 
     public function handle(BugInterface $bug, int $from, int $to): void
@@ -37,11 +41,15 @@ public function handle(BugInterface $bug, int $from, int $to): void
         }
 
         $bug->setDebug(false);
-        $this->stepsRunner->run($newSteps, $bug, function (Throwable $throwable) use ($bug, $newSteps): void {
-            if ($throwable->getMessage() === $bug->getMessage()) {
-                $this->bugRepository->updateSteps($bug, $newSteps);
-                $this->messageBus->dispatch(new ReduceBugMessage($bug->getId()));
+        $this->stepsRunner->run(
+            $this->stepHelper->cloneStepsAndResetColor($newSteps),
+            $bug,
+            function (Throwable $throwable) use ($bug, $newSteps): void {
+                if ($throwable->getMessage() === $bug->getMessage()) {
+                    $this->bugRepository->updateSteps($bug, $newSteps);
+                    $this->messageBus->dispatch(new ReduceBugMessage($bug->getId()));
+                }
             }
-        });
+        );
     }
 }
diff --git a/src/Repository/BugRepository.php b/src/Repository/BugRepository.php
@@ -21,7 +21,7 @@ public function updateSteps(BugInterface $bug, array $newSteps): void
             // Refresh the bug for the latest steps's length.
             $this->getEntityManager()->refresh($bug);
 
-            if (count($newSteps) <= count($bug->getSteps())) {
+            if (count($newSteps) < count($bug->getSteps())) {
                 $this->getEntityManager()->lock($bug, LockMode::PESSIMISTIC_WRITE);
                 $bug->getProgress()->setTotal(0);
                 $bug->getProgress()->setProcessed(0);
@@ -68,14 +68,4 @@ public function stopRecording(BugInterface $bug): void
         $bug->getVideo()->setRecording(false);
         $this->getEntityManager()->flush();
     }
-
-    public function updateVideoErrorMessage(BugInterface $bug, ?string $errorMessage): void
-    {
-        // Recording bug may take long time. Reconnect to flush changes.
-        $this->getEntityManager()->getConnection()->connect();
-        // Refresh so we don't update other fields while recording.
-        $this->getEntityManager()->refresh($bug);
-        $bug->getVideo()->setErrorMessage($errorMessage);
-        $this->getEntityManager()->flush();
-    }
 }
diff --git a/src/Repository/BugRepositoryInterface.php b/src/Repository/BugRepositoryInterface.php
@@ -16,6 +16,4 @@ public function increaseTotal(BugInterface $bug, int $total): void;
     public function startRecording(BugInterface $bug): void;
 
     public function stopRecording(BugInterface $bug): void;
-
-    public function updateVideoErrorMessage(BugInterface $bug, ?string $errorMessage): void;
 }
diff --git a/src/Resources/config/services.php b/src/Resources/config/services.php
@@ -68,6 +68,8 @@
 use Tienvx\Bundle\MbtBundle\Service\Step\Runner\StepRunner;
 use Tienvx\Bundle\MbtBundle\Service\Step\Runner\StepRunnerInterface;
 use Tienvx\Bundle\MbtBundle\Service\Step\Runner\TaskStepsRunner;
+use Tienvx\Bundle\MbtBundle\Service\Step\StepHelper;
+use Tienvx\Bundle\MbtBundle\Service\Step\StepHelperInterface;
 use Tienvx\Bundle\MbtBundle\Service\Task\TaskHelper;
 use Tienvx\Bundle\MbtBundle\Service\Task\TaskHelperInterface;
 use Tienvx\Bundle\MbtBundle\Validator\TagsValidator;
@@ -136,6 +138,7 @@
                 service(MessageBusInterface::class),
                 service(BugStepsRunner::class),
                 service(StepsBuilderInterface::class),
+                service(StepHelperInterface::class),
             ])
         ->set(RandomReducer::class)
             ->args([
@@ -153,6 +156,7 @@
                 service(MessageBusInterface::class),
                 service(BugStepsRunner::class),
                 service(StepsBuilderInterface::class),
+                service(StepHelperInterface::class),
             ])
         ->set(SplitReducer::class)
             ->args([
@@ -230,6 +234,7 @@
                 service(BugRepositoryInterface::class),
                 service(MessageBusInterface::class),
                 service(BugNotifierInterface::class),
+                service(StepHelperInterface::class),
                 service(BugStepsRunner::class),
                 service(ConfigInterface::class),
             ])
@@ -258,6 +263,9 @@
             ])
             ->alias(PetrinetDomainLogicInterface::class, PetrinetDomainLogic::class)
 
+        ->set(StepHelperInterface::class)
+            ->alias(StepHelperInterface::class, StepHelper::class)
+
         ->set(StepRunner::class)
             ->args([
                 service(CommandRunnerManagerInterface::class),

diff --git a/src/Service/Bug/BugHelper.php b/src/Service/Bug/BugHelper.php
@@ -14,13 +14,15 @@
 use Tienvx\Bundle\MbtBundle\Repository\BugRepositoryInterface;
 use Tienvx\Bundle\MbtBundle\Service\ConfigInterface;
 use Tienvx\Bundle\MbtBundle\Service\Step\Runner\BugStepsRunner;
+use Tienvx\Bundle\MbtBundle\Service\Step\StepHelperInterface;
 
 class BugHelper implements BugHelperInterface
 {
     protected ReducerManagerInterface $reducerManager;
     protected BugRepositoryInterface $bugRepository;
     protected MessageBusInterface $messageBus;
     protected BugNotifierInterface $bugNotifier;
+    protected StepHelperInterface $stepHelper;
     protected BugStepsRunner $stepsRunner;
     protected ConfigInterface $config;
 
@@ -29,13 +31,15 @@ public function __construct(
         BugRepositoryInterface $bugRepository,
         MessageBusInterface $messageBus,
         BugNotifierInterface $bugNotifier,
+        StepHelperInterface $stepHelper,
         BugStepsRunner $stepsRunner,
         ConfigInterface $config
     ) {
         $this->reducerManager = $reducerManager;
         $this->bugRepository = $bugRepository;
         $this->messageBus = $messageBus;
         $this->bugNotifier = $bugNotifier;
+        $this->stepHelper = $stepHelper;
         $this->stepsRunner = $stepsRunner;
         $this->config = $config;
     }
@@ -95,12 +99,15 @@ public function recordVideo(int $bugId): void
 
         $this->bugRepository->startRecording($bug);
         $bug->setDebug(true);
-        $this->stepsRunner->run($bug->getSteps(), $bug, function (Throwable $throwable) use ($bug) {
-            $this->bugRepository->updateVideoErrorMessage(
-                $bug,
-                $throwable->getMessage() !== $bug->getMessage() ? $throwable->getMessage() : null
-            );
-        });
+        $this->stepsRunner->run(
+            $this->stepHelper->cloneStepsAndResetColor($bug->getSteps()),
+            $bug,
+            function (Throwable $throwable) use ($bug) {
+                $bug->getVideo()->setErrorMessage(
+                    $throwable->getMessage() !== $bug->getMessage() ? $throwable->getMessage() : null
+                );
+            }
+        );
         $this->bugRepository->stopRecording($bug);
     }
 

diff --git a/src/Service/Step/Runner/BugStepsRunner.php b/src/Service/Step/Runner/BugStepsRunner.php
@@ -2,39 +2,11 @@
 
 namespace Tienvx\Bundle\MbtBundle\Service\Step\Runner;
 
-use Facebook\WebDriver\Remote\RemoteWebDriver;
-use SingleColorPetrinet\Model\Color;
-use SingleColorPetrinet\Model\ColorInterface;
 use Throwable;
 use Tienvx\Bundle\MbtBundle\Model\Bug\StepInterface;
-use Tienvx\Bundle\MbtBundle\Model\DebugInterface;
-use Tienvx\Bundle\MbtBundle\Model\Model\RevisionInterface;
 
 class BugStepsRunner extends StepsRunner
 {
-    protected ?ColorInterface $lastColor;
-
-    protected function start(DebugInterface $entity): RemoteWebDriver
-    {
-        $this->lastColor = new Color();
-
-        return parent::start($entity);
-    }
-
-    protected function stop(?RemoteWebDriver $driver): void
-    {
-        parent::stop($driver);
-        $this->lastColor = null;
-    }
-
-    protected function runStep(StepInterface $step, RevisionInterface $revision, RemoteWebDriver $driver): void
-    {
-        $color = clone $step->getColor();
-        $step->setColor($this->lastColor);
-        parent::runStep($step, $revision, $driver);
-        $this->lastColor = $color;
-    }
-
     protected function catchException(callable $handleException, Throwable $throwable, ?StepInterface $step): void
     {
         $handleException($throwable);

diff --git a/src/Service/Step/StepHelper.php b/src/Service/Step/StepHelper.php
@@ -0,0 +1,27 @@
+<?php
+
+namespace Tienvx\Bundle\MbtBundle\Service\Step;
+
+use SingleColorPetrinet\Model\Color;
+use Tienvx\Bundle\MbtBundle\Exception\UnexpectedValueException;
+use Tienvx\Bundle\MbtBundle\Model\Bug\StepInterface;
+
+class StepHelper implements StepHelperInterface
+{
+    public function cloneStepsAndResetColor(array $steps): array
+    {
+        $lastColor = null;
+        $newSteps = [];
+        foreach ($steps as $step) {
+            if (!$step instanceof StepInterface) {
+                throw new UnexpectedValueException(sprintf('Step must be instance of "%s".', StepInterface::class));
+            }
+            $newStep = clone $step;
+            $newStep->setColor($lastColor ?? new Color());
+            $newSteps[] = $newStep;
+            $lastColor = clone $step->getColor();
+        }
+
+        return $newSteps;
+    }
+}
diff --git a/src/Service/Step/StepHelperInterface.php b/src/Service/Step/StepHelperInterface.php
@@ -0,0 +1,8 @@
+<?php
+
+namespace Tienvx\Bundle\MbtBundle\Service\Step;
+
+interface StepHelperInterface
+{
+    public function cloneStepsAndResetColor(array $steps): array;
+}
diff --git a/tests/Reducer/HandlerTestCase.php b/tests/Reducer/HandlerTestCase.php
@@ -16,6 +16,7 @@
 use Tienvx\Bundle\MbtBundle\Repository\BugRepositoryInterface;
 use Tienvx\Bundle\MbtBundle\Service\Step\Builder\StepsBuilderInterface;
 use Tienvx\Bundle\MbtBundle\Service\Step\Runner\BugStepsRunner;
+use Tienvx\Bundle\MbtBundle\Service\Step\StepHelperInterface;
 
 abstract class HandlerTestCase extends TestCase
 {
@@ -24,6 +25,7 @@ abstract class HandlerTestCase extends TestCase
     protected MessageBusInterface $messageBus;
     protected BugStepsRunner $stepsRunner;
     protected StepsBuilderInterface $stepsBuilder;
+    protected StepHelperInterface $stepHelper;
     protected array $newSteps;
     protected BugInterface $bug;
 
@@ -33,6 +35,7 @@ protected function setUp(): void
         $this->messageBus = $this->createMock(MessageBusInterface::class);
         $this->stepsRunner = $this->createMock(BugStepsRunner::class);
         $this->stepsBuilder = $this->createMock(StepsBuilderInterface::class);
+        $this->stepHelper = $this->createMock(StepHelperInterface::class);
         $this->newSteps = [
             $this->createMock(StepInterface::class),
             $this->createMock(StepInterface::class),
@@ -73,6 +76,11 @@ public function testHandleOldBug(): void
      */
     public function testHandle(?Throwable $exception, bool $updateSteps): void
     {
+        $this->stepHelper
+            ->expects($this->once())
+            ->method('cloneStepsAndResetColor')
+            ->with($this->newSteps)
+            ->willReturnArgument(0);
         $this->stepsRunner->expects($this->once())
             ->method('run')
             ->with(

diff --git a/tests/Reducer/Random/RandomHandlerTest.php b/tests/Reducer/Random/RandomHandlerTest.php
@@ -27,7 +27,8 @@ protected function setUp(): void
             $this->bugRepository,
             $this->messageBus,
             $this->stepsRunner,
-            $this->stepsBuilder
+            $this->stepsBuilder,
+            $this->stepHelper
         );
     }
 }
diff --git a/tests/Reducer/Split/SplitHandlerTest.php b/tests/Reducer/Split/SplitHandlerTest.php
@@ -27,7 +27,8 @@ protected function setUp(): void
             $this->bugRepository,
             $this->messageBus,
             $this->stepsRunner,
-            $this->stepsBuilder
+            $this->stepsBuilder,
+            $this->stepHelper
         );
     }
 }
diff --git a/tests/Repository/BugRepositoryTest.php b/tests/Repository/BugRepositoryTest.php
@@ -58,16 +58,17 @@ protected function setUp(): void
         $this->connection = $this->createMock(Connection::class);
     }
 
-    public function testUpdateSteps(): void
+    /**
+     * @dataProvider stepsProvider
+     */
+    public function testUpdateSteps(int $length, int $expectedLength, int $expectedProcessed, int $expectedTotal): void
     {
-        $newSteps = [
-            $this->createMock(StepInterface::class),
-            $this->createMock(StepInterface::class),
-            $this->createMock(StepInterface::class),
-            $this->createMock(StepInterface::class),
-        ];
+        $newSteps = array_map(fn () => $this->createMock(StepInterface::class), range(0, $length - 1));
         $this->manager->expects($this->once())->method('refresh')->with($this->bug);
-        $this->manager->expects($this->never())->method('lock');
+        $this->manager
+            ->expects($this->exactly(count($this->bug->getSteps()) !== $expectedLength))
+            ->method('lock')
+            ->with($this->bug, LockMode::PESSIMISTIC_WRITE);
         $this->manager
             ->expects($this->once())
             ->method('wrapInTransaction')
@@ -77,31 +78,18 @@ public function testUpdateSteps(): void
                 return true;
             }));
         $this->bugRepository->updateSteps($this->bug, $newSteps);
-        $this->assertNotSame($newSteps, $this->bug->getSteps());
-        $this->assertSame(5, $this->bug->getProgress()->getProcessed());
-        $this->assertSame(10, $this->bug->getProgress()->getTotal());
+        $this->assertCount($expectedLength, $this->bug->getSteps());
+        $this->assertSame($expectedProcessed, $this->bug->getProgress()->getProcessed());
+        $this->assertSame($expectedTotal, $this->bug->getProgress()->getTotal());
     }
 
-    public function testUpdateStepsWithShorterSteps(): void
+    public function stepsProvider(): array
     {
-        $newSteps = [
-            $this->createMock(StepInterface::class),
-            $this->createMock(StepInterface::class),
+        return [
+            [4, 3, 5, 10],
+            [3, 3, 5, 10],
+            [2, 2, 0, 0],
         ];
-        $this->manager->expects($this->once())->method('refresh')->with($this->bug);
-        $this->manager->expects($this->once())->method('lock')->with($this->bug, LockMode::PESSIMISTIC_WRITE);
-        $this->manager
-            ->expects($this->once())
-            ->method('wrapInTransaction')
-            ->with($this->callback(function ($callback) {
-                $callback();
-
-                return true;
-            }));
-        $this->bugRepository->updateSteps($this->bug, $newSteps);
-        $this->assertSame($newSteps, $this->bug->getSteps());
-        $this->assertSame(0, $this->bug->getProgress()->getProcessed());
-        $this->assertSame(0, $this->bug->getProgress()->getTotal());
     }
 
     public function testIncreaseProcessed(): void
@@ -174,15 +162,4 @@ public function testStopRecordingBug(): void
         $this->bugRepository->stopRecording($this->bug);
         $this->assertFalse($this->bug->getVideo()->isRecording());
     }
-
-    public function testUpdateVideoErrorMessage(): void
-    {
-        $this->connection->expects($this->once())->method('connect');
-        $this->bug->getVideo()->setErrorMessage(null);
-        $this->manager->expects($this->once())->method('refresh')->with($this->bug);
-        $this->manager->expects($this->once())->method('flush');
-        $this->manager->expects($this->once())->method('getConnection')->willReturn($this->connection);
-        $this->bugRepository->updateVideoErrorMessage($this->bug, 'New error');
-        $this->assertSame('New error', $this->bug->getVideo()->getErrorMessage());
-    }
 }