Add input file configuration parsing for spec tests (#4662)

* Update spec files

* Adapt spec test to extract configuration arguments from input files

* Add STYLES configuration key to AmpRuntimeCss transformer

* Nake use of styles provided via config if available

* Remove runtime style tag if stylesheet is linked

* Add more tests to assert runtime transformer behavior

* Complete stubbed requests data

* Remove unused import

* Add git attributes file to mark certain file as being generated

* Use substr() instead of a replacement for removing the leading comment

* Remove redundant JSONOBJECT_AS_ARRAY constant

* Move .gitattributes file into lib/optimizer folder

* Revert "Move .gitattributes file into lib/optimizer folder"

.gitattributes

@@ -0,0 +1,2 @@
+lib/optimizer/resources/local_fallback/* linguist-generated=true
+lib/optimizer/tests/spec/*               linguist-generated=true

lib/optimizer/resources/local_fallback/rtv/metadata

@@ -1 +1 @@
-{"ampRuntimeVersion":"012004030010070","ampCssUrl":"https://cdn.ampproject.org/rtv/012004030010070/v0.css","canaryPercentage":"0.005","diversions":["002004012111560","002004012158290","022004030010070","032004012111560","032004012158290","042004030033570"],"ltsRuntimeVersion":"012002251816300","ltsCssUrl":"https://cdn.ampproject.org/rtv/012002251816300/v0.css"}
+{"ampRuntimeVersion":"012004252135000","ampCssUrl":"https://cdn.ampproject.org/rtv/012004252135000/v0.css","canaryPercentage":"0.005","diversions":["002004252135000","002005050322000","022004252135000","032004252135000","032005050322000","042005051629000","052004252135000","102004252135000"],"ltsRuntimeVersion":"012004030010070","ltsCssUrl":"https://cdn.ampproject.org/rtv/012004030010070/v0.css"}

lib/optimizer/src/Configuration/AmpRuntimeCssConfiguration.php

@@ -20,10 +20,21 @@ final class AmpRuntimeCssConfiguration extends BaseTransformerConfiguration
     /**
      * Configuration key that holds the version number to use.
      *
+     * If the version is not provided, the latest runtime version is fetched from cdn.ampproject.org.
+     *
      * @var string
      */
     const VERSION = 'version';
 
+    /**
+     * Configuration key that holds the actual runtime CSS styles to use.
+     *
+     * If the styles are not provided, the latest runtime styles are fetched from cdn.ampproject.org.
+     *
+     * @var string
+     */
+    const STYLES = 'styles';
+
     /**
      * Configuration key that holds the flag for the canary version of the runtime style.
      *
@@ -42,6 +53,7 @@ protected function getAllowedKeys()
     {
         return [
             self::VERSION => '',
+            self::STYLES  => '',
             self::CANARY  => false,
         ];
     }
@@ -63,6 +75,13 @@ protected function validate($key, $value)
                 $value = trim($value);
                 break;
 
+            case self::STYLES:
+                if (! is_string($value)) {
+                    throw InvalidConfigurationValue::forInvalidSubValueType(self::class, self::STYLES, 'string', gettype($value));
+                }
+                $value = trim($value);
+                break;
+
             case self::CANARY:
                 if (! is_bool($value)) {
                     throw InvalidConfigurationValue::forInvalidSubValueType(self::class, self::CANARY, 'boolean', gettype($value));

lib/optimizer/src/Transformer/AmpRuntimeCss.php

@@ -5,11 +5,9 @@
 use AmpProject\Amp;
 use AmpProject\Attribute;
 use AmpProject\Dom\Document;
-use AmpProject\Optimizer\Configurable;
 use AmpProject\Optimizer\Configuration\AmpRuntimeCssConfiguration;
 use AmpProject\Optimizer\Error;
 use AmpProject\Optimizer\ErrorCollection;
-use AmpProject\Optimizer\MakesRemoteRequests;
 use AmpProject\Optimizer\TransformerConfiguration;
 use AmpProject\RemoteGetRequest;
 use AmpProject\Optimizer\Transformer;
@@ -36,7 +34,7 @@
  *
  * @package ampproject/optimizer
  */
-final class AmpRuntimeCss implements Transformer, Configurable, MakesRemoteRequests
+final class AmpRuntimeCss implements Transformer
 {
 
     /**
@@ -61,29 +59,29 @@ final class AmpRuntimeCss implements Transformer, Configurable, MakesRemoteReque
     const V0_CSS_URL = Amp::CACHE_HOST . '/' . self::V0_CSS;
 
     /**
-     * Transport to use for remote requests.
+     * Configuration store to use.
      *
-     * @var RemoteGetRequest
+     * @var TransformerConfiguration
      */
-    private $remoteRequest;
+    private $configuration;
 
     /**
-     * Configuration store to use.
+     * Transport to use for remote requests.
      *
-     * @var TransformerConfiguration
+     * @var RemoteGetRequest
      */
-    private $configuration;
+    private $remoteRequest;
 
     /**
      * Instantiate an AmpRuntimeCss object.
      *
-     * @param RemoteGetRequest         $remoteRequest Transport to use for remote requests.
      * @param TransformerConfiguration $configuration Configuration store to use.
+     * @param RemoteGetRequest         $remoteRequest Transport to use for remote requests.
      */
-    public function __construct(RemoteGetRequest $remoteRequest, TransformerConfiguration $configuration)
+    public function __construct(TransformerConfiguration $configuration, RemoteGetRequest $remoteRequest)
     {
-        $this->remoteRequest = $remoteRequest;
         $this->configuration = $configuration;
+        $this->remoteRequest = $remoteRequest;
     }
 
     /**
@@ -143,6 +141,7 @@ private function addStaticCss(Document $document, DOMElement $ampRuntimeStyle, E
         } catch (Exception $exception) {
             $errors->add(Error\CannotInlineRuntimeCss::fromException($exception, $ampRuntimeStyle, $version));
             $this->linkCss($document, $ampRuntimeStyle);
+            $ampRuntimeStyle->parentNode->removeChild($ampRuntimeStyle);
         }
     }
 
@@ -164,14 +163,21 @@ private function inlineCss(DOMElement $ampRuntimeStyle, $version)
         }
 
         $ampRuntimeStyle->setAttribute(Attribute::I_AMPHTML_VERSION, $version);
-        $response   = $this->remoteRequest->get($v0CssUrl);
-        $statusCode = $response->getStatusCode();
 
-        if ($statusCode < 200 || $statusCode >= 300) {
-            return;
+        $styles = $this->configuration->get(AmpRuntimeCssConfiguration::STYLES);
+
+        if (empty($styles)) {
+            $response   = $this->remoteRequest->get($v0CssUrl);
+            $statusCode = $response->getStatusCode();
+
+            if (200 < $statusCode || $statusCode >= 300) {
+                return;
+            }
+
+            $styles = $response->getBody();
         }
 
-        $ampRuntimeStyle->textContent = $response->getBody();
+        $ampRuntimeStyle->textContent = $styles;
     }
 
     /**

lib/optimizer/tests/SpecTest.php

@@ -3,6 +3,7 @@
 namespace AmpProject\Optimizer;
 
 use AmpProject\Dom\Document;
+use AmpProject\Optimizer\Configuration\AmpRuntimeCssConfiguration;
 use AmpProject\Optimizer\Tests\MarkupComparison;
 use AmpProject\Optimizer\Tests\TestMarkup;
 use AmpProject\Optimizer\Transformer\AmpRuntimeCss;
@@ -11,6 +12,8 @@
 use AmpProject\RemoteRequest\StubbedRemoteGetRequest;
 use DirectoryIterator;
 use PHPUnit\Framework\TestCase;
+use ReflectionClass;
+use ReflectionException;
 
 /**
  * Test the individual transformers against the NodeJS spec test suite.
@@ -32,20 +35,19 @@ final class SpecTest extends TestCase
     const CLASS_SKIP_TEST = '__SKIP__';
 
     /**
-     * Associative array of mapping data for stubbing remote requests for specific tests.
+     * Regular expression to match the leading HTML comment that provides the configuration for a spec test.
      *
-     * @todo This is a temporary fix only to get the test to pass with our current transformer.
-     *       We'll need to adapt the transformer to take the following changes into account:
-     *       https://github.com/ampproject/amp-toolbox/commit/b154a73c6dc9231e4060434c562a90d983e2a46d
+     * @see https://regex101.com/r/ImDtxI/2
      *
-     * @var array
+     * @var string
      */
-    const STUBBED_REMOTE_REQUESTS_FOR_TESTS = [
-        'AmpRuntimeCss - always_inlines_v0css' => [
-            'https://cdn.ampproject.org/v0.css' => '/* v0-prod.css */',
-        ],
-    ];
+    const LEADING_HTML_COMMENT_REGEX_PATTERN = '/^\s*<!--\s*(?<json>{(?>[^}]*})*)\s*-->/';
 
+    /**
+     * Provide the data for running the spec tests.
+     *
+     * @return array Scenarios to test.
+     */
     public function dataTransformerSpecFiles()
     {
         $scenarios = [];
@@ -106,41 +108,119 @@ public function testTransformerSpecFiles($scenario, $transformerClass, $source,
             $this->markTestSkipped("Skipping {$source}, {$expected}");
         }
 
+        $configuration = $this->mapConfigurationData($this->extractConfigurationData($source));
+
         $document = Document::fromHtmlFragment($source);
 
-        $transformer = $this->getTransformer($scenario, $transformerClass);
-        $errors      = new ErrorCollection();
+        $transformer   = $this->getTransformer($scenario, $transformerClass, $configuration);
+        $errors        = new ErrorCollection();
 
         $transformer->transform($document, $errors);
 
         $this->assertSimilarMarkup($expected, $document->saveHTMLFragment());
     }
 
     /**
-     * Get the transformer to test.
+     * Map spec test input file configuration data to configuration arguments as needed by the PHP transformers.
      *
-     * @param string $scenario         Test scenario.
-     * @param string $transformerClass Class of the transformer to get.
-     * @return Transformer Instantiated transformer object.
+     * @param array $configurationData Associative array of configuration data coming from the spec test input file.
+     * @return Configuration Configuration object to use for the transformation engine.
      */
-    private function getTransformer($scenario, $transformerClass)
+    public function mapConfigurationData($configurationData)
     {
-        $arguments = [];
+        $mappedConfiguration = [];
+
+        foreach ($configurationData as $key => $value) {
+            switch ($key) {
+                case 'ampRuntimeStyles':
+                    $mappedConfiguration[AmpRuntimeCss::class][AmpRuntimeCssConfiguration::STYLES] = $value;
+                    break;
+                case 'ampRuntimeVersion':
+                    $mappedConfiguration[AmpRuntimeCss::class][AmpRuntimeCssConfiguration::VERSION] = $value;
+                    break;
+
+                // @TODO: Known configuration arguments used in spec tests that are not implemented yet.
+                case 'ampUrlPrefix':
+                case 'ampUrl':
+                case 'canonical':
+                case 'experimentBindAttribute':
+                case 'geoApiUrl':
+                case 'lts':
+                case 'preloadHeroImage':
+                case 'rtv':
+                default:
+                    $this->fail("Configuration argument not yet implemented: {$key}.");
+            }
+        }
 
-        if (is_a($transformerClass, MakesRemoteRequests::class, true)) {
-            $stubbedRequests = TestMarkup::STUBBED_REMOTE_REQUESTS;
+        return new Configuration($mappedConfiguration);
+    }
 
-            if (array_key_exists($scenario, self::STUBBED_REMOTE_REQUESTS_FOR_TESTS)) {
-                $stubbedRequests = array_merge($stubbedRequests, self::STUBBED_REMOTE_REQUESTS_FOR_TESTS[$scenario]);
-            }
+    /**
+     * Parse the input source file and extract the configuration data.
+     *
+     * Input HTML files can contain a leading HTML comment that provides configuration arguments in the form of a JSON
+     * object.
+     *
+     * @param string $source Input source file to parse for a configuration snippet.
+     * @return array Associative array of configuration data found in the input HTML file.
+     */
+    public function extractConfigurationData(&$source)
+    {
+        $matches = [];
+        if (!preg_match(self::LEADING_HTML_COMMENT_REGEX_PATTERN, $source, $matches)) {
+            return [];
+        }
 
-            $arguments[] = new StubbedRemoteGetRequest($stubbedRequests);
+        $json   = trim($matches['json']);
+        $source = substr($source, strlen($matches[0]));
+
+        if (empty($json)) {
+            return [];
         }
 
-        if (is_a($transformerClass, Configurable::class, true)) {
-            $arguments[] = (new Configuration())->getTransformerConfiguration($transformerClass);
+        $configurationData = (array)json_decode($json, true);
+        if (empty($configurationData) || json_last_error() !== JSON_ERROR_NONE) {
+            return [];
         }
 
-        return new $transformerClass(...$arguments);
+        return $configurationData;
+    }
+
+    /**
+     * Get the transformer to test.
+     *
+     * @param string        $scenario         Test scenario.
+     * @param string        $transformerClass Class of the transformer to get.
+     * @param Configuration $configuration    Configuration to use.
+     * @return Transformer Instantiated transformer object.
+     */
+    private function getTransformer($scenario, $transformerClass, $configuration)
+    {
+        $stubbedRequests = TestMarkup::STUBBED_REMOTE_REQUESTS;
+
+        $transformationEngine = new TransformationEngine(
+            $configuration,
+            new StubbedRemoteGetRequest($stubbedRequests)
+        );
+
+        return new $transformerClass(...$this->callPrivateMethod($transformationEngine, 'getTransformerDependencies', [$transformerClass]));
+    }
+
+    /**
+     * Call a private method as if it was public.
+     *
+     * @param object|string $object     Object instance or class string to call the method of.
+     * @param string        $methodName Name of the method to call.
+     * @param array         $args       Optional. Array of arguments to pass to the method.
+     * @return mixed Return value of the method call.
+     * @throws ReflectionException If the object could not be reflected upon.
+     */
+    private function callPrivateMethod($object, $methodName, $args = [])
+    {
+        $method = (new ReflectionClass($object))->getMethod($methodName);
+        $method->setAccessible(true);
+
+        return $method->invokeArgs($object, $args);
     }
 }

lib/optimizer/tests/Transformer/AmpRuntimeCssTest.php

@@ -42,7 +42,37 @@ public function dataTransform()
                 '<style amp-runtime="" i-amphtml-version="023456789000000">/* v0.css */</style>' .
                 '</head><body></body></html>',
 
-                ['canary' => true],
+                [AmpRuntimeCssConfiguration::CANARY => true],
+            ],
+
+            'override version via config' => [
+                TestMarkup::DOCTYPE . '<html><head>' . TestMarkup::META_CHARSET . TestMarkup::STYLE_AMPRUNTIME . '</head></html>',
+
+                TestMarkup::DOCTYPE . '<html><head>' . TestMarkup::META_CHARSET .
+                '<style amp-runtime="" i-amphtml-version="012345678901234">/* 012345678901234/v0.css */</style>' .
+                '</head><body></body></html>',
+
+                [AmpRuntimeCssConfiguration::VERSION => '012345678901234'],
+            ],
+
+            'override styles via config' => [
+                TestMarkup::DOCTYPE . '<html><head>' . TestMarkup::META_CHARSET . TestMarkup::STYLE_AMPRUNTIME . '</head></html>',
+
+                TestMarkup::DOCTYPE . '<html><head>' . TestMarkup::META_CHARSET .
+                '<style amp-runtime="" i-amphtml-version="012345678900000">/* custom-styles-v0.css */</style>' .
+                '</head><body></body></html>',
+
+                [AmpRuntimeCssConfiguration::STYLES => '/* custom-styles-v0.css */'],
+            ],
+
+            'trying to inline invalid version results in linked stylesheet' => [
+                TestMarkup::DOCTYPE . '<html><head>' . TestMarkup::META_CHARSET . TestMarkup::STYLE_AMPRUNTIME . '</head></html>',
+
+                TestMarkup::DOCTYPE . '<html><head>' . TestMarkup::META_CHARSET .
+                '<link rel="stylesheet" href="https://cdn.ampproject.org/v0.css">' .
+                '</head><body></body></html>',
+
+                [AmpRuntimeCssConfiguration::VERSION => '0000000000000000'],
             ],
         ];
     }

lib/optimizer/tests/spec/end-to-end/body-only/expected_output.valid.html

@@ -1,7 +1,7 @@
 <!doctype html>
 <html amp i-amphtml-layout i-amphtml-no-boilerplate transformed="self;v=1">
 <head>
-  <meta data-auto charset="utf-8"><style amp-runtime i-amphtml-version="123456789000000">/* v0.css */</style>
+  <meta data-auto charset="utf-8"><style amp-runtime i-amphtml-version="123456789000000">/* ampproject.org/rtv v0.css */</style>
   <meta data-auto name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
   <link rel="preload" href="https://cdn.ampproject.org/v0.js" as="script">
   <script data-auto async src="https://cdn.ampproject.org/v0.js"></script>