Text and Amp-Carousel Bind Integration Tests #7637

kmh287 · 2017-02-17T17:33:52Z

An integration tesat for amp-bind and amp-carousel.

jridgewell

I dislike all these private properties we're exposing for testing. Seems like we should be returning the promises.

jridgewell · 2017-02-17T18:42:39Z

extensions/amp-bind/0.1/bind-impl.js

    this.boundElements_.forEach(boundElement => {
      const {element, boundProperties} = boundElement;
      const tagName = element.tagName;

-      this.resources_.mutateElement(element, () => {
+      const applyPromise = this.resources_.mutateElement(element, () => {


Could we use boundElements#map?

kmh287 · 2017-02-17T18:59:52Z

Returning the promise won't quite work because the callers aren't always reachable from the test.

How would you feel about making these properties private and instead exposing methods that return the promises?

jridgewell · 2017-02-17T19:06:01Z

Returning the promise won't quite work because the callers aren't always reachable from the test.

It'd require reworking several. #apply, #digest, #setState.

How would you feel about making these properties private and instead exposing methods that return the promises?

That's better than using the property directly.

dreamofabear · 2017-02-17T19:19:09Z

#7573 changes those methods to return promises FYI.

…ion_tests

dreamofabear · 2017-02-21T17:20:37Z

extensions/amp-bind/0.1/test/test-bind-carousel.js

+import {createIframePromise} from '../../../../testing/iframe';
+import {bindForDoc} from '../../../../src/bind';
+
+describe('test-scrollable-carousel', () => {


I think amp-bind should have a single integration test test-bind-integration.js with a nested describe() for each component.

A <p> element with a [text] and [class] binding is the simplest starting point. Then amp-img, etc.

dreamofabear · 2017-02-21T17:26:58Z

extensions/amp-bind/0.1/test/test-bind-carousel.js

+  let bind;
+
+  beforeEach(() => {
+    return createIframePromise().then(i => {


Check out how createFixtureIframe is used in other integration tests.

They live in /test/integration/ or in individual extension folders (searching for "test integration" in Sublime's open resource should suffice).

Are you saying you want me to use a test fixture instead of what I have here?

dreamofabear · 2017-02-21T17:34:20Z

extensions/amp-bind/0.1/test/test-bind-carousel.js

+  it('should change slides when the slide variable binding changes', () => {
+    const impl = carousel.implementation_;
+    expect(impl.slideIndex_).to.equal(0);
+    bind.setState({selectedSlide: 1});


Integration tests should verify functionality at a higher level (e.g. an on="tap:AMP.setState(...) action rather than method invocation).

kmh287 · 2017-02-21T21:21:32Z

I've made the promises private and exposed methods to wait on them.

…ion_tests

kmh287 · 2017-02-23T17:30:34Z

#7369

…ion_tests

kmh287 · 2017-02-27T17:01:18Z

@choumx Let me know if you have any more comments.

dreamofabear · 2017-02-28T00:37:19Z

extensions/amp-bind/0.1/bind-impl.js

+   * @return {?Promise}
+   * @visibleForTesting
+   */
+  waitForInitializationForTesting() {


Nit: initializePromiseForTesting

"waitForInitialization" is a verb and sounds like the method is actively doing something.

dreamofabear · 2017-02-28T00:38:21Z

extensions/amp-bind/0.1/bind-impl.js

+    /**
+     * @private {?Promise}
+     */
+    this.applyPromise_ = null;


This is not set anywhere.

dreamofabear · 2017-02-28T00:38:42Z

extensions/amp-bind/0.1/bind-impl.js

+   * @return {?Promise}
+   * @visibleForTesting
+   */
+  waitForApplicationForTesting() {


setStatePromiseForTesting

dreamofabear · 2017-02-28T00:40:18Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+      chunkInstanceForTesting(iframe.ampdoc);
+      toggleExperiment(iframe.win, 'amp-bind', true, true);
+      bind = installBindForTesting(iframe.ampdoc);
+      return iframe.ampdoc.whenReady();


There might be some quirk to using it. Ideally we shouldn't need to manually set up AMP services in the fixture.

I'll patch and give this a try tomorrow.

dreamofabear · 2017-02-28T18:41:24Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+    }).then(() => {
+      const ampdocService = ampdocServiceFor(iframe.win);
+      ampdoc = ampdocService.getAmpDoc(iframe.doc);
+      chunkInstanceForTesting(ampdoc);


Lines 40-42 shouldn't be necessary.

dreamofabear · 2017-02-28T18:42:46Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+      const ampdocService = ampdocServiceFor(iframe.win);
+      ampdoc = ampdocService.getAmpDoc(iframe.doc);
+      chunkInstanceForTesting(ampdoc);
+      bind = installBindForTesting(ampdoc);


I think you can use bind.js#bindForDoc and avoid the need for the new method entirely.

Unfortunately if I add the script tag and try to use bindForDoc it just stalls indefinitely. I can clean this up a bit though.

Ah, it's because you're missing the amp-bind binary in the fixture.

<script async custom-element="amp-bind" src="dist/v0/amp-bind-0.1.js"></script>

dreamofabear · 2017-03-02T16:22:10Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+      return iframe.awaitEvent('amp:load:start', 1);
+    }).then(() => {
+      const ampdocService = ampdocServiceFor(iframe.win);
+      ampdoc = ampdocService.getAmpDoc(iframe.doc);


ampdoc.js#getAmpDoc is a more concise way to do this, although not sure you need it after fixing the other comment.

still need the ampdoc to call bindForDoc

…d in beforeEach

kmh287 · 2017-03-03T03:30:04Z

@jridgewell let me know if you have any more comments

jridgewell · 2017-03-03T03:46:18Z

extensions/amp-bind/0.1/bind-impl.js

+
+      if (opt_verifyOnly) {
+        this.verify_(results);
+        return Promise.resolve();


Nit: No need to return here.

jridgewell · 2017-03-03T03:47:53Z

extensions/amp-bind/0.1/bind-impl.js

+   * @return {?Promise}
+   * @visibleForTesting
+   */
+  initializePromiseForTesting() {


Nonblocking: @erwinmombay, can you confirm we strip *ForTesting methods?

* carousel test, still in progress * caorusel integration test working * cleanup * cleanup2 * lint errors * made promises on bindimpl private * lint error * pr comment * wrong path for closure annotation * changing some type annotations * pr comments * lint errors * retry * cleanup * cleanup * don't access private property * missing semicolon * class binding test * pr comments * timeout for sauce labs * title * merge * pr comment * cleanup * pr comments * cleanup * clean * clean * renaming to match other integration tests, comment about setup of bind in beforeEach * more commenting * pr comment

kmh287 added 5 commits February 17, 2017 10:54

carousel test, still in progress

059eb7a

caorusel integration test working

9caad5c

cleanup

51c7c94

cleanup2

366a967

lint errors

8dd143c

jridgewell requested review from jridgewell and dreamofabear February 17, 2017 18:40

jridgewell reviewed Feb 17, 2017

View reviewed changes

kmh287 added 6 commits February 21, 2017 10:38

Merge branch 'master' of github.com:kmh287/amphtml into bind_integrat…

3c24381

…ion_tests

made promises on bindimpl private

b6532d4

lint error

8c6e681

pr comment

4e09373

wrong path for closure annotation

a93ced8

changing some type annotations

a604e08

kmh287 mentioned this pull request Feb 21, 2017

Weekly Status 2017-02-20 #7679

Closed

dreamofabear requested changes Feb 21, 2017

View reviewed changes

kmh287 added 3 commits February 21, 2017 15:42

pr comments

aec88bd

lint errors

3f42611

retry

0286446

kmh287 changed the title ~~Bind-Carousel Integration Test~~ Text and Bind-Carousel Integration Test Feb 21, 2017

kmh287 added 5 commits February 21, 2017 16:30

cleanup

220f7a6

cleanup

9fc6099

don't access private property

b471190

missing semicolon

87acaa1

class binding test

6f681ac

kmh287 changed the title ~~Text and Bind-Carousel Integration Test~~ Text and Amp-Carousel Bind Integration Tests Feb 22, 2017

kmh287 added 2 commits February 23, 2017 11:44

Merge branch 'master' of github.com:kmh287/amphtml into bind_integrat…

ee96295

…ion_tests

pr comments

e05aac2

kmh287 added 7 commits February 23, 2017 12:49

timeout for sauce labs

261c39f

title

9c9e771

Merge branch 'master' of github.com:kmh287/amphtml into bind_integrat…

ccf6504

…ion_tests

merge

785377d

merge

948a7d1

pr comment

870ab4f

cleanup

7ed7ed1

dreamofabear reviewed Feb 28, 2017

View reviewed changes

pr comments

81a80de

kmh287 force-pushed the bind_integration_tests branch from d1ec400 to 81a80de Compare February 28, 2017 16:25

dreamofabear reviewed Feb 28, 2017

View reviewed changes

cleanup

5462373

kmh287 force-pushed the bind_integration_tests branch from 2e0c115 to 5462373 Compare March 2, 2017 14:54

kmh287 added 2 commits March 2, 2017 09:57

clean

36f502a

clean

6ef82e2

dreamofabear reviewed Mar 2, 2017

View reviewed changes

kmh287 added 2 commits March 2, 2017 12:24

renaming to match other integration tests, comment about setup of bin…

17d7d4a

…d in beforeEach

more commenting

3eb7c06

dreamofabear approved these changes Mar 2, 2017

View reviewed changes

jridgewell approved these changes Mar 3, 2017

View reviewed changes

kmh287 added 2 commits March 3, 2017 09:25

pr comment

2608395

merge

1300dfd

kmh287 merged commit 711cc13 into ampproject:master Mar 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text and Amp-Carousel Bind Integration Tests #7637

Text and Amp-Carousel Bind Integration Tests #7637

kmh287 commented Feb 17, 2017

jridgewell left a comment

jridgewell Feb 17, 2017

kmh287 Feb 21, 2017

kmh287 commented Feb 17, 2017

jridgewell commented Feb 17, 2017

dreamofabear commented Feb 17, 2017

dreamofabear Feb 21, 2017

dreamofabear Feb 21, 2017

kmh287 Feb 21, 2017

dreamofabear Feb 21, 2017

kmh287 commented Feb 21, 2017

kmh287 commented Feb 23, 2017

kmh287 commented Feb 27, 2017

dreamofabear Feb 28, 2017

kmh287 Feb 28, 2017

dreamofabear Feb 28, 2017

kmh287 Feb 28, 2017

dreamofabear Feb 28, 2017

kmh287 Feb 28, 2017

dreamofabear Feb 28, 2017

dreamofabear Feb 28, 2017

dreamofabear Feb 28, 2017

kmh287 Feb 28, 2017 •

edited

dreamofabear Mar 2, 2017

dreamofabear Mar 2, 2017

kmh287 Mar 2, 2017

kmh287 commented Mar 3, 2017

jridgewell Mar 3, 2017

kmh287 Mar 3, 2017

jridgewell Mar 3, 2017

Text and Amp-Carousel Bind Integration Tests #7637

Text and Amp-Carousel Bind Integration Tests #7637

Conversation

kmh287 commented Feb 17, 2017

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Feb 17, 2017

jridgewell commented Feb 17, 2017

dreamofabear commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Feb 21, 2017

kmh287 commented Feb 23, 2017

kmh287 commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Feb 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Feb 28, 2017 •

edited