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

✅ 360 component visual diff test #30739

Merged
merged 27 commits into from
Dec 4, 2020

Conversation

processprocess
Copy link
Contributor

Visual diff test for rendering a 360 image.

cc @ampproject/wg-stories

@processprocess processprocess self-assigned this Oct 19, 2020
@processprocess processprocess marked this pull request as draft October 20, 2020 13:37
@gmajoulet
Copy link
Contributor

image

Do you know why it's not loading on Percy?

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2020

CLA assistant check
All committers have signed the CLA.

@processprocess
Copy link
Contributor Author

Do you know why it's not loading on Percy?

I think it's not waiting until the image loads. I'll look into the loading_complete_selectors

},
() => {
user().error(TAG, 'Failed to load the amp-video.');
}
);
}

/** @private */
addClassForVisualTests_() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually never ship code that is only meant to help testing, but we can always rename this method and make it reusable in the future for other use cases.

Maybe we can add a *-loaded class in a markAsLoaded_() method to replicate amp-story.js behavior?

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Can we edit the assets to not bloat the AMPHTML repository? I'm worried about every single AMPHTML engineer having to download these when they clone the repository.

The png can be heavily optimized, even if it ends up becoming pretty low quality. As long as it renders we're good.
For the video, can we edit it to just keep the first half second or something?

@processprocess
Copy link
Contributor Author

processprocess commented Dec 1, 2020

CC @danielrozenberg As we've discussed, WebGL might not be rendering in Percy.
A screenshot from Puppeteer renders correctly:
Screen Shot 2020-12-01 at 3 15 16 PM

update: This might be because Percy takes a DOM snapshot and does not capture the state of the canvas element.
We might be able to solve this by setting an img src with canvas.toDataURL but the frame might be blank due to the drawing buffer being swapped.

@processprocess processprocess marked this pull request as ready for review December 2, 2020 20:53
@amp-owners-bot
Copy link

amp-owners-bot bot commented Dec 2, 2020

Hey @danielrozenberg! These files were changed:

build-system/tasks/visual-diff/index.js
build-system/tasks/visual-diff/snippets/freeze-canvas-image.js

Hey @gmajoulet, @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js

@processprocess
Copy link
Contributor Author

cc @danielrozenberg & @erwinmombay for owners approval.

@processprocess processprocess merged commit 58ccc9f into ampproject:master Dec 4, 2020
@processprocess processprocess deleted the visual-tests branch December 4, 2020 14:48
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* rebase

* Update html template

* Fix typo in comment.

* rebase

* rebase

* rebase

* rebase

* Add video assets to visual-tests folder.

* Compress assets.

* rebase

* Edit loading selector.

* Require amp-video

* merge conflict

* merge conflicts

* revert line

* loading selector

* Fix image path and selector.

* Remove controls gyroscope.

* Replace canvas with image for percy dom state.

* remove image type and quality.

* Add flag for testing in zuho and visual tests.

* heading on video.

* Remove video test.

* use replacewith
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants