amphtml: `sanitizeHtml` in src/sanitizer.js is causing visual diff JS errors (started with prod push)

See discussion at https://github.com/ampproject/amphtml/pull/13333#issuecomment-363918699

This started failing on master with this error:

/home/travis/.rvm/gems/ruby-2.4.1/gems/poltergeist-1.17.0/lib/capybara/poltergeist/browser.rb:384:in `command': One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details). (Capybara::Poltergeist::JavascriptError)
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
	from /home/travis/.rvm/gems/ruby-2.4.1/gems/poltergeist-1.17.0/lib/capybara/poltergeist/browser.rb:39:in `visit'
	from /home/travis/.rvm/gems/ruby-2.4.1/gems/poltergeist-1.17.0/lib/capybara/poltergeist/driver.rb:99:in `visit'
	from /home/travis/.rvm/gems/ruby-2.4.1/gems/capybara-2.17.0/lib/capybara/session.rb:274:in `visit'
	from build-system/tasks/visual-diff.rb:288:in `block (2 levels) in generate_snapshots'
	from build-system/tasks/visual-diff.rb:281:in `each'
	from build-system/tasks/visual-diff.rb:281:in `block in generate_snapshots'
	from build-system/tasks/visual-diff.rb:275:in `each'
	from build-system/tasks/visual-diff.rb:275:in `generate_snapshots'
	from build-system/tasks/visual-diff.rb:250:in `run_visual_tests'
	from build-system/tasks/visual-diff.rb:433:in `main'
	from build-system/tasks/visual-diff.rb:441:in `<main>'

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 18 (12 by maintainers)

Most upvoted comments

@choumx I’ve just switched the visual tests from phantomJS to headless chrome.

rsimha on Feb 9, 2018

Yeah, we use PhantomJS. See the link under https://github.com/ampproject/amphtml/blob/master/contributing/DEVELOPING.md#visual-diff-tests.

I guess there are two questions to answer:

Why is this failing on PhantomJS?
Why did a prod push affect the behavior of a locally built runtime?

rsimha on Feb 7, 2018

@choumx It’s supposed to use local binaries with the canary and prod configs applied to it one after another. Clearly, something must have changed with the test pages in examples/visual_tests, causing the test to fetch the prod AMP binary.

I’m making some wholesale changes to the visual diff infra as we speak and will investigate, but meanwhile, let’s disable the failing test while we investigate. SG?

rsimha on Feb 7, 2018