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

✅ Add integration for shadow AMP v0.js #26344

Merged
merged 1 commit into from
Feb 12, 2020
Merged

✅ Add integration for shadow AMP v0.js #26344

merged 1 commit into from
Feb 12, 2020

Conversation

kevinkimball
Copy link
Contributor

Adds basic integration tests that show attaching a shadow AMP document and laying out an <amp-img> component. Closes #5580

@kevinkimball kevinkimball changed the title ✅Add integration for shadow AMP v0.js ✅ Add integration for shadow AMP v0.js Jan 14, 2020
erwinmombay
erwinmombay reviewed Jan 14, 2020
View reviewed changes
examples/shadow-amp/test.js Outdated
@@ -0,0 +1,33 @@
/**
* Copyright 2018 The AMP HTML Authors. All Rights Reserved.

This comment was marked as resolved.

Sign in to view

erwinmombay
erwinmombay reviewed Jan 14, 2020
View reviewed changes
test/integration/test-shadow-amp.js Outdated
@@ -0,0 +1,54 @@
/**
* Copyright 2018 The AMP HTML Authors. All Rights Reserved.

This comment was marked as resolved.

Sign in to view

erwinmombay
erwinmombay reviewed Jan 14, 2020
View reviewed changes
testing/test-helper.js Outdated
@@ -210,9 +210,9 @@ export class RequestBank {
}

export class BrowserController {
constructor(win) {
constructor(win, doc) {

This comment was marked as resolved.

Sign in to view

erwinmombay
erwinmombay reviewed Jan 14, 2020
View reviewed changes
examples/shadow-amp/test.js Outdated
@@ -0,0 +1,33 @@
/**

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the app code for the test subject page

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the port below and stuff, maybe it's just better to be in the integration js file itself as a string?

dvoytenko
dvoytenko suggested changes Jan 15, 2020
View reviewed changes
testing/test-helper.js Outdated
@@ -210,9 +210,9 @@ export class RequestBank {
}

export class BrowserController {
constructor(win) {
constructor(win, opt_doc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit misleading. This is actually ShadowRoot or at least Document|ShadowRoot, right? Maybe then we should call it rootNode?

testing/test-helper.js Outdated
* @param {number=} timeout
* @return {!Promise}
*/
waitForShadowDoc(hostSelector, timeout = 10000) {
Copy link
Contributor

Choose a reason for hiding this comment

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

waitForShadowRoot

@kevinkimball kevinkimball force-pushed the shadow-amp-integration-test branch from a5a1b4f to 57297c1 Compare January 16, 2020 19:39
@kevinkimball
Copy link
Contributor Author

@dvoytenko Good naming suggestions , thanks. Updated the PR

@kevinkimball kevinkimball force-pushed the shadow-amp-integration-test branch 8 times, most recently from d70ed9e to 9aee0d7 Compare January 21, 2020 18:53
dvoytenko
dvoytenko reviewed Jan 21, 2020
View reviewed changes
testing/test-helper.js Outdated
this.win_ = win;
this.doc_ = this.win_.document;
this.doc_ = opt_rootNode || this.win_.document;

This comment was marked as resolved.

Sign in to view

dvoytenko
dvoytenko reviewed Jan 21, 2020
View reviewed changes
build-system/server/amp4test.js Outdated
/**
* Serves an amp doc for test-shadow-amp.js
*/
app.get('/shadow-amp', (_req, res) =>

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other test documents seem to be loaded from this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Ok let's keep then since it's an established pattern. Though I think it's a bit painful.

@kevinkimball kevinkimball force-pushed the shadow-amp-integration-test branch 2 times, most recently from 14591b6 to 1fb797e Compare January 22, 2020 18:45
@amp-owners-bot
Copy link

Hey @rsimha, these files were changed:

  • .vscode/settings.json

rsimha
rsimha reviewed Jan 22, 2020
View reviewed changes
.vscode/settings.json Outdated
Comment on lines 28 to 35
"[markdown]": {
"editor.formatOnSave": true
},
"editor.codeActionsOnSave": {
"source.fixAll.eslint": true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change can be reverted after you sync to HEAD.

@kevinkimball kevinkimball force-pushed the shadow-amp-integration-test branch 14 times, most recently from 3cdf5b7 to 4436f40 Compare February 10, 2020 19:21
@amp-bundle-size amp-bundle-size bot requested a review from powerivq February 10, 2020 19:29
@kevinkimball kevinkimball force-pushed the shadow-amp-integration-test branch 3 times, most recently from 08cb627 to 19db117 Compare February 10, 2020 21:13
@kevinkimball kevinkimball force-pushed the shadow-amp-integration-test branch from 19db117 to 2055e33 Compare February 10, 2020 22:01
@kevinkimball
Copy link
Contributor Author

@dvoytenko I believe all issues have been addressed, can you take another look? Thanks.

@kevinkimball kevinkimball merged commit 4ad2a25 into ampproject:master Feb 12, 2020
@kevinkimball kevinkimball deleted the shadow-amp-integration-test branch February 12, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha left review comments

@erwinmombay erwinmombay erwinmombay approved these changes

@dvoytenko dvoytenko dvoytenko approved these changes

Assignees
No one assigned
Labels
Category: PWA cla: yes PR use: In Beta / Experimental PR use: In LTS PR use: In Stable WG: performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add integration test for shadow-amp startup
6 participants
@kevinkimball @erwinmombay @dvoytenko @rsimha @googlebot @ampprojectbot