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

Add Inabox integration test #19711

Merged
merged 7 commits into from Dec 19, 2018
Merged

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Dec 7, 2018

Closes #19710

scrolling="no"
width="300" height="250">
</iframe>
<script src="/examples/inabox-tag-integration.js"></script>
Copy link
Contributor

@zombifier zombifier Dec 13, 2018

Choose a reason for hiding this comment

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

(low-key watching this PR because I'm looking to take advantage of it)

You may already be aware of this issue (if so sorry!), but due to #9120 the iframe will attempt to get visibility information from the top level window, but since in Karma the top-level window is not the test fixture with the host script loaded, the message will be discarded without results. The test would still pass on browsers that didn't need the host script in the first place, but it fails on Safari due to lacking IntersectionObserver.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zombifier Thanks for catching this! We actually have our own IntersectionObserver polyfill for Safari, so this test may still work 😄

I'll let @lannka Make the final decision though, as they are more familiar with this than I am.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zombifier that's a good observation, that might be the cause of the Safari failing. I haven't looked into it yet (was OOO for a couple of days). Will update this thread once I figured out.

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

LGTM 😄 Just one small requested change.

expect(Date.now()).to.be.below(scrollTime);
expect(req.url).to.equal('/foo?cid=');
});
const analPromise = RequestBank.withdraw('analytics').then(req => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this variable? I don't think people will think analytics from this shorthand 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that's indeed a big mistake (after reading it to myself twice)

@lannka
Copy link
Contributor Author

lannka commented Dec 19, 2018

Unfortunately the BTF test is blocked by the other bug: #14010 . inabox visible trigger is not working in nested iframes.

@lannka
Copy link
Contributor Author

lannka commented Dec 19, 2018

To unblock this PR, removed the BTF test. Added a TODO to add it back once #14010 is fixed.

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@lannka lannka merged commit 7ffcffe into ampproject:master Dec 19, 2018
@lannka lannka deleted the inabox-integration-test branch December 19, 2018 21:18
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Add integration test for amp-inabox

* add test for ATF

* rename

*  fix test.

* remove BTF test

* revert some unnecessary changes
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.

None yet

4 participants