Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Fix failing Optimizer spec tests with ID generation #4799

Merged
merged 18 commits into from
Jun 2, 2020
Merged

Fix failing Optimizer spec tests with ID generation #4799

merged 18 commits into from
Jun 2, 2020

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented May 29, 2020 •
edited by westonruter
Loading

Summary

Fixes the spec test failures introduced by the merge of ampproject/amp-toolbox#763 in the ampproject/amp-toolbox package.

  • Changes the default prefix for Document::getElementId() from i-amp-id to i-amp
  • Changes the indexing behavior for Document::getElementId() to start with 0 instead of 1
  • Changes the indexing behavior for Document::getElementId() to always add the index instead of skipping the first instance
  • (fixed upstream) Skips the following tests until Attribute transformers generate unnecessary IDs amp-toolbox#809 is resolved:
    - 'ServerSideRendering - converts_sizes_attribute_to_css'
    - 'ServerSideRendering - converts_heights_attribute_to_css'
    - 'ServerSideRendering - converts_media_attribute_to_css'
  • Adds CssRule and CssRules abstractions so that CSS can be compacted (same properties merged into one rule with multiple selectors)
  • Adapts the ServerSideRendering so that it makes use of CssRule(s) objects to adapt blocking attributes
  • Fixes buggy media query negation logic
  • Skips the following test until Merging CSS rules with same properties fails in spec test amp-toolbox#819 is resolved:
    • 'ServerSideRendering - converts_sizes_attribute_to_css'

Fixes #4798

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@schlessera schlessera added Optimizer Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs labels May 29, 2020
@googlebot googlebot added the cla: yes Signed the Google CLA label May 29, 2020
@westonruter
Copy link
Member

westonruter commented May 29, 2020 •
edited
Loading

There's one remaining test failure:

1) AmpProject\Optimizer\SpecTest::testTransformerSpecFiles with data set "ServerSideRendering - converts_media_attribute_to_css" ('ServerSideRendering - convert...to_css', 'AmpProject\Optimizer\Transfor...dering', '<html ⚡>\n  <head>\n    <scri...tml>\n', '<html ⚡ i-amphtml-layout i-am.../html>')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
-    21 => '@media not all and (min-width: 650px){#i-amp-0,#my-image,#i-amp-4{display:none}}@media not screen and (min-width: 650px){#i-amp-1{display:none}}@media screen and (min-width: 650px){#i-amp-2{display:none}}@media (min-width: 650px){#i-amp-3,#i-amp-5,#i-amp-6{display:none}}'
+    21 => '@media not all and (min-width: 650px){#i-amp-0{display:none}}@media not screen and (min-width: 650px){#i-amp-1{display:none}}@media not not screen and (min-width: 650px){#i-amp-2{display:none}}@media not not (min-width: 650px){#i-amp-3{display:none}}@media not not (min-width: 650px){#i-amp-5{display:none}}@media not not (min-width: 650px){#i-amp-6{display:none}}@media not all and (min-width: 650px){#my-image{display:none}}@media not all and (min-width: 650px){#i-amp-4{display:none}}'

/app/public/content/plugins/amp/lib/optimizer/tests/src/MarkupComparison.php:45
/app/public/content/plugins/amp/lib/optimizer/tests/src/MarkupComparison.php:65
/app/public/content/plugins/amp/lib/optimizer/tests/SpecTest.php:120

westonruter
westonruter requested changes May 29, 2020
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

One more failing test.

westonruter
westonruter approved these changes Jun 2, 2020
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Looks perfect. Great job.

@westonruter westonruter added this to the v1.6 milestone Jun 2, 2020
@westonruter westonruter merged commit b11c04c into develop Jun 2, 2020
@westonruter westonruter deleted the fix/4798-fix-failing-optimizer-spec-tests-with-ids branch June 2, 2020 06:58
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@schlessera schlessera added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 12, 2020
@schlessera schlessera mentioned this pull request Aug 25, 2020
Merged
3 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@westonruter westonruter westonruter approved these changes

Assignees
No one assigned
Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA Optimizer Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.

Failing tests in ampproject/optimizer library
3 participants
@schlessera @westonruter @googlebot