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

Introduce e2e environments for AMPHTML ads #23834

Merged
merged 13 commits into from
Aug 13, 2019
Merged

Introduce e2e environments for AMPHTML ads #23834

merged 13 commits into from
Aug 13, 2019

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Aug 8, 2019 •
edited

for #24038

@lannka lannka requested a review from estherkim August 8, 2019 22:56
@lannka
Copy link
Contributor Author

lannka commented Aug 8, 2019

@estherkim do you know what's a typical cause of this timeout error:

TimeoutError: Waiting for element to be located amp-ad > iframe

https://travis-ci.org/ampproject/amphtml/jobs/569574768#L3824

@lannka lannka requested a review from powerivq August 8, 2019 23:30
@estherkim
Copy link
Collaborator

Hmm why is it timing out after ~5s? We set the timeout for beforeEach to 30s.

amphtml/build-system/tasks/e2e/describes-e2e.js

Lines 352 to 357 in e6839ff

function doTemplate(name, variant, browserName) {
const env = Object.create(variant);
this.timeout(TEST_TIMEOUT);
beforeEach(async function() {
this.timeout(SETUP_TIMEOUT);
await fixture.setup(env, browserName);

Can you try removing the inner describe() so that there are only it()s inside describes.endtoend()?

zombifier
zombifier reviewed Aug 9, 2019
View reviewed changes
build-system/server-inabox-template.html Outdated
@@ -22,6 +22,8 @@ <h1>In-a-box Envelope</h1>
<div style="background: #eee; overflow: hidden;; height: OFFSET;">scroll down to see the ad</div>

<iframe
id="inabox-frame"
src="AD_URL"
Copy link
Contributor

@zombifier zombifier Aug 9, 2019 •
edited

Choose a reason for hiding this comment

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

Duplicate src attribute (see below), which might cause errors. I believe this line snuck in during the merge with #23699.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

@lannka
Copy link
Contributor Author

lannka commented Aug 12, 2019

@estherkim @powerivq PTAL

estherkim
estherkim approved these changes Aug 13, 2019
View reviewed changes
Copy link
Collaborator

@estherkim estherkim 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 e1579c9 into ampproject:master Aug 13, 2019
@lannka lannka deleted the ads-env-e2e-tests branch August 13, 2019 16:46
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
@lannka
Introduce e2e environments for AMPHTML ads (ampproject#23834)
1e739d1 
* Introduce e2e environments for AMPHTML ads

* test-amp-bind-state

* 2 more inabox envs.

* remove describe

* enabled all inabox envs

* lint

* Enable inabox-viewport-friendly in all examples

* fix dup src

* Try to increase selenium engine ELEMENT_WAIT_TIMEOUT

* Skip failing fie test

* Skip failing fie test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zombifier zombifier zombifier left review comments

@estherkim estherkim estherkim approved these changes

@powerivq powerivq Awaiting requested review from powerivq

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
4 participants
@lannka @estherkim @zombifier @googlebot