lonvia PR feedback

.travis.yml

@@ -25,7 +25,7 @@ script:
   - cd $TRAVIS_BUILD_DIR/
   - if [[ $TEST_SUITE == "tests" ]]; then phpcs --report-width=120 . ; fi
   - cd $TRAVIS_BUILD_DIR/test/php
-  - if [[ $TEST_SUITE == "tests" ]]; then UNIT_TEST_DSN='pgsql:dbname=nominatim_unit_tests' /usr/bin/phpunit ./ ; fi
+  - if [[ $TEST_SUITE == "tests" ]]; then /usr/bin/phpunit ./ ; fi
   - cd $TRAVIS_BUILD_DIR/test/bdd
   - # behave --format=progress3 api
   - if [[ $TEST_SUITE == "tests" ]]; then behave -DREMOVE_TEMPLATE=1 --format=progress3 db ; fi

lib/DB.php

@@ -251,7 +251,7 @@ class DB
     }
 
     /**
-     * Deletes a table. Returns true on success. Returns true if the table didn't exist.
+     * Deletes a table. Returns true if deleted or didn't exist.
      *
      * @param string  $sTableName
      *
@@ -405,29 +405,16 @@ END;
      */
     public static function generateDSN($aInfo)
     {
-        $sDSN = 'pgsql:';
+        $sDSN = sprintf(
-        if (isset($aInfo['host'])) {
+            'pgsql:host=%s;port=%s;dbname=%s;user=%s;password=%s;',
-            $sDSN .= 'host=' . $aInfo['host'] . ';';
+            $aInfo['host'] ?? $aInfo['hostspec'] ?? '',
-        } elseif (isset($aInfo['hostspec'])) {
+            $aInfo['port'] ?? '',
-            $sDSN .= 'host=' . $aInfo['hostspec'] . ';';
+            $aInfo['dbname'] ?? $aInfo['database'] ?? '',
-        }
+            $aInfo['user'] ?? '',
-        if (isset($aInfo['port'])) {
+            $aInfo['password'] ?? ''
-            $sDSN .= 'port=' . $aInfo['port'] . ';';
+        );
-        }
+        $sDSN = preg_replace('/\b\w+=;/', '', $sDSN);
-        if (isset($aInfo['dbname'])) {
+        $sDSN = preg_replace('/;\Z/', '', $sDSN);
-            $sDSN .= 'dbname=' . $aInfo['dbname'] . ';';
-        } elseif (isset($aInfo['database'])) {
-            $sDSN .= 'dbname=' . $aInfo['database'] . ';';
-        }
-        if (isset($aInfo['user'])) {
-            $sDSN .= 'user=' . $aInfo['user'] . ';';
-        } elseif (isset($aInfo['username'])) {
-            $sDSN .= 'user=' . $aInfo['username'] . ';';
-        }
-        if (isset($aInfo['password'])) {
-            $sDSN .= 'password=' . $aInfo['password'] . ';';
-        }
-        $sDSN = preg_replace('/;$/', '', $sDSN);
 
         return $sDSN;
     }

test/README.md

@@ -51,8 +51,8 @@ To execute the test suite run
 It will read phpunit.xml which points to the library, test path, bootstrap
 strip and set other parameters.
 
-The database set by `UNIT_TEST_DSN` will be deleted and recreated. Not setting
+It will use (and destroy) a local database 'nominatim_unit_tests'. You can set
-it will skip some tests as pending, but not fail the tests.
+a different connection string with e.g. UNIT_TEST_DSN='pgsql:dbname=foo_unit_tests'.
 
 BDD Functional Tests
 ====================

test/php/Nominatim/DBTest.php

@@ -128,11 +128,19 @@ class DBTest extends \PHPUnit\Framework\TestCase
 
     public function testAgainstDatabase()
     {
-        if (getenv('UNIT_TEST_DSN') == false) $this->markTestSkipped('UNIT_TEST_DSN not set');
+        $unit_test_dsn = getenv('UNIT_TEST_DSN') != false ?
+                            getenv('UNIT_TEST_DSN') :
+                            'pgsql:dbname=nominatim_unit_tests';
+
+        $this->assertRegExp(
+            '/unit_test/',
+            $unit_test_dsn,
+            'Test database will get destroyed, thus should have a name like unit_test to be safe'
+        );
 
         ## Create the database.
         {
-            $aDSNParsed = \Nominatim\DB::parseDSN(getenv('UNIT_TEST_DSN'));
+            $aDSNParsed = \Nominatim\DB::parseDSN($unit_test_dsn);
             $sDbname = $aDSNParsed['database'];
             $aDSNParsed['database'] = 'postgres';
 
@@ -142,7 +150,7 @@ class DBTest extends \PHPUnit\Framework\TestCase
             $oDB->exec('CREATE DATABASE ' . $sDbname);
         }
 
-        $oDB = new \Nominatim\DB(getenv('UNIT_TEST_DSN'));
+        $oDB = new \Nominatim\DB($unit_test_dsn);
         $oDB->connect();
 
         $this->assertTrue(