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

Test that the demo page loads without marking the post as dirty #10104

Merged
merged 2 commits into from Sep 26, 2018

Conversation

Projects
None yet
2 participants
@notnownikki
Member

notnownikki commented Sep 21, 2018

Description

Makes sure that the demo page loads without marking itself as dirty.

Embed responses are rewritten so that the src of the Vimeo iframe is removed, and we don't load any actual content from Vimeo, which causes errors in the console. The size of the iframe is preserved so that aspect ratio calculations still happen.

How has this been tested?

Run the test, it should pass.
Remove the aspect ratio class names from the Vimeo embed block in the demo post content, run the test, it should fail.

@notnownikki notnownikki requested a review from aduth Sep 21, 2018

@notnownikki notnownikki added this to the 4.0 milestone Sep 21, 2018

@jorgefilipecosta

I wonder given that vimeo is causing us troubles while testing and that its iframe causes console errors would chaning the demo page to use a vimeo alternative be an option?

Show outdated Hide outdated test/e2e/specs/demo.test.js Outdated
// Remove the first src attribute. This stops the Vimeo iframe loading the actual
// embedded content, but the height and width are preserved so layout related
// attributes, like aspect ratio CSS classes, remain the same.
preview.html = preview.html.replace( /src=[^\s]+/, '' );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 21, 2018

Member

Should we only apply this replace to vimeo?

@jorgefilipecosta

jorgefilipecosta Sep 21, 2018

Member

Should we only apply this replace to vimeo?

This comment has been minimized.

@notnownikki

notnownikki Sep 22, 2018

Member

It's the only call we do to the embed API, and we want to do this to all embeds so we're not loading third party scripts in our tests.

@notnownikki

notnownikki Sep 22, 2018

Member

It's the only call we do to the embed API, and we want to do this to all embeds so we're not loading third party scripts in our tests.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 22, 2018

Member

@jorgefilipecosta

I wonder given that vimeo is causing us troubles while testing and that its iframe causes console errors would chaning the demo page to use a vimeo alternative be an option?

Embeds are just... a nightmare when it comes to this stuff. Let me tell you the tale 🤣

This test was originally to make sure that we don't have any JavaScript errors on the demo page. Because we use a sandbox iframe with security restrictions, we will get errors when embeds try to use capabilities we've shut off, including (but not limited to) submitting tracking forms, and presentation. In addition, the Travis setup stops some requests from going through, resulting in 403 or 404 errors that we normally wouldn't see in regular use. All of that made it unreasonable to try to filter out embed related errors in the console vs actual errors in the console, so we intercepted the embed request and returned a mock response that had just a paragraph in the HTML. Unfortunately, that wasn't good enough either, because we detect for iframes that are sized for common video formats and apply styling so they're responsive, and our mock response wasn't triggering that and showing that the post was getting marked as dirty. We needed to respond with the iframe that actually came back from Vimeo to made sure that in actual use, the aspect ratio detection wouldn't dirty the post. Because we only do one embed, and it's the Vimeo embed, removing the src attribute works for this test. Eventually, we want to move that out of the test and into something that can be reused by other tests, and that means doing more parsing and DOM manipulation than we need to for this. But it will happen in another PR - most likely the e2e test for embeds.

Member

notnownikki commented Sep 22, 2018

@jorgefilipecosta

I wonder given that vimeo is causing us troubles while testing and that its iframe causes console errors would chaning the demo page to use a vimeo alternative be an option?

Embeds are just... a nightmare when it comes to this stuff. Let me tell you the tale 🤣

This test was originally to make sure that we don't have any JavaScript errors on the demo page. Because we use a sandbox iframe with security restrictions, we will get errors when embeds try to use capabilities we've shut off, including (but not limited to) submitting tracking forms, and presentation. In addition, the Travis setup stops some requests from going through, resulting in 403 or 404 errors that we normally wouldn't see in regular use. All of that made it unreasonable to try to filter out embed related errors in the console vs actual errors in the console, so we intercepted the embed request and returned a mock response that had just a paragraph in the HTML. Unfortunately, that wasn't good enough either, because we detect for iframes that are sized for common video formats and apply styling so they're responsive, and our mock response wasn't triggering that and showing that the post was getting marked as dirty. We needed to respond with the iframe that actually came back from Vimeo to made sure that in actual use, the aspect ratio detection wouldn't dirty the post. Because we only do one embed, and it's the Vimeo embed, removing the src attribute works for this test. Eventually, we want to move that out of the test and into something that can be reused by other tests, and that means doing more parsing and DOM manipulation than we need to for this. But it will happen in another PR - most likely the e2e test for embeds.

author_url: 'https://vimeo.com/terjes',
is_plus: '0',
account_type: 'basic',
html: '<p>Embedded content</p>',

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 26, 2018

Member

Thank you for your explanation around embeds @notnownikki 👍 I think I understand now the reason for this PR. There is just one thing I'm missing. Would the test be valid if we hardcoded the Vimeo response without the src as the HTML in this line?

@jorgefilipecosta

jorgefilipecosta Sep 26, 2018

Member

Thank you for your explanation around embeds @notnownikki 👍 I think I understand now the reason for this PR. There is just one thing I'm missing. Would the test be valid if we hardcoded the Vimeo response without the src as the HTML in this line?

This comment has been minimized.

@notnownikki

notnownikki Sep 26, 2018

Member

No, because it would always pass, even if Vimeo's response changed and caused the post to be dirty in real world use. If Vimeo decide to change their iframe dimensions for some reason, then in actual use the post might end up dirty, but in the test it would pass.

This seems like an unlikely thing to happen, but YouTube did it not too long ago with 16:9 and 4:3 videos getting different aspect ratios, so I don't want to run the risk of us having a test that says "Hey, everything is fine!" and causing us to miss actual errors.

@notnownikki

notnownikki Sep 26, 2018

Member

No, because it would always pass, even if Vimeo's response changed and caused the post to be dirty in real world use. If Vimeo decide to change their iframe dimensions for some reason, then in actual use the post might end up dirty, but in the test it would pass.

This seems like an unlikely thing to happen, but YouTube did it not too long ago with 16:9 and 4:3 videos getting different aspect ratios, so I don't want to run the risk of us having a test that says "Hey, everything is fine!" and causing us to miss actual errors.

@jorgefilipecosta

Thank you for the clarifications @notnownikki. I followed the test instructions and it worked as expected. In this case, given that we want to test against real Vimeo dimensions but not load the Vimeo inframe I think this is an acceptable solution 👍

@notnownikki notnownikki merged commit 8300578 into master Sep 26, 2018

2 checks passed

codecov/project 48.81% remains the same compared to 01bb2f2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@notnownikki notnownikki deleted the update/test-demo-does-not-alter-content-when-loaded branch Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment