Merge pull request #16 from factorio-item-browser/feature/better-error-messages

Author: BluePsyduck

CHANGELOG.md

@@ -1,10 +1,11 @@
 # Changelog
 
-## Unreleased
+## 2.0.1 - 2020-04-24
 
 ### Changed
 
 - Extracted icon renderer to Go-based binary for major performance improvement.
+- Improved error message in the case that Factorio crashed.
 
 ### Fixed
 

config/autoload/base.global.php

@@ -17,5 +17,5 @@
     ConfigAggregator::ENABLE_CACHE => true,
     'debug' => false,
     'name' => 'Factorio Item Browser - Export',
-    'version' => '2.0.0'
+    'version' => '2.0.1'
 ];

src/Exception/FactorioExecutionException.php

@@ -0,0 +1,72 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FactorioItemBrowser\Export\Exception;
+
+use Throwable;
+
+/**
+ * The exception thrown when the execution of Factorio failed.
+ *
+ * @author BluePsyduck <[email protected]>
+ * @license http://opensource.org/licenses/GPL-3.0 GPL v3
+ */
+class FactorioExecutionException extends ExportException
+{
+    /**
+     * The message template of the exception.
+     */
+    protected const MESSAGE = 'Factorio exited with code %d: %s';
+
+    /**
+     * The regular expression to detect the line starting the error.
+     */
+    protected const REGEXP_ERROR_LINE = '(^[ ]*\d+\.\d{3} Error |^Error: )';
+
+    /**
+     * Initializes the exception.
+     * @param int $exitCode
+     * @param string $output
+     * @param Throwable|null $previous
+     */
+    public function __construct(int $exitCode, string $output, ?Throwable $previous = null)
+    {
+        $message = $this->extractErrorMessageFromOutput($output);
+        parent::__construct(sprintf(self::MESSAGE, $exitCode, $message), $exitCode, $previous);
+    }
+
+    /**
+     * Extracts the actual error message from the output.
+     * @param string $output
+     * @return string
+     */
+    protected function extractErrorMessageFromOutput(string $output): string
+    {
+        $errorLines = [];
+        $errorFound = false;
+
+        $lines = array_reverse(explode(PHP_EOL, $output));
+        foreach ($lines as $line) {
+            $errorLines[] = $line;
+            if (preg_match(self::REGEXP_ERROR_LINE, $line) > 0) {
+                $errorFound = true;
+                break;
+            }
+        }
+
+        if (!$errorFound) {
+            // We were unable to detect the start of the error. So take the last 10 lines instead.
+            $errorLines = [];
+            foreach (array_slice($lines, 0, 10) as $line) {
+                if (strpos($line, '>>>') !== false) {
+                    // We have our dump placeholder, so break now before we add the dump to the message.
+                    break;
+                }
+                $errorLines[] = $line;
+            }
+        }
+
+        return implode(PHP_EOL, array_reverse($errorLines));
+    }
+}

src/Factorio/Instance.php

@@ -9,6 +9,7 @@
 use FactorioItemBrowser\Export\Entity\Dump\Dump;
 use FactorioItemBrowser\Export\Entity\InfoJson;
 use FactorioItemBrowser\Export\Exception\ExportException;
+use FactorioItemBrowser\Export\Exception\FactorioExecutionException;
 use FactorioItemBrowser\Export\Mod\ModFileManager;
 use JMS\Serializer\SerializerInterface;
 use Symfony\Component\Process\Process;
@@ -57,6 +58,12 @@ class Instance
      */
     protected $instancesDirectory;
 
+    /**
+     * The version of the export project.
+     * @var string
+     */
+    protected $version;
+
     /**
      * The directory for the combination instance.
      * @var string
@@ -71,21 +78,24 @@ class Instance
      * @param SerializerInterface $exportSerializer
      * @param string $factorioDirectory
      * @param string $instancesDirectory
+     * @param string $version
      */
     public function __construct(
         Console $console,
         DumpExtractor $dumpExtractor,
         ModFileManager $modFileManager,
         SerializerInterface $exportSerializer,
         string $factorioDirectory,
-        string $instancesDirectory
+        string $instancesDirectory,
+        string $version
     ) {
         $this->console = $console;
         $this->dumpExtractor = $dumpExtractor;
         $this->modFileManager = $modFileManager;
         $this->serializer = $exportSerializer;
         $this->factorioDirectory = $factorioDirectory;
         $this->instancesDirectory = $instancesDirectory;
+        $this->version = $version;
     }
 
     /**
@@ -177,8 +187,9 @@ protected function createDumpInfoJson(array $modNames): InfoJson
 
         $info = new InfoJson();
         $info->setName('Dump')
+             ->setTitle('Factorio Item Browser - Dump')
              ->setAuthor('factorio-item-browser')
-             ->setVersion('1.0.0')
+             ->setVersion($this->version)
              ->setFactorioVersion($baseInfo->getVersion())
              ->setDependencies($modNames);
 
@@ -188,11 +199,16 @@ protected function createDumpInfoJson(array $modNames): InfoJson
     /**
      * Executes the Factorio instance.
      * @return string
+     * @throws ExportException
      */
     protected function execute(): string
     {
         $process = $this->createProcess();
         $process->run();
+        if (!$process->isSuccessful()) {
+            throw new FactorioExecutionException((int) $process->getExitCode(), $process->getOutput());
+        }
+
         return $process->getOutput();
     }
 

