Merge pull request #79 from OndraM/feature/one-class-per-file

Check one class per file & refactoring
lmc-eu · Aug 5, 2016 · 051d050 · 051d050
2 parents 520b538 + 9e9d749
commit 051d050
Show file tree

Hide file tree

Showing 6 changed files with 181 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@
 - Directory for logs is automatically created if it does not exist. ([#67](https://github.com/lmc-eu/steward/issues/67))
 - Process output printed to console (when using `-vv` or `-vvv`) is now prefixed with class name.
 - BC: Pass instance of current PHPUnit test as third parameter of `publishResult()` method of `AbstractPublisher` descendants.
+- Throw an exception if there are multiple testcase classes defined in one file.
 
 ### Fixed
 - Parsing of latest Selenium server version in `install` command so that even Selenium 3 beta releases are installed ([#76](https://github.com/lmc-eu/steward/pull/76))

diff --git a/src-tests/Process/Fixtures/InvalidTests/MultipleClassesInFileTest.php b/src-tests/Process/Fixtures/InvalidTests/MultipleClassesInFileTest.php
@@ -0,0 +1,17 @@
+<?php
+
+namespace Lmc\Steward\Process\Fixtures\InvalidTests;
+
+use Lmc\Steward\Test\AbstractTestCase;
+
+/**
+ * File containing two classes (thus violating PSR-0 and PSR-4).
+ * @codingStandardsIgnoreFile
+ */
+class MultipleClassesInFileTest extends AbstractTestCase
+{
+}
+
+class AnotherClassTest extends AbstractTestCase
+{
+}
diff --git a/src-tests/Process/Fixtures/InvalidTests/WrongClassTest.php b/src-tests/Process/Fixtures/InvalidTests/WrongClassTest.php
@@ -6,6 +6,7 @@
 
 /**
  * Class with wrong name (differs from name of the file)
+ * @codingStandardsIgnoreFile
  */
 class ReallyWrongClassTest extends AbstractTestCase
 {

diff --git a/src-tests/Process/ProcessSetCreatorTest.php b/src-tests/Process/ProcessSetCreatorTest.php
@@ -118,8 +118,8 @@ public function testShouldCreateProcessSetFromGivenFiles()
     public function testShouldThrowExceptionIfAddingFileWithNoClass()
     {
         $files = $this->findDummyTests('NoClassTest.php', 'InvalidTests');
-
         $fileName = key(iterator_to_array($files->getIterator()));
+
         $this->setExpectedException(
             \RuntimeException::class,
             'No class found in file "' . $fileName . '"'
@@ -140,6 +140,20 @@ public function testShouldThrowExceptionIfAddingClassWithNameMismatchingTheFileN
         $this->creator->createFromFiles($files, [], []);
     }
 
+    public function testShouldThrowExceptionIfMultipleClassesAreDefinedInFile()
+    {
+        $files = $this->findDummyTests('MultipleClassesInFileTest.php', 'InvalidTests');
+        $fileName = key(iterator_to_array($files->getIterator()));
+
+        $this->setExpectedException(
+            \RuntimeException::class,
+            'File "' . $fileName . '" contains definition of 2 classes. However, each class must be defined in its'
+            . ' own separate file.'
+        );
+        $this->creator->createFromFiles($files, [], []);
+    }
+
+
     public function testShouldOnlyAddTestsOfGivenGroups()
     {
         $processSet = $this->creator->createFromFiles($this->findDummyTests(), ['bar', 'foo'], []);

diff --git a/src/Listener/WebDriverListener.php b/src/Listener/WebDriverListener.php
@@ -69,37 +69,10 @@ public function startTest(\PHPUnit_Framework_Test $test)
             $test->getName()
         );
 
-        $capabilities = new DesiredCapabilities(
-            [
-                WebDriverCapabilityType::BROWSER_NAME => $config->browserName,
-                WebDriverCapabilityType::PLATFORM => WebDriverPlatform::ANY,
-                'name' => get_class($test) . '::' . $test->getName(),
-            ]
-        );
-
-        if (!empty($config->capability)) {
-            $extraCapabilities = json_decode($config->capability);
-            foreach ($extraCapabilities as $extraCapabilityName => $extraCapabilityValue) {
-                $capabilities->setCapability($extraCapabilityName, $extraCapabilityValue);
-            }
-        }
-
-        $ci = CiDetector::detect();
-        if ($ci) {
-            $capabilities->setCapability(
-                'build',
-                ConfigProvider::getInstance()->env . '-' . CiDetector::detect()->getBuildNumber()
-            );
-            $capabilities->setCapability(
-                'tags',
-                [ConfigProvider::getInstance()->env, $ci->getCiName(), get_class($test)]
-            );
-        }
-
         $this->createWebDriver(
             $test,
             $config->serverUrl .  SeleniumServerAdapter::HUB_ENDPOINT,
-            $this->setupCustomCapabilities($capabilities, $config->browserName),
+            $this->setupCapabilities($test),
             $connectTimeoutMs = 2*60*1000,
             // How long could request to Selenium take (eg. how long could we wait in hub's queue to available node)
             $requestTimeoutMs = 60*60*1000 // 1 hour (same as timeout for the whole process)
@@ -278,4 +251,44 @@ protected function setupPhantomjsCapabilities(DesiredCapabilities $capabilities)
     {
         return $capabilities;
     }
+
+    /**
+     * @param AbstractTestCase $test
+     * @return DesiredCapabilities
+     */
+    private function setupCapabilities(AbstractTestCase $test)
+    {
+        $config = ConfigProvider::getInstance();
+
+        $capabilities = new DesiredCapabilities(
+            [
+                WebDriverCapabilityType::BROWSER_NAME => $config->browserName,
+                WebDriverCapabilityType::PLATFORM => WebDriverPlatform::ANY,
+                'name' => get_class($test) . '::' . $test->getName(),
+            ]
+        );
+
+        if (!empty($config->capability)) {
+            $extraCapabilities = json_decode($config->capability);
+            foreach ($extraCapabilities as $extraCapabilityName => $extraCapabilityValue) {
+                $capabilities->setCapability($extraCapabilityName, $extraCapabilityValue);
+            }
+        }
+
+        $ci = CiDetector::detect();
+        if ($ci) {
+            $capabilities->setCapability(
+                'build',
+                ConfigProvider::getInstance()->env . '-' . CiDetector::detect()->getBuildNumber()
+            );
+            $capabilities->setCapability(
+                'tags',
+                [ConfigProvider::getInstance()->env, $ci->getCiName(), get_class($test)]
+            );
+        }
+
+        $capabilities = $this->setupCustomCapabilities($capabilities, $config->browserName);
+
+        return $capabilities;
+    }
 }
diff --git a/src/Process/ProcessSetCreator.php b/src/Process/ProcessSetCreator.php
@@ -62,22 +62,22 @@ public function __construct(
      */
     public function createFromFiles(
         Finder $files,
-        array $groups = null,
-        array $excludeGroups = null,
+        array $groups,
+        array $excludeGroups,
         $filter = null,
         $ignoreDelays = false
     ) {
         $files->sortByName();
         $processSet = $this->getProcessSet();
 
         if ($this->output->isVeryVerbose()) {
-            if ($groups || $excludeGroups || !empty($filter)) {
+            if (!empty($groups) || !empty($excludeGroups) || !empty($filter)) {
                 $this->output->writeln('Filtering testcases:');
             }
-            if ($groups) {
+            if (!empty($groups)) {
                 $this->output->writeln(sprintf(' - by group(s): %s', implode(', ', $groups)));
             }
-            if ($excludeGroups) {
+            if (!empty($excludeGroups)) {
                 $this->output->writeln(sprintf(' - excluding group(s): %s', implode(', ', $excludeGroups)));
             }
             if (!empty($filter)) {
@@ -89,31 +89,10 @@ public function createFromFiles(
         /** @var SplFileInfo $file */
         foreach ($files as $file) {
             $fileName = $file->getRealPath();
-            // Parse classes from the testcase file
-            $classes = AnnotationsParser::parsePhp(\file_get_contents($fileName));
+            $className = $this->getClassNameFromFile($fileName);
+            $annotations = $this->getClassAnnotations($className, $fileName);
 
-            if (empty($classes)) {
-                throw new \RuntimeException(sprintf('No class found in file "%s"', $fileName));
-            }
-
-            // Get annotations for the first class in testcase (one file = one class)
-            try {
-                $annotations = AnnotationsParser::getAll(new \ReflectionClass(key($classes)));
-            } catch (\ReflectionException $e) {
-                throw new \RuntimeException(
-                    sprintf(
-                        'Error loading class "%s" from file "%s". Make sure the class name and namespace matches '
-                        . 'the file path.',
-                        key($classes),
-                        $fileName
-                    )
-                );
-            }
-
-            // Filter out test-cases having any of excluded groups
-            if ($excludeGroups && array_key_exists('group', $annotations)
-                && count($excludingGroups = array_intersect($excludeGroups, $annotations['group']))
-            ) {
+            if ($excludingGroups = $this->getExcludingGroups($excludeGroups, $annotations)) {
                 $this->output->writeln(
                     sprintf('Excluding testcase file %s with group %s', $fileName, implode(', ', $excludingGroups)),
                     OutputInterface::VERBOSITY_DEBUG
@@ -122,7 +101,7 @@ public function createFromFiles(
             }
 
             // Filter out test-cases without any matching group
-            if ($groups) {
+            if (!empty($groups)) {
                 if (!array_key_exists('group', $annotations)
                     || !count($matchingGroups = array_intersect($groups, $annotations['group']))
                 ) {
@@ -147,7 +126,7 @@ public function createFromFiles(
 
             $phpunitArgs = [
                 '--log-junit=logs/'
-                . Strings::webalize(key($classes), null, $lower = false)
+                . Strings::webalize($className, null, $lower = false)
                 . '.xml',
                 '--configuration=' . realpath(__DIR__ . '/../phpunit.xml'),
             ];
@@ -161,28 +140,14 @@ public function createFromFiles(
                 $phpunitArgs[] = '--colors=always';
             }
 
-            $className = key($classes);
             $processWrapper = new ProcessWrapper(
                 $this->buildProcess($fileName, $phpunitArgs),
                 $className,
                 $this->publisher
             );
 
             if (!$ignoreDelays) {
-                $delayAfter = !empty($annotations['delayAfter']) ? current($annotations['delayAfter']) : '';
-                $delayMinutes = !empty($annotations['delayMinutes']) ? current($annotations['delayMinutes']) : null;
-                if ($delayAfter) {
-                    $processWrapper->setDelay($delayAfter, $delayMinutes);
-                } elseif ($delayMinutes !== null && empty($delayAfter)) {
-                    throw new \InvalidArgumentException(
-                        sprintf(
-                            'Testcase "%s" has defined delay %d minutes, '
-                            . 'but doesn\'t have defined the testcase to run after',
-                            $className,
-                            $delayMinutes
-                        )
-                    );
-                }
+                $this->setupProcessDelays($processWrapper, $annotations);
             }
 
             $processSet->add($processWrapper);
@@ -269,4 +234,99 @@ private function encodeCapabilties(array $capabilities)
 
         return json_encode($outputCapabilities);
     }
+
+    /**
+     * Get array of groups that cause this class to be excluded.
+     *
+     * @param array $excludeGroups
+     * @param array $annotations
+     * @return array Empty if class should not be excluded.
+     */
+    private function getExcludingGroups(array $excludeGroups, array $annotations)
+    {
+        $excludingGroups = [];
+
+        if (!empty($excludeGroups) && array_key_exists('group', $annotations)) {
+            if (!empty(array_intersect($excludeGroups, $annotations['group']))) {
+                $excludingGroups = array_intersect($excludeGroups, $annotations['group']);
+            }
+        }
+
+        return $excludingGroups;
+    }
+
+    /**
+     * @param $fileName
+     * @return string
+     */
+    private function getClassNameFromFile($fileName)
+    {
+        // Parse classes from the testcase file
+        $classes = AnnotationsParser::parsePhp(\file_get_contents($fileName));
+
+        if (empty($classes)) {
+            throw new \RuntimeException(sprintf('No class found in file "%s"', $fileName));
+        }
+
+        if (count($classes) > 1) {
+            throw new \RuntimeException(
+                sprintf(
+                    'File "%s" contains definition of %d classes. However, each class must be defined in its own'
+                    . ' separate file.',
+                    $fileName,
+                    count($classes)
+                )
+            );
+        }
+
+        return key($classes);
+    }
+
+    /**
+     * @param ProcessWrapper $processWrapper
+     * @param array $annotations
+     */
+    private function setupProcessDelays(ProcessWrapper $processWrapper, array $annotations)
+    {
+        $delayAfter = !empty($annotations['delayAfter']) ? current($annotations['delayAfter']) : '';
+        $delayMinutes = !empty($annotations['delayMinutes']) ? current($annotations['delayMinutes']) : null;
+
+        if ($delayAfter) {
+            $processWrapper->setDelay($delayAfter, $delayMinutes);
+        } elseif ($delayMinutes !== null && empty($delayAfter)) {
+            throw new \InvalidArgumentException(
+                sprintf(
+                    'Testcase "%s" has defined delay %d minutes, '
+                    . 'but doesn\'t have defined the testcase to run after',
+                    $processWrapper->getClassName(),
+                    $delayMinutes
+                )
+            );
+        }
+    }
+
+    /**
+     * Get annotations for the first class in testcase (one file = one class)
+     *
+     * @param string $className
+     * @param string $fileName
+     * @return array
+     */
+    private function getClassAnnotations($className, $fileName)
+    {
+        try {
+            $annotations = AnnotationsParser::getAll(new \ReflectionClass($className));
+
+            return $annotations;
+        } catch (\ReflectionException $e) {
+            throw new \RuntimeException(
+                sprintf(
+                    'Error loading class "%s" from file "%s". Make sure the class name and namespace matches '
+                    . 'the file path.',
+                    $className,
+                    $fileName
+                )
+            );
+        }
+    }
 }