Homarus refactor / Increase test coverage (#57)

* Tell if we failed Fix some missed things * Clean up tester, fix test coverage * Fix last tests I hope * Refactor Homarus Update test coverage * Coder fix
Islandora · Jan 10, 2019 · 8276680 · 8276680
1 parent 2a16ceb
commit 8276680
Show file tree

Hide file tree

Showing 19 changed files with 638 additions and 44 deletions.
diff --git a/Gemini/phpunit.xml.dist b/Gemini/phpunit.xml.dist
@@ -23,6 +23,8 @@
             <exclude>
                 <file>./src/index.php</file>
                 <file>./src/app.php</file>
+                <file>./src/console.php</file>
+                <directory>./src/Migrations</directory>
             </exclude>
             <directory suffix=".php">./src</directory>
         </whitelist>

diff --git a/Gemini/tests/Islandora/Gemini/Tests/DeleteTest.php b/Gemini/tests/Islandora/Gemini/Tests/DeleteTest.php
@@ -16,6 +16,7 @@
 class DeleteTest extends \PHPUnit_Framework_TestCase
 {
     /**
+     * @covers ::__construct
      * @covers ::delete
      */
     public function testReturns404WhenNotFound()
@@ -44,6 +45,7 @@ public function testReturns404WhenNotFound()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::delete
      */
     public function testReturns204WhenDeleted()

diff --git a/Gemini/tests/Islandora/Gemini/Tests/GetTest.php b/Gemini/tests/Islandora/Gemini/Tests/GetTest.php
@@ -16,6 +16,7 @@
 class GetTest extends \PHPUnit_Framework_TestCase
 {
     /**
+     * @covers ::__construct
      * @covers ::get
      */
     public function testReturns404WhenNotFound()
@@ -44,6 +45,7 @@ public function testReturns404WhenNotFound()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::get
      */
     public function testReturns200WhenFound()

diff --git a/Gemini/tests/Islandora/Gemini/Tests/PostTest.php b/Gemini/tests/Islandora/Gemini/Tests/PostTest.php
@@ -16,6 +16,7 @@
 class PostTest extends \PHPUnit_Framework_TestCase
 {
     /**
+     * @covers ::__construct
      * @covers ::post
      */
     public function testReturns400OnMalformedRequest()
@@ -68,6 +69,7 @@ public function testReturns400OnMalformedRequest()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::post
      */
     public function testReturns200OnSuccess()

diff --git a/Gemini/tests/Islandora/Gemini/Tests/PutTest.php b/Gemini/tests/Islandora/Gemini/Tests/PutTest.php
@@ -17,6 +17,7 @@
 class PutTest extends \PHPUnit_Framework_TestCase
 {
     /**
+     * @covers ::__construct
      * @covers ::put
      */
     public function testReturns204OnUpdate()
@@ -58,6 +59,7 @@ public function testReturns204OnUpdate()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::put
      */
     public function testReturns201OnCreation()
@@ -107,6 +109,7 @@ public function testReturns201OnCreation()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::put
      */
     public function testReturns400OnMalformedRequest()

diff --git a/Gemini/tests/Islandora/Gemini/Tests/UrlMapperTest.php b/Gemini/tests/Islandora/Gemini/Tests/UrlMapperTest.php
@@ -16,6 +16,7 @@
 class UrlMapperTest extends \PHPUnit_Framework_TestCase
 {
     /**
+     * @covers ::__construct
      * @covers ::getUrls
      */
     public function testGetUrlsReturnsUnmodifiedResults()
@@ -56,6 +57,7 @@ public function testGetUrlsReturnsUnmodifiedResults()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::saveUrls
      */
     public function testSaveUrlsReturnsTrueOnCreation()
@@ -78,6 +80,7 @@ public function testSaveUrlsReturnsTrueOnCreation()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::saveUrls
      */
     public function testSaveUrlsReturnsFalseOnUpdate()
@@ -104,6 +107,7 @@ public function testSaveUrlsReturnsFalseOnUpdate()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::saveUrls
      * @expectedException \Exception
      */
@@ -121,6 +125,7 @@ public function testSaveUrlsRollsBackOnException()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::deleteUrls
      */
     public function testDeleteUrlsReturnsTrueIfFound()
@@ -141,6 +146,7 @@ public function testDeleteUrlsReturnsTrueIfFound()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::deleteUrls
      */
     public function testDeleteUrlsReturnsFalseIfNotFound()
@@ -161,6 +167,7 @@ public function testDeleteUrlsReturnsFalseIfNotFound()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::deleteUrls
      * @expectedException \Exception
      */

diff --git a/Gemini/tests/Islandora/Gemini/Tests/UrlMinterTest.php b/Gemini/tests/Islandora/Gemini/Tests/UrlMinterTest.php
@@ -12,6 +12,7 @@
 class UrlMinterTest extends \PHPUnit_Framework_TestCase
 {
     /**
+     * @covers ::__construct
      * @covers ::mint
      * @expectedException \InvalidArgumentException
      * @expectedExceptionCode 400
@@ -23,6 +24,7 @@ public function testThrowsExceptionOnBlankString()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::mint
      * @expectedException \InvalidArgumentException
      * @expectedExceptionCode 400
@@ -34,6 +36,7 @@ public function testThrowsExceptionOnShortUUID()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::mint
      */
     public function testHandlesMissingTrailingSlashInBaseUrl()
@@ -51,6 +54,7 @@ public function testHandlesMissingTrailingSlashInBaseUrl()
     }
 
     /**
+     * @covers ::__construct
      * @covers ::mint
      */
     public function testMintsUrlWithPairTrees()

diff --git a/Homarus/cfg/config.example.yaml b/Homarus/cfg/config.example.yaml
@@ -7,12 +7,13 @@ homarus:
       - video/mp4
       - video/x-msvideo
       - video/ogg
-    default_video: video/mp4
+    default: video/mp4
   mime_to_format:
     valid:
       - video/mp4_mp4
       - video/x-msvideo_avi
       - video/ogg_ogg
+    default: mp4
 
 fedora_resource:
   base_url: http://localhost:8080/fcrepo/rest

diff --git a/Homarus/src/Controller/HomarusController.php b/Homarus/src/Controller/HomarusController.php
@@ -29,28 +29,66 @@ class HomarusController
    */
     protected $log;
 
+  /**
+   * Not used I think.
+   *
+   * @var array
+   */
+    private $mimetypes;
+
+  /**
+   * Default FFmpeg format
+   *
+   * @var string
+   */
+    private $default_mimetype;
+
+  /**
+   * Mapping of mime-type to ffmpeg formats.
+   *
+   * @var array
+   */
+    private $mime_to_format;
+
+    /**
+     * The default format.
+     *
+     * @var string
+     */
+    private $default_format;
+
+  /**
+   * The executable.
+   *
+   * @var string
+   */
+    private $executable;
+
   /**
    * Controller constructor.
    * @param \Islandora\Crayfish\Commons\CmdExecuteService $cmd
-   * @param array $formats
-   * @param string $default_format
+   * @param array $mimetypes
+   * @param string $default_mimetype
    * @param string $executable
    * @param $log
+   * @param array $mime_to_format
    */
     public function __construct(
         CmdExecuteService $cmd,
-        $formats,
-        $default_format,
+        $mimetypes,
+        $default_mimetype,
         $executable,
         $log,
-        $mime_to_format
+        $mime_to_format,
+        $default_format
     ) {
         $this->cmd = $cmd;
-        $this->formats = $formats;
-        $this->default_format = $default_format;
+        $this->mimetypes = $mimetypes;
+        $this->default_mimetype = $default_mimetype;
         $this->executable = $executable;
         $this->log = $log;
         $this->mime_to_format = $mime_to_format;
+        $this->default_format = $default_format;
     }
 
   /**
@@ -63,7 +101,7 @@ public function convert(Request $request)
 
         // Short circuit if there's no Apix-Ldp-Resource header.
         if (!$request->headers->has("Apix-Ldp-Resource")) {
-            $this->log->debug("Malformed request, no Apix-Ldp-Resource header present");
+            $this->log->error("Malformed request, no Apix-Ldp-Resource header present");
             return new Response(
                 "Malformed request, no Apix-Ldp-Resource header present",
                 400
@@ -74,8 +112,7 @@ public function convert(Request $request)
 
         // Find the format
         $content_types = $request->getAcceptableContentTypes();
-        $content_type = $this->getContentType($content_types);
-        $format = $this->getFfmpegFormat($content_type);
+        list($content_type, $format) = $this->getFfmpegFormat($content_types);
 
         $cmd_params = "";
         if ($format == "mp4") {
@@ -89,7 +126,7 @@ public function convert(Request $request)
         $this->log->debug("X-Islandora-Args:", ['args' => $args]);
 
         $cmd_string = "$this->executable -i $source $cmd_params -f $format -";
-        $this->log->info('Ffempg Command:', ['cmd' => $cmd_string]);
+        $this->log->debug('Ffempg Command:', ['cmd' => $cmd_string]);
 
         // Return response.
         try {
@@ -104,34 +141,38 @@ public function convert(Request $request)
         }
     }
 
-
-    private function getContentType($content_types)
+    /**
+     * Filters through an array of acceptable content-types and returns a FFmpeg format.
+     *
+     * @param array $content_types
+     *   The Accept content-types.
+     *
+     * @return array
+     *   Array with [ $content-type, $format ], falls back to defaults.
+     */
+    private function getFfmpegFormat(array $content_types)
     {
         $content_type = null;
         foreach ($content_types as $type) {
-            if (in_array($type, $this->formats)) {
+            if (in_array($type, $this->mimetypes)) {
                 $content_type = $type;
                 break;
             }
         }
 
         if ($content_type === null) {
-            $content_type = $this->default_format;
-            $this->log->info('Falling back to default content type');
+            $this->log->info('No matching content-type, falling back to default.');
+            return [$this->default_mimetype, $this->default_format];
         }
-        return $content_type;
-    }
 
-    private function getFfmpegFormat($content_type)
-    {
         foreach ($this->mime_to_format as $format) {
-            if (strpos($format, $content_type) !== false) {
-                $this->log->info("does it get here");
-                $format_info = explode("_", $format);
-                break;
+            $format_info = explode("_", $format);
+            if ($format_info[0] == $content_type) {
+                return [$content_type, $format_info[1]];
             }
         }
-        return $format_info[1];
+        $this->log->info('No matching content-type to format mapping, falling back to default.');
+        return [$this->default_mimetype, $this->default_format];
     }
 
   /**

diff --git a/Homarus/src/app.php b/Homarus/src/app.php
@@ -16,10 +16,11 @@
     return new HomarusController(
         $app['crayfish.cmd_execute_service'],
         $app['crayfish.homarus.mime_types.valid'],
-        $app['crayfish.homarus.mime_types.default_video'],
+        $app['crayfish.homarus.mime_types.default'],
         $app['crayfish.homarus.executable'],
         $app['monolog'],
-        $app['crayfish.homarus.mime_to_format']
+        $app['crayfish.homarus.mime_to_format.valid'],
+        $app['crayfish.homarus.mime_to_format.default']
     );
 };