Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

🏗 Added simple Visual Diff Tests for amp-consent #20783

Merged
merged 5 commits into from Feb 21, 2019

Conversation

torch2424
Copy link
Contributor

@torch2424 torch2424 commented Feb 11, 2019

This adds visual diff tests for a very simple amp-consent use case. Also, this allows requests from the *.localhost subdomain, to allow checking the consent from *.localhost domains.

Issues / PRs that this process has opened

Per pair programming between @danielrozenberg and I, or just feedback from me
relates to #20671
opens #20780
opens #20781
opens #20782
opens #20785

Example

screen shot 2019-02-11 at 2 54 58 pm

Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

Code looks good! I'll let @zhouyx have the final say and approve the Percy build

@zhouyx
Copy link
Contributor

zhouyx commented Feb 21, 2019

Sorry for the late review. This is awesome! Thank you so much for adding them

@torch2424 torch2424 merged commit 30d121b into ampproject:master Feb 21, 2019
@torch2424 torch2424 deleted the consent-ui-visual-diff branch February 21, 2019 20:39
@newmuis
Copy link
Contributor

newmuis commented Feb 21, 2019

Hi @torch2424, this appears to have broken the build, since verifyCssElements doesn't exist in build-system/tasks/visual-diff/helpers.js (it was removed in #20888).

Can you roll back?

torch2424 added a commit that referenced this pull request Feb 21, 2019
@torch2424 torch2424 restored the consent-ui-visual-diff branch February 21, 2019 22:38
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Started writing amp-consent visual diff tests

* Finally got this working. Need to split into two PRs

* Moved testing code out

* Removed the need to go to random subdomain. Now handled by testing suite
itself
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
* Started writing amp-consent visual diff tests

* Finally got this working. Need to split into two PRs

* Moved testing code out

* Removed the need to go to random subdomain. Now handled by testing suite
itself
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants