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 an e2e test for amp-subscriptions-google. #23665

Merged
merged 11 commits into from Aug 9, 2019

Conversation

@qidonna
Copy link
Contributor

commented Aug 2, 2019

This test only checks if the SwG dialog is rendered. More sophisticated test cases will be added later.
@cvializ

Yuan Duan added 2 commits Aug 2, 2019
Yuan Duan
Yuan Duan

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

Yuan Duan
@cvializ
Copy link
Contributor

left a comment

🎉 Nice just a couple questions

'amp-subscriptions-google',
{
testUrl:
'http://localhost:8000/test/manual/amp-subscriptions-google/swg.amp.html',

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 5, 2019

Contributor

Currently other components use the path test/fixtures/e2e/amp-subscriptions-google/swg.html

This comment has been minimized.

Copy link
@qidonna

qidonna Aug 5, 2019

Author Contributor

I'll move it over. Thanks!


// Check the SwG subscription offer dialog iframe.
const iframe = await controller.findElement('.swg-dialog');
await expect(iframe).to.exist;

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 5, 2019

Contributor

findElement will always return an element or it will time out, so you can test something more helpful here. Is there an attribute in the DOM or something that the user can see so they would know it's working correctly? Those are the kinds of values E2E tests are best at checking.

This comment has been minimized.

Copy link
@qidonna

qidonna Aug 7, 2019

Author Contributor

Updated the test to check the DOM inside the iframe.
test passed
gulp e2e --files=extensions/amp-subscriptions-google/0.1/test-e2e/test-amp-subscriptions-google.js

Yuan Duan added 3 commits Aug 7, 2019
Yuan Duan
Yuan Duan
Yuan Duan
* @return {!Promise}
*/
async switchToFrameNum(index) {
await this.driver.switchTo().frame(index);

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 7, 2019

Contributor

Why didn't switchToFrame work with a handle to the iframe tag? I think this extra method makes the API more cluttered than it needs to be.

e.g.

const swgOuterFrame = await controller.findElement('iframe.some-classname-on-the-swg-iframe');
await controller.switchToFrame(swgOuterFrame);

edit: I mistakenly omitted the await

@qidonna

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Yuan Duan
@cvializ

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

I think I found the issue. You have to use await when calling controller.findElement

const swgOuterFrame = await controller.findElement('iframe.swg-dialog');
await controller.switchToFrame(swgOuterFrame);
@cvializ
Copy link
Contributor

left a comment

Glad it worked! I'll LGTM after the switchToFrameNum method is removed

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

left a comment

LGTM

@amp-bundle-size amp-bundle-size bot requested a review from choumx Aug 9, 2019

Yuan Duan added 2 commits Aug 9, 2019
Yuan Duan
Yuan Duan

@choumx choumx merged commit d433dd4 into ampproject:master Aug 9, 2019

13 of 15 checks passed

ampproject/pr-deploy Ready to create a test site.
Details
ampproject/tests/unit (local-changes) Tests were not required
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
ampproject/bundle-size Δ +0.00KB | no approval necessary
Details
ampproject/tests/e2e (local) 311 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) 10141 tests passed
Details
ampproject/tests/unit (saucelabs) 7856 tests passed
Details
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing 81b16d1...901c212
Details
codecov/project 79.47% (+0.01%) compared to 81b16d1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Add an e2e test for amp-subscriptions-google. (ampproject#23665)
* Add SwG e2e test.

* fix lint errors

* Update test

* Add test page

* Update the test with swithToFrame function

* Remove switchToFrameNum from controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.