Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add input file configuration parsing for spec tests #4662

Merged
merged 13 commits into from
May 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
lib/optimizer/resources/local_fallback/* linguist-generated=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about using .gitattributes to suppress diffs for generated code (docs) 😄.

Would the path lib/optimiser/ be a better place for this file?

Copy link
Member

@westonruter westonruter May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. Placing it into lib/optimizer will work, per Git docs:

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as GitHub recognizes this, as their docs only mention the repo root.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed it would only work in the project root, but I can try moving it around to see what happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't work when the .gitattributes file is in a subfolder. I'll revert that change again and leave it in the root for now. This will need to be adapted once we extract the libraries into separate repositories.

Image 2020-05-07 at 8 29 08 AM

lib/optimizer/tests/spec/* linguist-generated=true
2 changes: 1 addition & 1 deletion lib/optimizer/resources/local_fallback/rtv/metadata
Original file line number Diff line number Diff line change
@@ -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"}
4 changes: 2 additions & 2 deletions lib/optimizer/resources/local_fallback/v0.css

Large diffs are not rendered by default.

19 changes: 19 additions & 0 deletions lib/optimizer/src/Configuration/AmpRuntimeCssConfiguration.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -42,6 +53,7 @@ protected function getAllowedKeys()
{
return [
self::VERSION => '',
self::STYLES => '',
self::CANARY => false,
];
}
Expand All @@ -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));
Expand Down
18 changes: 13 additions & 5 deletions lib/optimizer/src/Transformer/AmpRuntimeCss.php
Original file line number Diff line number Diff line change
Expand Up @@ -141,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);
}
}

Expand All @@ -162,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 (200 < $statusCode || $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;
}

/**
Expand Down
106 changes: 86 additions & 20 deletions lib/optimizer/tests/SpecTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -34,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 = [];
Expand Down Expand Up @@ -108,33 +108,99 @@ 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());
}

/**
* Map spec test input file configuration data to configuration arguments as needed by the PHP transformers.
*
* @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.
*/
public function mapConfigurationData($configurationData)
{
$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}.");
}
}

return new Configuration($mappedConfiguration);
}

/**
* 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 [];
}

$json = trim($matches['json']);
$source = substr($source, strlen($matches[0]));

if (empty($json)) {
return [];
}

$configurationData = (array)json_decode($json, true);
if (empty($configurationData) || json_last_error() !== JSON_ERROR_NONE) {
return [];
}

return $configurationData;
}

/**
* Get the transformer to test.
*
* @param string $scenario Test scenario.
* @param string $transformerClass Class of the transformer to get.
* @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)
private function getTransformer($scenario, $transformerClass, $configuration)
{
$stubbedRequests = TestMarkup::STUBBED_REMOTE_REQUESTS;

if (array_key_exists($scenario, self::STUBBED_REMOTE_REQUESTS_FOR_TESTS)) {
$stubbedRequests = array_merge($stubbedRequests, self::STUBBED_REMOTE_REQUESTS_FOR_TESTS[$scenario]);
}

$transformationEngine = new TransformationEngine(
new Configuration(),
$configuration,
new StubbedRemoteGetRequest($stubbedRequests)
);

Expand Down
32 changes: 31 additions & 1 deletion lib/optimizer/tests/Transformer/AmpRuntimeCssTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
],
];
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<!doctype html>
<html i-amphtml-layout i-amphtml-no-boilerplate transformed="self;v=1">
<head>
<meta charset="utf-8"><style amp-runtime i-amphtml-version="123456789000000">/* v0.css */</style>
<meta charset="utf-8"><style amp-runtime i-amphtml-version="123456789000000">/* example.com v0.css */</style>
<meta name="viewport" content="width=device-width,minimum-scale=1">
<link rel="preload" href="/amp/rtv/123456789000000/v0.js" as="script">
<meta name="runtime-host" content="/amp">
<link rel="preload" href="https://example.com/amp/rtv/123456789000000/v0.js" as="script">
<meta name="runtime-host" content="https://example.com/amp">
<meta name="amp-geo-api" content="/geo">
<script async src="/amp/rtv/123456789000000/v0.js"></script>
<script async custom-element="amp-experiment" src="/amp/rtv/123456789000000/v0/amp-experiment-0.1.js"></script>
<script async custom-element="amp-video" src="/amp/rtv/123456789000000/v0/amp-video-0.1.js"></script>
<script async custom-template="amp-mustache" src="/amp/rtv/123456789000000/v0/amp-mustache-0.1.js"></script>
<script async src="https://example.com/amp/rtv/123456789000000/v0.js"></script>
<script async custom-element="amp-experiment" src="https://example.com/amp/rtv/123456789000000/v0/amp-experiment-0.1.js"></script>
<script async custom-element="amp-video" src="https://example.com/amp/rtv/123456789000000/v0/amp-video-0.1.js"></script>
<script async custom-template="amp-mustache" src="https://example.com/amp/rtv/123456789000000/v0/amp-mustache-0.1.js"></script>
<link rel="canonical" href="self.html"><style amp-custom>h1{margin:16px}</style>
<link rel="amphtml" href="https://example.com/amp-version.html">
<link rel="amphtml" href="/amp-version.html">
</head>
<body>
<h1>Hello, AMP world!</h1>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<!doctype html>
<html ⚡ i-amphtml-layout i-amphtml-no-boilerplate transformed="self;v=1">
<head>
<meta charset="utf-8"><style amp-runtime i-amphtml-version="123456789000000">/* v0.css */</style>
<meta charset="utf-8"><style amp-runtime i-amphtml-version="123456789000000">/* ampproject.org/rtv v0.css */</style>
<meta name="viewport" content="width=device-width,minimum-scale=1">
<link rel="preload" href="https://cdn.ampproject.org/v0.js" as="script">
<script async src="https://cdn.ampproject.org/v0.js"></script>
Expand Down
5 changes: 3 additions & 2 deletions lib/optimizer/tests/spec/end-to-end/hello-world/input.html
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
<!--
{
"ampUrlPrefix": "/amp",
"ampUrlPrefix": "https://example.com/amp",
"ampRuntimeVersion": "123456789000000",
"ampUrl": "https://example.com/amp-version.html",
"ampUrl": "/amp-version.html",
"rtv": true,
"geoApiUrl": "/geo"
}
-->
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!doctype html>
<html ⚡>
<head>
<script async nomodule src="https://cdn.ampproject.org/v0/amp-experiment-0.1.js"></script>
<script async custom-element="amp-experiment" src="https://cdn.ampproject.org/v0/amp-experiment-0.1.mjs" type="module" crossorigin="anonymous"></script>
<link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="preload">
<script async nomodule src="https://cdn.ampproject.org/v0.js"></script>
<script async src="https://cdn.ampproject.org/v0.mjs" type="module" crossorigin="anonymous"></script>
<link rel="stylesheet" href="https://cdn.ampproject.org/v0.css">
<link href="https://fonts.googleapis.com/css?foobar" rel="stylesheet" type="text/css">
<link href="https://example.com/favicon.ico" rel="icon">
<link rel="preload" href="https://cdn.ampproject.org/v0.css" as="style">
</head>
<body></body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!doctype html>
<html ⚡>
<head>
<script async custom-element=amp-experiment src=https://cdn.ampproject.org/v0/amp-experiment-0.1.js></script>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<link rel=stylesheet href=https://cdn.ampproject.org/v0.css>
<link href=https://fonts.googleapis.com/css?foobar rel=stylesheet type=text/css>
<link href=https://example.com/favicon.ico rel=icon>
</head>
<body></body>
</html>
Loading