MAGETWO-44154: Random PAT build failures due to 400/503 HTTP response errors being thrown

 - add unit test coverage for GENERATION_SKIP case
 - refactor logic of including classes

lib/internal/Magento/Framework/Code/Generator.php

@@ -104,27 +104,10 @@ class Generator
                     new \Magento\Framework\Phrase(implode(' ', $errors))
                 );
             }
-            $this->tryToIncludeFile($file, $className, true);
-            return self::GENERATION_SUCCESS;
-        }
-    }
-
-    /**
-     * Include file conditionally
-     *
-     * @param string $fileName
-     * @param string $className
-     * @param bool $mustExist Skip inclusion if file doesn't exist and this is false
-     * @return void
-     */
-    public function tryToIncludeFile($fileName, $className, $mustExist)
-    {
-        if (!$this->definedClasses->isClassLoadableFromMemory($className)) {
-            if (!$mustExist && file_exists($fileName)) {
-                include $fileName;
-            } else {
-                include $fileName;
+            if (!$this->definedClasses->isClassLoadableFromMemory($className)) {
+                $this->_ioObject->includeFile($file);
             }
+            return self::GENERATION_SUCCESS;
         }
     }
 
@@ -208,7 +191,16 @@ class Generator
         if (!$resultEntityType || !$sourceClassName) {
             return self::GENERATION_ERROR;
         } else if ($this->definedClasses->isClassLoadableFromDisc($resultClass)) {
-            $this->tryToIncludeFile($this->_ioObject->getResultFileName($resultClass), $resultClass, false);
+            $generatedFileName = $this->_ioObject->getResultFileName($resultClass);
+            /**
+             * Must handle two edge cases: a competing process has generated the class and written it to disc already,
+             * or the class exists in committed code, despite matching pattern to be generated.
+             */
+            if ($this->_ioObject->fileExists($generatedFileName)
+                && !$this->definedClasses->isClassLoadableFromMemory($resultClass)
+            ) {
+                $this->_ioObject->includeFile($generatedFileName);
+            }
             return self::GENERATION_SKIP;
         } else if (!isset($this->_generatedEntities[$resultEntityType])) {
             throw new \InvalidArgumentException('Unknown generation entity.');

lib/internal/Magento/Framework/Code/Generator/Io.php

@@ -146,6 +146,18 @@ class Io
         return $this->filesystemDriver->isExists($fileName);
     }
 
+    /**
+     * Wrapper for include
+     *
+     * @param string $fileName
+     * @return mixed
+     * @codeCoverageIgnore
+     */
+    public function includeFile($fileName)
+    {
+        return include $fileName;
+    }
+
     /**
      * @param string $directory
      * @return bool

lib/internal/Magento/Framework/Code/Test/Unit/GeneratorTest.php

@@ -8,6 +8,10 @@
 
 namespace Magento\Framework\Code\Test\Unit;
 
+use Magento\Framework\Code\Generator;
+use Magento\Framework\Code\Generator\DefinedClasses;
+use Magento\Framework\Code\Generator\Io;
+
 class GeneratorTest extends \PHPUnit_Framework_TestCase
 {
     /**
@@ -34,15 +38,28 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
     protected $model;
 
     /**
-     * @var \PHPUnit_Framework_MockObject_MockObject|Generator\Io
+     * @var \PHPUnit_Framework_MockObject_MockObject|Io
      */
     protected $ioObjectMock;
 
+    /** @var \Magento\Framework\Code\Generator\DefinedClasses | \PHPUnit_Framework_MockObject_MockObject */
+    protected $definedClassesMock;
+
     protected function setUp()
     {
+        $this->definedClassesMock = $this->getMock('\Magento\Framework\Code\Generator\DefinedClasses');
         $this->ioObjectMock = $this->getMockBuilder('\Magento\Framework\Code\Generator\Io')
             ->disableOriginalConstructor()
             ->getMock();
+        $this->model = $this->buildModel(
+            $this->ioObjectMock,
+            [
+                'factory' => '\Magento\Framework\ObjectManager\Code\Generator\Factory',
+                'proxy' => '\Magento\Framework\ObjectManager\Code\Generator\Proxy',
+                'interceptor' => '\Magento\Framework\Interception\Code\Generator\Interceptor'
+            ],
+            $this->definedClassesMock
+        );
     }
 
     protected function tearDown()
@@ -52,9 +69,10 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
 
     public function testGetGeneratedEntities()
     {
-        $this->model = new \Magento\Framework\Code\Generator(
+        $this->model = $this->buildModel(
             $this->ioObjectMock,
-            ['factory', 'proxy', 'interceptor']
+            ['factory', 'proxy', 'interceptor'],
+            $this->definedClassesMock
         );
         $this->assertEquals(array_values($this->expectedEntities), $this->model->getGeneratedEntities());
     }
@@ -65,14 +83,6 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
      */
     public function testGenerateClass($className, $entityType)
     {
-        $this->model = new \Magento\Framework\Code\Generator(
-            $this->ioObjectMock,
-            [
-                'factory' => '\Magento\Framework\ObjectManager\Code\Generator\Factory',
-                'proxy' => '\Magento\Framework\ObjectManager\Code\Generator\Proxy',
-                'interceptor' => '\Magento\Framework\Interception\Code\Generator\Interceptor'
-            ]
-        );
         $objectManagerMock = $this->getMock('Magento\Framework\ObjectManagerInterface');
         $fullClassName = $className . $entityType;
         $entityGeneratorMock = $this->getMockBuilder('\Magento\Framework\Code\Generator\EntityAbstract')
@@ -85,8 +95,6 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
 
     public function testGenerateClassWithWrongName()
     {
-        $this->model = new \Magento\Framework\Code\Generator($this->ioObjectMock);
-
         $this->assertEquals(
             \Magento\Framework\Code\Generator::GENERATION_ERROR,
             $this->model->generateClass(self::SOURCE_CLASS)
@@ -98,15 +106,6 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
      */
     public function testGenerateClassWithError()
     {
-        $this->model = new \Magento\Framework\Code\Generator(
-            $this->ioObjectMock,
-            [
-                'factory' => '\Magento\Framework\ObjectManager\Code\Generator\Factory',
-                'proxy' => '\Magento\Framework\ObjectManager\Code\Generator\Proxy',
-                'interceptor' => '\Magento\Framework\Interception\Code\Generator\Interceptor'
-            ]
-        );
-
         $expectedEntities = array_values($this->expectedEntities);
         $resultClassName = self::SOURCE_CLASS . ucfirst(array_shift($expectedEntities));
         $objectManagerMock = $this->getMock('Magento\Framework\ObjectManagerInterface');
@@ -118,6 +117,32 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
         $this->model->generateClass($resultClassName);
     }
 
+    /**
+     * @dataProvider trueFalseDataProvider
+     */
+    public function testGenerateClassWithExistName($fileExists)
+    {
+        $this->definedClassesMock->expects($this->any())
+            ->method('isClassLoadableFromDisc')
+            ->willReturn(true);
+
+        $resultClassFileName = '/Magento/Path/To/Class.php';
+        $this->ioObjectMock->expects($this->once())->method('getResultFileName')->willReturn($resultClassFileName);
+        $this->ioObjectMock->expects($this->once())->method('fileExists')->willReturn($fileExists);
+        $includeFileInvokeCount = $fileExists ? 1 : 0;
+        $this->ioObjectMock->expects($this->exactly($includeFileInvokeCount))->method('includeFile');
+
+        $this->assertEquals(
+            \Magento\Framework\Code\Generator::GENERATION_SKIP,
+            $this->model->generateClass('Magento\GeneratedClass\Factory')
+        );
+    }
+
+    public function trueFalseDataProvider()
+    {
+        return [[true], [false]];
+    }
+
     /**
      * Data provider for generate class tests
      *
@@ -135,4 +160,17 @@ class GeneratorTest extends \PHPUnit_Framework_TestCase
         }
         return $data;
     }
+
+    /**
+     * Build SUT object
+     *
+     * @param Io $ioObject
+     * @param array $generatedEntities
+     * @param DefinedClasses $definedClasses
+     * @return Generator
+     */
+    private function buildModel(Io $ioObject, array $generatedEntities, DefinedClasses $definedClasses)
+    {
+        return new Generator($ioObject, $generatedEntities, $definedClasses);
+    }
 }