✅ Add integration for shadow AMP v0.js by kevinkimball · Pull Request #26344 · ampproject/amphtml

kevinkimball · 2020-01-14T23:39:38Z

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

examples/shadow-amp/test.js

@@ -0,0 +1,33 @@
+/**
+ * Copyright 2018 The AMP HTML Authors. All Rights Reserved.


test/integration/test-shadow-amp.js

@@ -0,0 +1,54 @@
+/**
+ * Copyright 2018 The AMP HTML Authors. All Rights Reserved.


testing/test-helper.js


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


kevinkimball · 2020-01-14T23:56:46Z

examples/shadow-amp/test.js

@@ -0,0 +1,33 @@
+/**


It's the app code for the test subject page

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

dvoytenko · 2020-01-15T22:49:04Z

testing/test-helper.js


 export class BrowserController {
-  constructor(win) {
+  constructor(win, opt_doc) {


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

dvoytenko · 2020-01-15T22:49:45Z

testing/test-helper.js

+   * @param {number=} timeout
+   * @return {!Promise}
+   */
+  waitForShadowDoc(hostSelector, timeout = 10000) {


waitForShadowRoot

kevinkimball · 2020-01-16T19:54:50Z

@dvoytenko Good naming suggestions , thanks. Updated the PR

testing/test-helper.js

+  constructor(win, opt_rootNode) {
    this.win_ = win;
-    this.doc_ = this.win_.document;
+    this.doc_ = opt_rootNode || this.win_.document;


kevinkimball · 2020-01-21T18:57:02Z

build-system/server/amp4test.js

+/**
+ * Serves an amp doc for test-shadow-amp.js
+ */
+app.get('/shadow-amp', (_req, res) =>


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

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

amp-owners-bot · 2020-01-22T21:50:14Z

Hey @rsimha, these files were changed:

.vscode/settings.json

rsimha · 2020-01-22T21:52:42Z

.vscode/settings.json

+  "[markdown]": {
+    "editor.formatOnSave": true
+  },
+  "editor.codeActionsOnSave": {
+    "source.fixAll.eslint": true
+  }


This change can be reverted after you sync to HEAD.

kevinkimball · 2020-02-10T22:44:24Z

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

kevinkimball requested a review from erwinmombay January 14, 2020 23:39

googlebot added the cla: yes label Jan 14, 2020

amp-owners-bot bot requested review from dvoytenko and estherkim January 14, 2020 23:39

kevinkimball added Category: PWA WG: performance labels Jan 14, 2020

kevinkimball changed the title ~~✅Add integration for shadow AMP v0.js~~ ✅ Add integration for shadow AMP v0.js Jan 14, 2020

erwinmombay reviewed Jan 14, 2020

View reviewed changes

examples/shadow-amp/test.js

@@ -0,0 +1,33 @@

/**

* Copyright 2018 The AMP HTML Authors. All Rights Reserved.

This comment was marked as resolved.

Sign in to view

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 reviewed Jan 14, 2020

View reviewed changes

testing/test-helper.js Outdated

export class BrowserController {

constructor(win) {

constructor(win, doc) {

This comment was marked as resolved.

Sign in to view

erwinmombay reviewed Jan 14, 2020

View reviewed changes

erwinmombay approved these changes Jan 15, 2020

View reviewed changes

dvoytenko suggested changes Jan 15, 2020

View reviewed changes

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

kevinkimball requested a review from dvoytenko January 16, 2020 19:54

kevinkimball force-pushed the shadow-amp-integration-test branch 8 times, most recently from d70ed9e to 9aee0d7 Compare January 21, 2020 18:53

dvoytenko reviewed Jan 21, 2020

View reviewed changes

testing/test-helper.js Outdated

constructor(win, opt_rootNode) {

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 reviewed Jan 21, 2020

View reviewed changes

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

rsimha reviewed Jan 22, 2020

View reviewed changes

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 bot requested a review from powerivq February 10, 2020 19:29

kevinkimball force-pushed the shadow-amp-integration-test branch 3 times, most recently from 08cb627 to 19db117 Compare February 10, 2020 21:13

Add integration for shadow AMP v0.js

2055e33

kevinkimball force-pushed the shadow-amp-integration-test branch from 19db117 to 2055e33 Compare February 10, 2020 22:01

kevinkimball requested a review from dvoytenko February 10, 2020 22:43

kevinkimball removed request for estherkim and powerivq February 10, 2020 22:44

dvoytenko approved these changes Feb 11, 2020

View reviewed changes

kevinkimball merged commit 4ad2a25 into ampproject:master Feb 12, 2020

kevinkimball deleted the shadow-amp-integration-test branch February 12, 2020 18:22

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Feb 27, 2020

ampprojectbot added the PR use: In LTS label Mar 10, 2020

		@@ -0,0 +1,33 @@
		/**
		* Copyright 2018 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,54 @@
		/**
		* Copyright 2018 The AMP HTML Authors. All Rights Reserved.

Conversation

kevinkimball commented Jan 14, 2020

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

kevinkimball Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

kevinkimball commented Jan 16, 2020

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

kevinkimball Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

amp-owners-bot bot commented Jan 22, 2020

Uh oh!

rsimha Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

kevinkimball commented Feb 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants