Amphtml: Ensure visual tests catch experiment changes

[choumx]

I believe we currently run visual tests with AMP runtime built on both prod and canary experiment configs (canary-config.json and prod-config.json), but PRs that change these configs don't trigger visual tests.

We should change this to ensure that our visual testing system can catch bugs like #15596.

From @rsimha:

...we generate a dummy Percy build (just one snapshot of a blank page) for PRs that don't change the runtime or the tests. This saves us the trouble of doing a full build of the runtime. https://github.com/ampproject/amphtml/blob/master/build-system/pr-check.js#L620-L621

There is a visual test for mustache, but I doubt it does enough. See https://github.com/ampproject/amphtml/blob/master/test/visual-diff/visual-tests#L127-L128

• [x] Verify that visual tests are run with prod and canary configs
• [x] Run visual tests when experiment configs change
• [x] TODO(choumx): Add a regression test for #15596.
• [x] Verify that an experiment change PR that introduces a visual test regression would be caught

/to @danielrozenberg /cc @rsimha

[rsimha]

Re: the first checkbox, here's where we apply the prod and canary configs before testing. I'll let @danielrozenberg confirm that the configs are indeed triggering the prod or canary specific behavior from the runtime.

https://github.com/ampproject/amphtml/blob/66f63cb5d202d938b60c2fa09380ba857e8541e4/build-system/tasks/visual-diff.rb#L321-L333

Edit: I reread the OP and realized that the tests aren't being triggered for config-only changes. That logic lives in pr-check.js. You'll need to add FLAG_CONFIG to the list of cases in which the visual tests are run.

https://github.com/ampproject/amphtml/blob/a63b728517371694a6ec7acc18963ab28c639058/build-system/pr-check.js#L611-L619

[choumx]

Re: (1) above, I've tried to get visual tests running with locally changed canary/prod configs and have been unsuccessful so far. Two things I've noticed:

1. You need to modify visual-diff.rb#apply_amp_config() to pass --local_branch to avoid clobbering your local canary/prod configs with the version from origin/master, which may be wildly out of date (as mine was, since I never push to origin/master). We should fix this.
2. The actual snapshots appear to pull the AMP JS files from cdn.ampproject.org rather than local folder. This is confusing because, with --debug, Selenium appears to output correct-looking HTML in the console. But when I added a sleep(30) after snapshot generation and inspected the page in Chrome, the HTML did point to CDN.

[choumx]

Re: (3) above, the regression test is being added in #15649.

[choumx]

I just realized actually we hit this issue awhile ago: https://github.com/ampproject/amphtml/issues/13351#issuecomment-363946198

Visual tests today may still not be running with locally built binaries!

[rsimha]

Yeah, IIRC, we hit the issue before, and saw that the local gulp server was serving amp.js. Not sure why the test page is still fetching from the cdn. Maybe there's another runtime setting somewhere that isn't in the correct state?

[danielrozenberg]

Re: (1) above, I created a branch to test this, by creating a new component* <amp-test-config> that changes its textContent to 'canary' or 'prod' depending on the value of AMP_CONFIG['canary'], and ran a visual diff on a test case (removing all other test cases) — looks good! I'm gonna tick this box

(* note, this branch should not be merged, because it's practically hacked together. lmk if you want to make this code merge-worthy. I don't see a lot of value in it, as long as we keep that branch around for future sanity checks)

[danielrozenberg]

Oops, looks like there is indeed an issue! If I run gulp visual-diff, the internal web server doesn't modify the example files to use the local build (https://cdn.ampproject.org/v0/ → /dist/v0), but if I run gulp visual-diff --webserver_debug, it does! I'll follow up on this

[rsimha]

Here's where --webserver-debug gets plumbed to:

https://github.com/ampproject/amphtml/blob/acc1927661eec3e7bea03456971c2cb7f83ffe4d/build-system/server.js#L59-L62

Seems like adding morgan('dev') to middleware is what triggers the URL replacement. Good catch!

[danielrozenberg]

@rsimha yep, @choumx and I just figured it out. Gonna send out a PR to fix this soon (I want to run a few local checks first)

[rsimha]

And here is why morgan('dev') and app were gated on quiet: https://github.com/ampproject/amphtml/pull/11594#discussion_r143317962

TL;DR: It's my fault. I mistakenly believed that doing so would merely silence the Powered by AMP ⚡ messages, but it had the side effect of pulling the runtime from the wrong place. And all my testing of the visual tests didn't catch this because I used --webserver_debug >_<

It's only visual diff testing that uses the --quiet flag for gulp serve, so normal testing was never affected by this bug.

I'm glad this is getting fixed!

/cc @timhaines

[danielrozenberg]

After merging #15717, running gulp visual-diff works as expected. Marking checkbox (1) as complete once again :)

[danielrozenberg]

Verified using the example that @choumx gave me, using #15744