Merge pull request #329 from ampproject/fix/prepare-tests-for-process…

…-isolation
ampproject · Aug 25, 2021 · 715d077 · 715d077
2 parents e0e6899 + 270f29a
commit 715d077
Show file tree

Hide file tree

Showing 10 changed files with 170 additions and 91 deletions.
diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist
@@ -40,6 +40,11 @@
         <exclude-pattern>src/Validator/Spec/Tag/*_.php</exclude-pattern>
     </rule>
 
+    <!-- Method names for overrides need to stick with their signature independently of the standard. -->
+    <rule ref="PSR1.Methods.CamelCapsMethodName.NotCamelCaps">
+        <exclude-pattern>tests/Validator/Spec/Section/*.php</exclude-pattern>
+    </rule>
+
     <!-- WordPress Coding Standards for Inline Documentation and Comments -->
     <rule ref="WordPress-Docs">
         <!-- This does not allow for useful @see and @todo tags with surrounding remarks. -->

diff --git a/tests/Dom/DocumentTest.php b/tests/Dom/DocumentTest.php
@@ -874,76 +874,79 @@ public function testHasInitialAmpDevMode($document, $hasInitialDevMode)
      */
     public function getGetElementIdData()
     {
-        $elementFactory = static function ($dom, $id = null) {
-            $element = $dom->createElement('div');
-
-            if ($id) {
-                $element->setAttribute('id', $id);
-            }
-
-            $dom->body->appendChild($element);
-
-            return $element;
-        };
-
         return [
             'single check with existing ID'         => [
                 [
-                    [ $elementFactory, 'my-id', 'some-prefix', 'my-id' ],
+                    [ true, 'my-id', 'some-prefix', 'my-id' ],
                 ],
             ],
 
             'single check without existing ID'      => [
                 [
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
+                    [ true, null, 'some-prefix', 'some-prefix-0' ],
                 ],
             ],
 
             'consecutive checks count upwards'      => [
                 [
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-1' ],
+                    [ true, null, 'some-prefix', 'some-prefix-0' ],
+                    [ true, null, 'some-prefix', 'some-prefix-1' ],
                 ],
             ],
 
             'consecutive checks for same element return same ID' => [
                 [
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
-                    [ null, null, 'some-prefix', 'some-prefix-0' ],
+                    [ true, null, 'some-prefix', 'some-prefix-0' ],
+                    [ false, null, 'some-prefix', 'some-prefix-0' ],
                 ],
             ],
 
             'mixing prefixes keeps counts separate' => [
                 [
-                    [ $elementFactory, 'my-id', 'some-prefix', 'my-id' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-1' ],
-                    [ $elementFactory, null, 'other-prefix', 'other-prefix-0' ],
-                    [ $elementFactory, null, 'other-prefix', 'other-prefix-1' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-2' ],
-                    [ $elementFactory, 'another-id', 'some-prefix', 'another-id' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-3' ],
-                    [ null, null, 'some-prefix', 'some-prefix-3' ],
+                    [ true, 'my-id', 'some-prefix', 'my-id' ],
+                    [ true, null, 'some-prefix', 'some-prefix-0' ],
+                    [ true, null, 'some-prefix', 'some-prefix-1' ],
+                    [ true, null, 'other-prefix', 'other-prefix-0' ],
+                    [ true, null, 'other-prefix', 'other-prefix-1' ],
+                    [ true, null, 'some-prefix', 'some-prefix-2' ],
+                    [ true, 'another-id', 'some-prefix', 'another-id' ],
+                    [ true, null, 'some-prefix', 'some-prefix-3' ],
+                    [ false, null, 'some-prefix', 'some-prefix-3' ],
                 ],
             ],
         ];
     }
 
+    private function elementFactory($dom, $id = null)
+    {
+        $element = $dom->createElement('div');
+
+        if ($id) {
+            $element->setAttribute('id', $id);
+        }
+
+        $dom->body->appendChild($element);
+
+        return $element;
+    }
+
     /**
      * Test Document::getElementId().
      *
      * @dataProvider getGetElementIdData
      * @covers \AmpProject\Dom\Document::getElementId()
      *
-     * @param array $checks Checks to perform. Each check is an array containing an element, a prefix and an expected ID.
+     * @param array $checks Checks to perform. Each check is an array containing a boolean whether create a new element,
+     *                      a prefix and an expected ID.
      */
     public function testGetElementId($checks)
     {
-        $dom = new Document();
-        foreach ($checks as list($elementFactory, $id, $prefix, $expected)) {
+        $dom     = new Document();
+        $element = null;
+        foreach ($checks as list($newElement, $id, $prefix, $expected)) {
             // If no element factory was passed, just reuse the previous element.
-            if ($elementFactory) {
-                $element = $elementFactory($dom, $id);
+            if ($newElement) {
+                $element = $this->elementFactory($dom, $id);
             }
 
             $actual = $dom->getElementId($element, $prefix);

diff --git a/tests/Optimizer/Transformer/TransformedIdentifierTest.php b/tests/Optimizer/Transformer/TransformedIdentifierTest.php
@@ -28,57 +28,23 @@ final class TransformedIdentifierTest extends TestCase
      */
     public function dataTransform()
     {
-        $input = static function ($html) {
-            return TestMarkup::DOCTYPE . $html . '<head>'
-                   . TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT . TestMarkup::SCRIPT_AMPRUNTIME
-                   . TestMarkup::LINK_FAVICON . TestMarkup::LINK_CANONICAL . TestMarkup::STYLE_AMPBOILERPLATE . TestMarkup::NOSCRIPT_AMPBOILERPLATE
-                   . '</head><body></body></html>';
-        };
-
-        $expected = static function ($html) {
-            return TestMarkup::DOCTYPE . $html . '<head>'
-                   . TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT . TestMarkup::SCRIPT_AMPRUNTIME
-                   . TestMarkup::LINK_FAVICON . TestMarkup::LINK_CANONICAL . TestMarkup::STYLE_AMPBOILERPLATE . TestMarkup::NOSCRIPT_AMPBOILERPLATE
-                   . '</head><body></body></html>';
-        };
-
         return [
             'adds identifier with default version to html tag' => [
-                $input('<html ⚡>'),
-                $expected('<html ⚡ transformed="self;v=1">'),
+                '<html ⚡>',
+                '<html ⚡ transformed="self;v=1">',
             ],
 
             'adds identifier with custom version to html tag' => [
-                $input('<html ⚡>'),
-                $expected('<html ⚡ transformed="self;v=5">'),
+                '<html ⚡>',
+                '<html ⚡ transformed="self;v=5">',
                 [ 'version' => 5 ],
             ],
 
             'adds identifier without version to html tag' => [
-                $input('<html ⚡>'),
-                $expected('<html ⚡ transformed="self">'),
+                '<html ⚡>',
+                '<html ⚡ transformed="self">',
                 [ 'version' => 0 ],
             ],
-
-            'enforces CSS max byte count by default' => [
-                $input('<html amp style="color:black">'),
-                $expected('<html amp style="color:black" transformed="self;v=1">'),
-                [],
-                function (Document $document) {
-                    $this->assertTrue($document->isCssMaxByteCountEnforced());
-                    $this->assertEquals(AmpTransformed::SPEC[SpecRule::MAX_BYTES] - strlen('color:black'), $document->getRemainingCustomCssSpace());
-                }
-            ],
-
-            'allows skipping enforcement of CSS max byte count' => [
-                $input('<html data-ampdevmode>'),
-                $expected('<html data-ampdevmode transformed="self;v=1">'),
-                [ 'enforcedCssMaxByteCount' => false ],
-                function (Document $document) {
-                    $this->assertFalse($document->isCssMaxByteCountEnforced());
-                    $this->assertEquals(PHP_INT_MAX, $document->getRemainingCustomCssSpace());
-                }
-            ],
         ];
     }
 
@@ -95,15 +61,72 @@ function (Document $document) {
      */
     public function testTransform($source, $expectedHtml, $config = [], $assert = null)
     {
-        $document    = Document::fromHtml($source);
+        $document    = Document::fromHtml($this->fullHtml($source));
         $transformer = new TransformedIdentifier(new TransformedIdentifierConfiguration($config));
         $errors      = new ErrorCollection();
 
         $transformer->transform($document, $errors);
 
-        $this->assertEqualMarkup($expectedHtml, $document->saveHTML());
+        $this->assertEqualMarkup($this->fullHtml($expectedHtml), $document->saveHTML());
         if ($assert) {
             $assert($document);
         }
     }
+
+    /**
+     * @covers \AmpProject\Optimizer\Transformer\TransformedIdentifier::transform()
+     */
+    public function testItEnforcesCssMaxByteCountByDefault()
+    {
+        $document    = Document::fromHtml($this->fullHtml('<html amp style="color:black">'));
+        $transformer = new TransformedIdentifier(new TransformedIdentifierConfiguration([]));
+        $errors      = new ErrorCollection();
+
+        $transformer->transform($document, $errors);
+
+        $this->assertEqualMarkup(
+            $this->fullHtml('<html amp style="color:black" transformed="self;v=1">'),
+            $document->saveHTML()
+        );
+        $this->assertTrue($document->isCssMaxByteCountEnforced());
+        $this->assertEquals(
+            AmpTransformed::SPEC[SpecRule::MAX_BYTES] - strlen('color:black'),
+            $document->getRemainingCustomCssSpace()
+        );
+    }
+
+    /**
+     * @covers \AmpProject\Optimizer\Transformer\TransformedIdentifier::transform()
+     */
+    public function testItAllowsSkippingEnforcementOfCssMaxByteCount()
+    {
+        $document    = Document::fromHtml($this->fullHtml('<html data-ampdevmode>'));
+        $transformer = new TransformedIdentifier(
+            new TransformedIdentifierConfiguration([ 'enforcedCssMaxByteCount' => false ])
+        );
+        $errors      = new ErrorCollection();
+
+        $transformer->transform($document, $errors);
+
+        $this->assertEqualMarkup(
+            $this->fullHtml('<html data-ampdevmode transformed="self;v=1">'),
+            $document->saveHTML()
+        );
+        $this->assertFalse($document->isCssMaxByteCountEnforced());
+        $this->assertEquals(PHP_INT_MAX, $document->getRemainingCustomCssSpace());
+    }
+
+    /**
+     * Get the full HTML document based on the HTML tag to use.
+     *
+     * @param string $html HTML tag to use.
+     * @return string Full HTML document to use.
+     */
+    private function fullHtml($html)
+    {
+        return TestMarkup::DOCTYPE . $html . '<head>'
+               . TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT . TestMarkup::SCRIPT_AMPRUNTIME
+               . TestMarkup::LINK_FAVICON . TestMarkup::LINK_CANONICAL . TestMarkup::STYLE_AMPBOILERPLATE . TestMarkup::NOSCRIPT_AMPBOILERPLATE
+               . '</head><body></body></html>';
+    }
 }
diff --git a/tests/Validator/Spec/Section/AttributeListsTest.php b/tests/Validator/Spec/Section/AttributeListsTest.php
@@ -18,10 +18,17 @@ class AttributeListsTest extends TestCase
     /** @var AttributeLists */
     private $attributeLists;
 
-    public function __construct()
+    /**
+     * Sets up the fixture, for example, open a network connection.
+     *
+     * This method is called before each test.
+     *
+     * @return void
+     */
+    protected function set_up()
     {
-        parent::__construct();
-        $spec       = new Spec();
+        parent::set_up();
+        $spec                 = new Spec();
         $this->attributeLists = $spec->attributeLists();
     }
 

diff --git a/tests/Validator/Spec/Section/CssSpecRulesTest.php b/tests/Validator/Spec/Section/CssSpecRulesTest.php
@@ -19,9 +19,16 @@ class CssSpecRulesTest extends TestCase
     /** @var CssRulesets */
     private $cssRulesets;
 
-    public function __construct()
+    /**
+     * Sets up the fixture, for example, open a network connection.
+     *
+     * This method is called before each test.
+     *
+     * @return void
+     */
+    protected function set_up()
     {
-        parent::__construct();
+        parent::set_up();
         $spec              = new Spec();
         $this->cssRulesets = $spec->cssRulesets();
     }

diff --git a/tests/Validator/Spec/Section/DeclarationListsTest.php b/tests/Validator/Spec/Section/DeclarationListsTest.php
@@ -17,10 +17,17 @@ class DeclarationListsTest extends TestCase
     /** @var DeclarationLists */
     private $declarationLists;
 
-    public function __construct()
+    /**
+     * Sets up the fixture, for example, open a network connection.
+     *
+     * This method is called before each test.
+     *
+     * @return void
+     */
+    protected function set_up()
     {
-        parent::__construct();
-        $spec       = new Spec();
+        parent::set_up();
+        $spec                   = new Spec();
         $this->declarationLists = $spec->declarationLists();
     }
 

diff --git a/tests/Validator/Spec/Section/DescendantTagListsTest.php b/tests/Validator/Spec/Section/DescendantTagListsTest.php
@@ -18,9 +18,16 @@ class DescendantTagListsTest extends TestCase
     /** @var DescendantTagLists */
     private $descendantTagLists;
 
-    public function __construct()
+    /**
+     * Sets up the fixture, for example, open a network connection.
+     *
+     * This method is called before each test.
+     *
+     * @return void
+     */
+    protected function set_up()
     {
-        parent::__construct();
+        parent::set_up();
         $spec                     = new Spec();
         $this->descendantTagLists = $spec->descendantTagLists();
     }

diff --git a/tests/Validator/Spec/Section/DocRulesetsTest.php b/tests/Validator/Spec/Section/DocRulesetsTest.php
@@ -19,9 +19,16 @@ class DocRulesetsTest extends TestCase
     /** @var DocRulesets */
     private $docRulesets;
 
-    public function __construct()
+    /**
+     * Sets up the fixture, for example, open a network connection.
+     *
+     * This method is called before each test.
+     *
+     * @return void
+     */
+    protected function set_up()
     {
-        parent::__construct();
+        parent::set_up();
         $spec              = new Spec();
         $this->docRulesets = $spec->docRulesets();
     }

diff --git a/tests/Validator/Spec/Section/ErrorsTest.php b/tests/Validator/Spec/Section/ErrorsTest.php
@@ -18,9 +18,16 @@ class ErrorsTest extends TestCase
     /** @var Errors */
     private $errors;
 
-    public function __construct()
+    /**
+     * Sets up the fixture, for example, open a network connection.
+     *
+     * This method is called before each test.
+     *
+     * @return void
+     */
+    protected function set_up()
     {
-        parent::__construct();
+        parent::set_up();
         $spec         = new Spec();
         $this->errors = $spec->errors();
     }