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

Fix various issues with the inabox integration tests #20972

Merged
merged 13 commits into from
Mar 6, 2019
Merged

Fix various issues with the inabox integration tests #20972

merged 13 commits into from
Mar 6, 2019

Conversation

zombifier
Copy link
Contributor

@zombifier zombifier commented Feb 20, 2019 •
edited
Loading

  • Previously the tests would only really test the host script since the inabox creatives load the JS files from the AMP CDN. Now they'll also fetch the scripts locally.
  • The BTF tests now works on Safari, They weren't working before for several reasons:
    • Visibility for nested iframes was broken before. This was fixed by Make inabox host correctly get position of a nested iframe #20599.
    • The host script didn't send the positioning update because the top level window, which is where it sits, technically wasn't scrolled. The top window will now also scroll by 1 pixel if only to trigger the message.
    • The test iframe was smaller than the viewport so the creative may trigger visibility even if it isn't visible. It was resized to full height.
  • The iframes weren't being unregistered after a test is done and they're removed. This does not affect the result of the tests at all but does lead to a slew of console errors before. Now their listeners are properly removed.

While I was there I also upgraded the safe frame version to the latest release, and add test coverage for the inabox-viewport-friendly flag.

@zombifier
Copy link
Contributor Author

zombifier commented Feb 21, 2019 •
edited
Loading

So this is not quite ready yet, there's still a couple of issues that just arised:

  • Single pass mode references some of the JS bundles using relative paths, which doesn't work with safe frame (which has an external URL). If we switch the safe frame to being locally loaded from a localhost subdomain (since the safeframe url is supposed to be 3rd party) then the test would fail on Firefox 65 (Skip flaky inabox tests #20682)
  • Safari still fires the ping prematurely in the "regular" BTF case, because the iframe is resized after it's rendered, so there's a split second where it's not resized. Will need further changes.

@zombifier
Relax the min percentage visible restriction.
3341cb4 
On Firefox occasionally the window may not scroll down all the way, barely missing out on the 50% mark.
@zombifier
Copy link
Contributor Author

@lannka can you take a look?

lannka
lannka approved these changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

Good job!

@lannka lannka merged commit e66b4ab into ampproject:master Mar 6, 2019
@zombifier
Copy link
Contributor Author

@lannka So bad news the BTF tests are failing on master again. This time, the weird issue is they only fail when run with the full integration suite; I can confirm that they pass when ran on their own.

I'll debug this tomorrow; if I can't submit a fix I'll make another PR to disable them and unbreak master. On the bright side the part that I really want in (fetching amp-analytics locally) works.

Sorry for the trouble again!

@zombifier zombifier mentioned this pull request Mar 7, 2019
Closed
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
@zombifier
Fix various issues with the inabox integration tests (ampproject#20972)
4133c77 
* Fix various issues with the inabox integration tests.

* Try referencing window directly

* Load safeframe from the Karma test server.

* Disable the problematic tests to verify the rest works

* Skip safe frame tests on Firefox

* lint

* Properly test inabox-viewport-friendly flag.

Notably this allows Safari to pass the friendly frame case without needing the host script

* lint

* Trivial comment

* Relax the min percentage visible restriction.

On Firefox occasionally the window may not scroll down all the way, barely missing out on the 50% mark.
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
@zombifier @bramanudom
Fix various issues with the inabox integration tests (ampproject#20972)
09af523 
* Fix various issues with the inabox integration tests.

* Try referencing window directly

* Load safeframe from the Karma test server.

* Disable the problematic tests to verify the rest works

* Skip safe frame tests on Firefox

* lint

* Properly test inabox-viewport-friendly flag.

Notably this allows Safari to pass the friendly frame case without needing the host script

* lint

* Trivial comment

* Relax the min percentage visible restriction.

On Firefox occasionally the window may not scroll down all the way, barely missing out on the 50% mark.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lannka lannka lannka approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zombifier @lannka @googlebot