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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃ИAdd Story Ads e2e testing #23827

Merged
merged 9 commits into from Aug 9, 2019

Conversation

@calebcordry
Copy link
Member

commented Aug 8, 2019

Also would like to verify ad choices click but needs some more work on controller implementation. Left a TODO.

@cvializ @estherkim please double check controller changes.

calebcordry added 4 commits Aug 7, 2019

@calebcordry calebcordry requested review from cvializ, lannka and estherkim Aug 8, 2019

@googlebot googlebot added the cla: yes label Aug 8, 2019

calebcordry added 2 commits Aug 8, 2019
@cvializ
Copy link
Contributor

left a comment

A couple of questions

@@ -559,7 +559,8 @@ class PuppeteerController {
* @override
*/
async getActiveElement() {
const getter = () => document.activeElement;
const root = await this.getRoot_();
const getter = () => root.ownerDocument.activeElement;

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 8, 2019

Contributor

I think you you need to pass root to this.evaluate and as a parameter in the getter, since the getter runs in the context of the page being tested

const root = await this.getRoot_();
const getter = root => root.ownerDocument.activeElement;
const element = await this.evaluate(getter, root);

This comment has been minimized.

Copy link
@calebcordry

calebcordry Aug 8, 2019

Author Member

this was sloppy copy paste, Done.

@@ -242,7 +242,7 @@ class SeleniumWebDriverController {
*/
async getActiveElement() {
const root = await this.getRoot_();
const getter = root => root.parentNode.activeElement;
const getter = root => root.ownerDocument.activeElement;

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 8, 2019

Contributor

I think we discovered that ownerDocument.activeElement will be the host element and not the focused element inside the ShadowDOM, is that what you want? To get the focused element itself you would need to use root.activeElement || root.ownerDocument.activeElement

This comment has been minimized.

Copy link
@calebcordry

calebcordry Aug 8, 2019

Author Member

Done.

@cvializ
cvializ approved these changes Aug 8, 2019
Copy link
Contributor

left a comment

lgtm

@estherkim
Copy link
Contributor

left a comment

LGTM, thanks Caleb! Does the controller interface need to be updated? Ok to do later (i.e. I can do it)

@lannka
lannka approved these changes Aug 8, 2019
'amp-story-auto-ads',
{
testUrl:
'http://localhost:8000/test/manual/amp-story-auto-ads/amp-story-auto-ads-basic.html',

This comment has been minimized.

Copy link
@lannka

lannka Aug 8, 2019

Contributor

is test/manual the right place to put e2e fixtures?

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 8, 2019

Contributor

test/fixtures/e2e/amp-story-auto-ads/basic.html would probably be better. I wouldn't let it block you though if you're ready to merge

This comment has been minimized.

Copy link
@calebcordry

calebcordry Aug 9, 2019

Author Member

Moved.

calebcordry added 2 commits Aug 9, 2019
@calebcordry

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

@estherkim added method to interface.

@calebcordry

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

closes #21998

@estherkim

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

lgtm thanks!

@calebcordry calebcordry merged commit b332f1e into ampproject:master Aug 9, 2019

14 of 16 checks passed

ampproject/owners-check ampproject/owners-check
Details
ampproject/pr-deploy Ready to create a test site.
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
ampproject/bundle-size 螖 +0.00KB | no approval necessary
Details
ampproject/tests/e2e (local) 310 tests passed
Details
ampproject/tests/integration (local) 270 tests passed
Details
ampproject/tests/integration (saucelabs) 417 tests passed
Details
ampproject/tests/integration (single-pass) 212 tests passed
Details
ampproject/tests/unit (local) 10131 tests passed
Details
ampproject/tests/unit (local-changes) 21 tests passed
Details
ampproject/tests/unit (saucelabs) 7849 tests passed
Details
cla/google All necessary CLAs are signed
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 79.44% (+<.01%) compared to b43c9d4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details

@calebcordry calebcordry deleted the calebcordry:story-ads-e2e branch Aug 9, 2019

thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
馃ИAdd Story Ads e2e testing (ampproject#23827)
* shadow root.

* jsdoc

* fix getActiveElement

* e2e test

* change vw

* has to be 500

* active element comments

* move test html file

* add to interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.