test/src/Exception/FactorioExecutionExceptionTest.php

@@ -0,0 +1,143 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FactorioItemBrowserTest\Export\Exception;
+
+use BluePsyduck\TestHelper\ReflectionTrait;
+use Exception;
+use FactorioItemBrowser\Export\Exception\FactorioExecutionException;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+use ReflectionException;
+
+/**
+ * The PHPUnit test of the FactorioExecutionException class.
+ *
+ * @author BluePsyduck <[email protected]>
+ * @license http://opensource.org/licenses/GPL-3.0 GPL v3
+ * @coversDefaultClass \FactorioItemBrowser\Export\Exception\FactorioExecutionException
+ */
+class FactorioExecutionExceptionTest extends TestCase
+{
+    use ReflectionTrait;
+
+    /**
+     * Tests the constructing.
+     * @covers ::__construct
+     */
+    public function testConstruct(): void
+    {
+        $output = 'abc';
+        $errorMessage = 'def';
+        $exitCode = 42;
+        $expectedMessage = 'Factorio exited with code 42: def';
+
+        /* @var Exception&MockObject $previous */
+        $previous = $this->createMock(Exception::class);
+
+        /* @var FactorioExecutionException&MockObject $exception */
+        $exception = $this->getMockBuilder(FactorioExecutionException::class)
+                          ->onlyMethods(['extractErrorMessageFromOutput'])
+                          ->disableOriginalConstructor()
+                          ->getMock();
+        $exception->expects($this->once())
+                  ->method('extractErrorMessageFromOutput')
+                  ->with($this->identicalTo($output))
+                  ->willReturn($errorMessage);
+
+        $exception->__construct($exitCode, $output, $previous);
+
+        $this->assertSame($expectedMessage, $exception->getMessage());
+        $this->assertSame($exitCode, $exception->getCode());
+        $this->assertSame($previous, $exception->getPrevious());
+    }
+
+    /**
+     * Provides the data for the extractErrorMessageFromOutput test.
+     * @return array<mixed>
+     */
+    public function provideExtractErrorMessageFromOutput(): array
+    {
+        $output1 = <<<EOT
+   0.007 Running in headless mode
+   0.009 Loading mod core 0.0.0 (data.lua)
+   0.061 Loading mod base 0.18.21 (data.lua)
+   0.235 Error Util.cpp:83: Failed to load mod "base": __base__/data.lua:89: __base__/prototypes/...
+stack traceback:
+	[C]: in function 'require'
+	__base__/data.lua:89: in main chunk
+EOT;
+        $message1 = <<<EOT
+   0.235 Error Util.cpp:83: Failed to load mod "base": __base__/data.lua:89: __base__/prototypes/...
+stack traceback:
+	[C]: in function 'require'
+	__base__/data.lua:89: in main chunk
+EOT;
+
+        $output2 = <<<EOT
+   2.399 Checksum for script /project/data/instances/2f4a45fa-a509-a9d1-aae6-ffcf984a7a76/...
+   2.401 Checksum for script __Dump__/control.lua: 3285258963
+Error: The mod Factorio Item Browser - Dump (1.0.0) caused a non-recoverable error.
+Please report this error to the mod author.
+
+Error while running event Dump::on_init()
+__Dump__/map.lua:82: attempt to index global 'prototype2' (a nil value)
+stack traceback:
+	__Dump__/map.lua:82: in function 'recipe'
+	__Dump__/control.lua:37: in function <__Dump__/control.lua:4>
+   2.448 Goodbye
+EOT;
+        $message2 = <<<EOT
+Error: The mod Factorio Item Browser - Dump (1.0.0) caused a non-recoverable error.
+Please report this error to the mod author.
+
+Error while running event Dump::on_init()
+__Dump__/map.lua:82: attempt to index global 'prototype2' (a nil value)
+stack traceback:
+	__Dump__/map.lua:82: in function 'recipe'
+	__Dump__/control.lua:37: in function <__Dump__/control.lua:4>
+   2.448 Goodbye
+EOT;
+
+        $output3 = <<<EOT
+Lorem ipsum dolor sit amet.
+>>>TEST>>>Actual dumped data we do not want to see in the error<<<TEST<<<
+foo
+bar
+   2.448 Goodbye
+EOT;
+        $message3 = <<<EOT
+foo
+bar
+   2.448 Goodbye
+EOT;
+
+        return [
+            [$output1, $message1],
+            [$output2, $message2],
+            [$output3, $message3],
+        ];
+    }
+
+    /**
+     * Tests the extractErrorMessageFromOutput method.
+     * @param string $output
+     * @param string $expectedResult
+     * @throws ReflectionException
+     * @covers ::extractErrorMessageFromOutput
+     * @dataProvider provideExtractErrorMessageFromOutput
+     */
+    public function testExtractErrorMessageFromOutput(string $output, string $expectedResult): void
+    {
+        /* @var FactorioExecutionException&MockObject $exception */
+        $exception = $this->getMockBuilder(FactorioExecutionException::class)
+                          ->onlyMethods(['__construct'])
+                          ->disableOriginalConstructor()
+                          ->getMock();
+
+        $result = $this->invokeMethod($exception, 'extractErrorMessageFromOutput', $output);
+
+        $this->assertSame($expectedResult, $result);
+    }
+}