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.

Sign up for GitHub

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

Jump to bottom

[amp-story] Fixing flaky test (vertical rendering) #33878

Merged
merged 4 commits into from
Apr 19, 2021
Merged

[amp-story] Fixing flaky test (vertical rendering) #33878

merged 4 commits into from
Apr 19, 2021

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Apr 16, 2021

Closes #33758

@Enriqe
Copy link
Contributor Author

Enriqe commented Apr 16, 2021 •
edited

@gmajoulet I couldn't figure out a way to remove the controls inside the video causing the flakiness (they are inside a shadow root (user agent) - which is not accessible via JS because it's not an open shadow DOM). So I just deleted the entire video element from the snapshot via an interactive test. It's not ideal but at least it tests everything else except showing the poster image of the video in vertical render mode.

(I didn't delete the <amp-video> element directly from the HTML as it tests some functionality coming from the captions and displaying them in the vertical view.)

@Enriqe Enriqe requested a review from gmajoulet April 16, 2021 21:48
@gmajoulet
Copy link
Contributor

The overall direction sounds good, do you think we could render a black CSS rectangle on top of the controls instead? That might an easier fix (no JS) and test more things

@gmajoulet
Copy link
Contributor

cc @danielrozenberg who tried to fix it in the past

@danielrozenberg
Copy link
Member

lol love this hack :D

@Enriqe Enriqe merged commit 961d6d7 into ampproject:main Apr 19, 2021
@Enriqe
Copy link
Contributor Author

Enriqe commented Apr 19, 2021

62ebee9d44a620406c1beb33d0434a96

@danielrozenberg danielrozenberg mentioned this pull request Apr 27, 2021
Closed
rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
@Enriqe @rochapablo
[amp-story] Fixing flaky test (vertical rendering) (ampproject#33878)
39aa0c2 
* fixing flaky test

* woops

* remove video

* hide controls via block of CSS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix flaky visual diff tests
4 participants
@Enriqe @gmajoulet @danielrozenberg @ampprojectbot