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

✅ Hide the share pill in visual tests, and re-enable tests that were flaky because of this #20022

Merged
merged 1 commit into from
Dec 20, 2018

Conversation

newmuis
Copy link
Contributor

@newmuis newmuis commented Dec 20, 2018

Filed #20021 to track centralizing these fixes in one place, but this PR should fix the issues to unblock PRs.

Fixes #19890 by hiding this button entirely during tests.

/cc @rsimha

@newmuis
Copy link
Contributor Author

newmuis commented Dec 20, 2018

@gmajoulet can you please also review the Percy diffs?

@gmajoulet
Copy link
Contributor

@newmuis
Copy link
Contributor Author

newmuis commented Dec 20, 2018

I think this is also WAI. Although it looks similar to the diffs we normally see, if you click on the "new" tab, you can see that it's actually just that the entire share pill is gone (which is what this PR intends to do)

@gmajoulet
Copy link
Contributor

Oh right, thanks. I got confused because it looked a lot like the previous flakes...

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@newmuis newmuis merged commit 2e9c8b7 into ampproject:master Dec 20, 2018
@rsimha
Copy link
Contributor

rsimha commented Dec 21, 2018

I'm surprised the tests passed for this PR. Pretty sure .i-amphtml-story-share-pill-label needs to be removed from loading_complete_css for each of these visual tests.

@gmajoulet
Copy link
Contributor

gmajoulet commented Dec 21, 2018

You're right it'd be better to remove it. But could it still pass because, even if hidden, because the .i-amphtml-story-share-pill-label element is still present in the DOM?

@rsimha
Copy link
Contributor

rsimha commented Dec 21, 2018

loading_complete_css is a list of CSS classes that the test runner waits for. If .i-amphtml-story-share-pill-label is removed, the test runner will still wait for the other CSS classes in that list.

@gmajoulet
Copy link
Contributor

Yes, of course, I meant that the tests still pass because .i-amphtml-story-share-pill-label is still present in the DOM.

@rsimha
Copy link
Contributor

rsimha commented Dec 21, 2018

Aha. Yes. :)

You could add it to the forbidden list if you want to make sure it's not on the page.

noranazmy pushed a commit to noranazmy/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.

amp-story visual tests are flaky due to moving "Share" button
4 participants