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

Adding a Cypress test for the amp-img #1171

Merged
merged 3 commits into from
Jan 15, 2019
Merged

Conversation

jamesbrumpton
Copy link
Contributor

Resolves #953

This adds a Cypress test to make sure that the image that is rendered on the page is an amp-img.

  • I have assigned myself to this PR and the corresponding issues
  • I have followed the merging checklist and this is ready to merge.

@jamesbrumpton jamesbrumpton self-assigned this Jan 14, 2019
@jamesbrumpton jamesbrumpton added blocked This issue should not be worked on until another internal issue is completed - see desc for details AMP Work related to AMP should labels Jan 14, 2019
@jamesbrumpton
Copy link
Contributor Author

I've put the blocked label onto this until #1153 has been merged as this test will fail otherwise.

dr3
dr3 previously approved these changes Jan 14, 2019
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Just added a comment to clarify something

const figure = getElement('figure').eq(0);
figure.should('be.visible');
figure.within(() => {
getElement('amp-img');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm does this make an assertion? getElement('amp-img') could return null but as we're not checking its value, the tests would pass.

Suggested change
getElement('amp-img');
getElement('amp-img').should('be.visible');

@bcmn
Copy link
Contributor

bcmn commented Jan 15, 2019

No longer blocked on #953.

@bcmn bcmn removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Jan 15, 2019
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Awesome, thanks 👍

@jamesbrumpton jamesbrumpton merged commit 90acb59 into latest Jan 15, 2019
@jamesbrumpton jamesbrumpton deleted the add-amp-img-cypress-test branch January 15, 2019 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Work related to AMP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants