Fake and real window fixtures for tests #5425

dvoytenko · 2016-10-05T19:14:12Z

No description provided.

cramforce

Don't have time for a thorough review, but this looks amazing!

cramforce · 2016-10-05T21:26:13Z

testing/describes.js

+
+    beforeEach(() => {
+      return new Promise(function(resolve, reject) {
+        const iframe = document.createElement('iframe');


Can this use iframe.js? I suppose you did this on purpose?

I did this on purpose here to separate "pure" window from window with AMP setup. And also I changed some defaults, such as runtimeOff that I believe are more valid for our tests. And for migration purposes I kept createIframePromise w/o changes to be able to migrate over time.

erwinmombay · 2016-10-06T15:46:43Z

test/functional/test-extensions.js

-import * as cust from '../../src/custom-element';
-import * as sinon from 'sinon';
-
+import * as describes from '../../testing/describes';


should we make it a global instead?

I also made sandbox global. Just sandbox for now to see how it plays out. We may do this for other vars as well.

lannka

It looks cool!

That definitely would help to further isolate tests.

In the past, one thing I couldn't find a nice solution is how to simulate a postMessage in the test. You know since we created this iframe, the postMessage should be from the iframe window, not the doc window.

lannka · 2016-10-06T15:43:33Z

testing/describes.js

+ *   clock: (!FakeClock|undefined),
+ * })} fn
+ */
+export function sandboxed(name, spec, fn) {


ha, shall we just make this the default? any use case a sandbox can be unwanted?

I was thinking about that. But for migration I decided to do sandboxed instead. You still need to do top-level describe and we'll adopt sandboxed instead until all tests are migrated. After that, we can push it into all tests by default.

How about just making it a part of the realWinSpec then?
I just don't like that we need to repeat such a hiarachy in every test:

describes.sandboxed(()=>{ describes.readWindow()=>

lannka · 2016-10-06T15:44:46Z

testing/describes.js

+export function realWin(name, spec, fn) {
+  return describe(name, function() {
+
+    const test = Object.create(null);


nitpick: how about call it env

lannka · 2016-10-06T15:52:50Z

test/functional/test-extensions.js

  });

  afterEach(() => {
    sandbox.restore();
  });

-  describe('registerExtension', () => {
+  describes.fakeWin('registerExtension', {}, test => {


just curious why the tests have to be done in a fake window?

Well. This simply recognizes that we do it a lot. A number of things are just too hard to mock out on a life window object.

lannka · 2016-10-06T15:54:12Z

testing/describes.js

+      delete test.win;
+    });
+
+    describe(SUB, function() {


Is the SUB reserved for future expansion to tests under more env?

No, but I have to call describe here to get tests-own beforeEach and afterEach called and if I call describe, it must have a name, even if it's just a space.

erwinmombay · 2016-10-06T16:11:14Z

testing/describes.js

+
+
+/**
+ * A test with a fake window.


should it be real win?

erwinmombay · 2016-10-06T16:14:44Z

testing/fake-dom.js

+
+    /**
+     * @param {function()} handler
+     * @param {number=} timeout


missing the var_args on the sig

dvoytenko · 2016-10-06T19:06:58Z

All! PTAL, all comments addressed.

@lannka, I don't have a story on postMessage yet. But I'm hoping, once this is in, we will all fill in the gaps.

erwinmombay · 2016-10-06T19:58:14Z

testing/fake-dom.js

+   */
+  static intercept(target) {
+    target.eventListeners = new EventListeners();
+    target.addEventListener = function(type, handler, captureOrOpts) {


do these listeners exist in the full scope of the test run?

Only between beforeEach and afterEach.

erwinmombay · 2016-10-06T21:57:26Z

testing/describes.js

+export function sandboxed(name, spec, fn) {
+  return describe(name, function() {
+
+    const env = Object.create(null);


@dvoytenko should we also freeze or seal the env object?

We can't. This object is shared between all tests runs :( That's why I'm so careful with deletes and probably will be even more so.

erwinmombay · 2016-10-06T22:21:06Z

LGTM deferring for a 2nd LGTM

lannka

With one comment.

lannka · 2016-10-06T23:20:09Z

testing/describes.js

+ *   clock: (!FakeClock|undefined),
+ * })} fn
+ */
+export function sandboxed(name, spec, fn) {


How about just making it a part of the realWinSpec then?
I just don't like that we need to repeat such a hiarachy in every test:

describes.sandboxed(()=>{ describes.readWindow()=>

dvoytenko · 2016-10-06T23:28:48Z

@lannka Hmm. This nesting is optional. But let me try to remove it altogether...

dvoytenko · 2016-10-07T01:06:41Z

@lannka I refactored things a bit. Now all describes create a sandbox (or reuse an existing one). This definitely looks better. PTAL.

jridgewell · 2016-10-07T01:33:37Z

testing/describes.js

+      }
+    });
+
+    describe(SUB, function() {


We do we have another describe block?

So that the callback we call (fn) can call its own beforeEach and afterEach and we observe the nested behavior.

lannka · 2016-10-07T02:28:29Z

test/functional/test-extensions.js


-  describe('registerExtension', () => {
+describes.sandboxed('Extensions', {}, () => {


I guess this is not needed anymore after you refactoring?

Not super needed, no. But I still like the grouping feature since it combines two distinct group of tests inside. The simpler cases will not need it.

@erwinmombay

…mp_reddit_extension * 'master' of https://github.com/ampproject/amphtml: (65 commits) Send vars from carousel to amp-analytics (ampproject#5388) Rolling back pan-x on scrollable carousel. (ampproject#5490) Add context.library.name to Segment (ampproject#5467) Rollback pan-x on carousel as it does not work on android. (ampproject#5473) cron job from @erwinmombay to update size.txt (ampproject#5468) Added example of usage of carousel arrows (ampproject#5397) ALP for A4A (ampproject#5430) Add Affiliate-B support for amp-ad (ampproject#5223) First cut of changes for New Type Inference (ampproject#5438) Lightbox 2.0: Use object-fit:cover for thumbnails Upgrade Closure Compiler to v20160911 (Sep 11, 2016) (ampproject#5457) Fake and real window fixtures for tests (ampproject#5425) Backgrounded Variable added to Timer Trigger (ampproject#5240) make `touch-action: pan-x` on `amp-carousel` (ampproject#5391) Fix PR check on new branches (ampproject#5455) Do not rely on ampExtendedElements being created (ampproject#5454) Turn type checking for 3p code back on. (ampproject#5452) Update ads/README.md with gulp lint tip (ampproject#5444) Migrate document-submit to doc scope (ampproject#5437) Link to Building an AMP Extension document. (ampproject#5451) ...

* Fake and real window fixtures for tests * review fixes * whitelist test vars * flush vsync * add storage API * linter * Always define sandbox and make fixtures reusable

dvoytenko added the NEEDS REVIEW label Oct 5, 2016

dvoytenko assigned cramforce, jridgewell, erwinmombay and lannka Oct 5, 2016

cramforce reviewed Oct 5, 2016

View reviewed changes

erwinmombay reviewed Oct 6, 2016

View reviewed changes

lannka reviewed Oct 6, 2016

View reviewed changes

erwinmombay reviewed Oct 6, 2016

View reviewed changes

Dima Voytenko added 4 commits October 6, 2016 15:19

Fake and real window fixtures for tests

6b42ca5

review fixes

48a5d45

whitelist test vars

dfcee7c

flush vsync

72e877e

erwinmombay approved these changes Oct 6, 2016

View reviewed changes

add storage API

b3e3bbb

dvoytenko force-pushed the amptest1-fakewin branch from 2670eb6 to b3e3bbb Compare October 6, 2016 22:40

linter

3ec985c

lannka approved these changes Oct 6, 2016

View reviewed changes

lannka added LGTM and removed NEEDS REVIEW labels Oct 6, 2016

Always define sandbox and make fixtures reusable

1172b16

dvoytenko merged commit d5c8afb into ampproject:master Oct 7, 2016

dvoytenko deleted the amptest1-fakewin branch October 7, 2016 01:19

jridgewell reviewed Oct 7, 2016

View reviewed changes

lannka reviewed Oct 7, 2016

View reviewed changes


		describe('registerExtension', () => {
		describes.sandboxed('Extensions', {}, () => {

Fake and real window fixtures for tests #5425

Fake and real window fixtures for tests #5425

Conversation

dvoytenko commented Oct 5, 2016

cramforce left a 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

lannka left a 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

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

dvoytenko commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Oct 6, 2016

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Oct 6, 2016

dvoytenko commented Oct 7, 2016

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

erwinmombay Oct 6, 2016 •

edited