From a1438b49774b56c9bd82ed205b7e2322a6131cbe Mon Sep 17 00:00:00 2001 From: tienvx Date: Sun, 24 Apr 2022 02:08:35 +0700 Subject: [PATCH 1/2] Prevent updating bug's steps after running --- src/Reducer/HandlerTemplate.php | 20 +++++--- src/Repository/BugRepository.php | 12 +---- src/Repository/BugRepositoryInterface.php | 2 - src/Resources/config/services.php | 8 +++ src/Service/Bug/BugHelper.php | 19 +++++--- src/Service/Step/Runner/BugStepsRunner.php | 28 ----------- src/Service/Step/StepHelper.php | 27 ++++++++++ src/Service/Step/StepHelperInterface.php | 8 +++ tests/Reducer/HandlerTestCase.php | 8 +++ tests/Reducer/Random/RandomHandlerTest.php | 3 +- tests/Reducer/Split/SplitHandlerTest.php | 3 +- tests/Repository/BugRepositoryTest.php | 57 +++++++--------------- tests/Service/Bug/BugHelperTest.php | 31 ++++++------ tests/Service/Step/StepHelperTest.php | 42 ++++++++++++++++ 14 files changed, 157 insertions(+), 111 deletions(-) create mode 100644 src/Service/Step/StepHelper.php create mode 100644 src/Service/Step/StepHelperInterface.php create mode 100644 tests/Service/Step/StepHelperTest.php diff --git a/src/Reducer/HandlerTemplate.php b/src/Reducer/HandlerTemplate.php index 8b8bb30b..0cc070ea 100644 --- a/src/Reducer/HandlerTemplate.php +++ b/src/Reducer/HandlerTemplate.php @@ -9,6 +9,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 HandlerTemplate implements HandlerInterface { @@ -16,17 +17,20 @@ abstract class HandlerTemplate implements HandlerInterface 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 index dee6d7ca..fac7f7f6 100644 --- 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 index 6e074a55..f834c7c8 100644 --- 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 index 0ff91385..f28e853a 100644 --- 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 index 3d9250e3..24f3ddd7 100644 --- a/src/Service/Bug/BugHelper.php +++ b/src/Service/Bug/BugHelper.php @@ -14,6 +14,7 @@ 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 { @@ -21,6 +22,7 @@ class BugHelper implements BugHelperInterface protected BugRepositoryInterface $bugRepository; protected MessageBusInterface $messageBus; protected BugNotifierInterface $bugNotifier; + protected StepHelperInterface $stepHelper; protected BugStepsRunner $stepsRunner; protected ConfigInterface $config; @@ -29,6 +31,7 @@ public function __construct( BugRepositoryInterface $bugRepository, MessageBusInterface $messageBus, BugNotifierInterface $bugNotifier, + StepHelperInterface $stepHelper, BugStepsRunner $stepsRunner, ConfigInterface $config ) { @@ -36,6 +39,7 @@ public function __construct( $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 index 7f2e4288..3635dabb 100644 --- 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 new file mode 100644 index 00000000..c13a2b87 --- /dev/null +++ b/src/Service/Step/StepHelper.php @@ -0,0 +1,27 @@ +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 new file mode 100644 index 00000000..88d4cd73 --- /dev/null +++ b/src/Service/Step/StepHelperInterface.php @@ -0,0 +1,8 @@ +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 index 3a1b866a..acd7d3eb 100644 --- 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 index 63515131..878bf5e2 100644 --- 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 index 413986e3..6be72ba0 100644 --- 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()); - } } diff --git a/tests/Service/Bug/BugHelperTest.php b/tests/Service/Bug/BugHelperTest.php index e3fe7c93..5efd72b3 100644 --- a/tests/Service/Bug/BugHelperTest.php +++ b/tests/Service/Bug/BugHelperTest.php @@ -24,6 +24,7 @@ use Tienvx\Bundle\MbtBundle\Service\Bug\BugNotifierInterface; use Tienvx\Bundle\MbtBundle\Service\ConfigInterface; use Tienvx\Bundle\MbtBundle\Service\Step\Runner\BugStepsRunner; +use Tienvx\Bundle\MbtBundle\Service\Step\StepHelperInterface; /** * @covers \Tienvx\Bundle\MbtBundle\Service\Bug\BugHelper @@ -45,6 +46,7 @@ class BugHelperTest extends TestCase protected BugRepositoryInterface $bugRepository; protected MessageBusInterface $messageBus; protected BugNotifierInterface $bugNotifier; + protected StepHelperInterface $stepHelper; protected BugStepsRunner $stepsRunner; protected ConfigInterface $config; protected BugHelperInterface $helper; @@ -57,6 +59,7 @@ protected function setUp(): void $this->bugRepository = $this->createMock(BugRepositoryInterface::class); $this->messageBus = $this->createMock(MessageBusInterface::class); $this->bugNotifier = $this->createMock(BugNotifierInterface::class); + $this->stepHelper = $this->createMock(StepHelperInterface::class); $this->stepsRunner = $this->createMock(BugStepsRunner::class); $this->config = $this->createMock(ConfigInterface::class); $this->helper = new BugHelper( @@ -64,6 +67,7 @@ protected function setUp(): void $this->bugRepository, $this->messageBus, $this->bugNotifier, + $this->stepHelper, $this->stepsRunner, $this->config ); @@ -225,7 +229,7 @@ public function testRecordVideoMissingBug(): void $this->helper->recordVideo(123); } - public function testRunTaskAlreadyRunning(): void + public function testRecordVideoAlreadyRecording(): void { $this->expectException(RecoverableMessageHandlingException::class); $this->expectExceptionMessage('Can not record video for bug 123: bug is recording. Will retry later'); @@ -237,7 +241,7 @@ public function testRunTaskAlreadyRunning(): void /** * @dataProvider exceptionProvider */ - public function testRecordVideo(?Throwable $exception): void + public function testRecordVideo(?Throwable $exception, ?string $expectedVideoErrorMessage): void { $this->stepsRunner ->expects($this->once()) @@ -254,29 +258,24 @@ public function testRecordVideo(?Throwable $exception): void }) ); $this->bugRepository->expects($this->once())->method('find')->with(123)->willReturn($this->bug); + $this->stepHelper + ->expects($this->once()) + ->method('cloneStepsAndResetColor') + ->with($this->bug->getSteps()) + ->willReturnArgument(0); $this->bugRepository->expects($this->once())->method('startRecording')->with($this->bug); $this->bugRepository->expects($this->once())->method('stopRecording')->with($this->bug); - if ($exception) { - $this->bugRepository - ->expects($this->once()) - ->method('updateVideoErrorMessage') - ->with( - $this->bug, - $exception->getMessage() !== $this->bug->getMessage() ? $exception->getMessage() : null - ); - } else { - $this->bugRepository->expects($this->never())->method('updateVideoErrorMessage'); - } $this->helper->recordVideo(123); $this->assertTrue($this->bug->isDebug()); + $this->assertSame($expectedVideoErrorMessage, $this->bug->getVideo()->getErrorMessage()); } public function exceptionProvider(): array { return [ - [null], - [new Exception('Something wrong')], - [new Exception('Something else wrong')], + [null, null], + [new Exception('Something wrong'), null], + [new Exception('Something else wrong'), 'Something else wrong'], ]; } } diff --git a/tests/Service/Step/StepHelperTest.php b/tests/Service/Step/StepHelperTest.php new file mode 100644 index 00000000..131a9a19 --- /dev/null +++ b/tests/Service/Step/StepHelperTest.php @@ -0,0 +1,42 @@ +cloneStepsAndResetColor($steps); + $this->assertCount(count($steps), $newSteps); + foreach ($newSteps as $index => $newStep) { + $this->assertInstanceOf(StepInterface::class, $newStep); + $this->assertSame($steps[$index]->getPlaces(), $newStep->getPlaces()); + $this->assertSame($steps[$index]->getTransition(), $newStep->getTransition()); + $this->assertNotSame($steps[$index]->getColor(), $newStep->getColor()); + if ($index > 0) { + $this->assertNotSame($steps[$index - 1]->getColor(), $newStep->getColor()); + $this->assertSame($steps[$index - 1]->getColor()->getValues(), $newStep->getColor()->getValues()); + } else { + $this->assertSame([], $newStep->getColor()->getValues()); + } + } + } +} From acc68e4858bf4500d6d2de0c4c50566c61b6e6e0 Mon Sep 17 00:00:00 2001 From: tienvx Date: Sun, 24 Apr 2022 02:14:00 +0700 Subject: [PATCH 2/2] Test clone invalid steps --- tests/Service/Step/StepHelperTest.php | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/Service/Step/StepHelperTest.php b/tests/Service/Step/StepHelperTest.php index 131a9a19..41b4abe6 100644 --- a/tests/Service/Step/StepHelperTest.php +++ b/tests/Service/Step/StepHelperTest.php @@ -4,6 +4,7 @@ use PHPUnit\Framework\TestCase; use SingleColorPetrinet\Model\Color; +use Tienvx\Bundle\MbtBundle\Exception\UnexpectedValueException; use Tienvx\Bundle\MbtBundle\Model\Bug\StepInterface; use Tienvx\Bundle\MbtBundle\Service\Step\StepHelper; use Tienvx\Bundle\MbtBundle\ValueObject\Bug\Step; @@ -15,6 +16,21 @@ */ class StepHelperTest extends TestCase { + protected StepHelper $stepHelper; + + protected function setUp(): void + { + $this->stepHelper = new StepHelper(); + } + + public function testCloneInvalidSteps(): void + { + $this->expectExceptionObject( + new UnexpectedValueException(sprintf('Step must be instance of "%s".', StepInterface::class)) + ); + $this->stepHelper->cloneStepsAndResetColor([new \stdClass()]); + } + public function testCloneStepsAndResetColor(): void { $steps = [ @@ -23,8 +39,7 @@ public function testCloneStepsAndResetColor(): void new Step([2], new Color(), 2), new Step([3], new Color(), 3), ]; - $stepHelper = new StepHelper(); - $newSteps = $stepHelper->cloneStepsAndResetColor($steps); + $newSteps = $this->stepHelper->cloneStepsAndResetColor($steps); $this->assertCount(count($steps), $newSteps); foreach ($newSteps as $index => $newStep) { $this->assertInstanceOf(StepInterface::class, $newStep);