Merge pull request #4799 from ampproject/fix/4798-fix-failing-optimiz…

…er-spec-tests-with-ids Fix failing Optimizer spec tests with ID generation
ampproject · Jun 2, 2020 · b11c04c · b11c04c
2 parents ee71620 + 5e0a502
commit b11c04c
Show file tree

Hide file tree

Showing 54 changed files with 1,528 additions and 150 deletions.
diff --git a/lib/common/src/Dom/Document.php b/lib/common/src/Dom/Document.php
@@ -1492,20 +1492,20 @@ public function isValidHeadNode(DOMNode $node)
      * @param string     $prefix  Optional. The prefix to use (should not have a trailing dash). Defaults to 'i-amp-id'.
      * @return string ID to use.
      */
-    public function getElementId(DOMElement $element, $prefix = 'i-amp-id')
+    public function getElementId(DOMElement $element, $prefix = 'i-amp')
     {
         if ($element->hasAttribute('id')) {
             return $element->getAttribute('id');
         }
 
         if (array_key_exists($prefix, $this->indexCounter)) {
             ++$this->indexCounter[$prefix];
-            $id = "{$prefix}-{$this->indexCounter[ $prefix ]}";
         } else {
-            $id                          = $prefix;
-            $this->indexCounter[$prefix] = 1;
+            $this->indexCounter[$prefix] = 0;
         }
 
+        $id = "{$prefix}-{$this->indexCounter[ $prefix ]}";
+
         while ($this->getElementById($id) instanceof DOMElement) {
             ++$this->indexCounter[$prefix];
             $id = "{$prefix}-{$this->indexCounter[ $prefix ]}";

diff --git a/lib/common/tests/Dom/DocumentTest.php b/lib/common/tests/Dom/DocumentTest.php
@@ -734,35 +734,35 @@ public function getGetElementIdData()
 
             'single check without existing ID'      => [
                 [
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix' ],
+                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
                 ],
             ],
 
             'consecutive checks count upwards'      => [
                 [
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-2' ],
+                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
+                    [ $elementFactory, null, 'some-prefix', 'some-prefix-1' ],
                 ],
             ],
 
             'consecutive checks for same element return same ID' => [
                 [
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix' ],
-                    [ null, null, 'some-prefix', 'some-prefix' ],
+                    [ $elementFactory, null, 'some-prefix', 'some-prefix-0' ],
+                    [ null, null, 'some-prefix', 'some-prefix-0' ],
                 ],
             ],
 
             'mixing prefixes keeps counts separate' => [
                 [
                     [ $elementFactory, 'my-id', 'some-prefix', 'my-id' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix' ],
+                    [ $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, null, 'other-prefix', 'other-prefix' ],
-                    [ $elementFactory, null, 'other-prefix', 'other-prefix-2' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-3' ],
                     [ $elementFactory, 'another-id', 'some-prefix', 'another-id' ],
-                    [ $elementFactory, null, 'some-prefix', 'some-prefix-4' ],
-                    [ null, null, 'some-prefix', 'some-prefix-4' ],
+                    [ $elementFactory, null, 'some-prefix', 'some-prefix-3' ],
+                    [ null, null, 'some-prefix', 'some-prefix-3' ],
                 ],
             ],
         ];
@@ -798,12 +798,12 @@ public function testGetElementId($checks)
     public function testGetElementIdOnPreexistingIds()
     {
         $dom = Document::fromHtml(
-            '<body><div id="some-prefix"><div id="some-prefix-2"><div id="some-prefix-3"></body>'
+            '<body><div id="some-prefix-0"><div id="some-prefix-1"><div id="some-prefix-2"></body>'
         );
 
         $element = $dom->createElement('div');
         $dom->body->appendChild($element);
 
-        $this->assertEquals('some-prefix-4', $dom->getElementId($element, 'some-prefix'));
+        $this->assertEquals('some-prefix-3', $dom->getElementId($element, 'some-prefix'));
     }
 }
diff --git a/lib/optimizer/resources/local_fallback/rtv/metadata b/lib/optimizer/resources/local_fallback/rtv/metadata
@@ -1 +1 @@
-{"ampRuntimeVersion":"012004252135000","ampCssUrl":"https://cdn.ampproject.org/rtv/012004252135000/v0.css","canaryPercentage":"0.005","diversions":["002005050322000","022004252135000","032005050322000","042005062312000","052004252135000","102005050322000"],"ltsRuntimeVersion":"012004030010070","ltsCssUrl":"https://cdn.ampproject.org/rtv/012004030010070/v0.css"}
+{"ampRuntimeVersion":"012005151844001","ampCssUrl":"https://cdn.ampproject.org/rtv/012005151844001/v0.css","canaryPercentage":"0.005","diversions":["002005150002001","002005220120000","022005151844001","032005150002001","032005220120000","042005272217000","052005151844001"],"ltsRuntimeVersion":"012005152259000","ltsCssUrl":"https://cdn.ampproject.org/rtv/012005152259000/v0.css"}
diff --git a/lib/optimizer/resources/local_fallback/v0.css b/lib/optimizer/resources/local_fallback/v0.css