Mock xhr-fetch by Raphaeljunior · Pull Request #10776 · ampproject/amphtml

Raphaeljunior · 2017-08-03T15:36:34Z

Mocks out fetch in realWin and fakeWin and avails it through expectFetch as follows.

googlebot · 2017-08-03T15:36:37Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

…mock

googlebot · 2017-08-03T15:43:22Z

CLAs look good, thanks!

rsimha · 2017-08-03T17:35:31Z

@Raphaeljunior, once #10671 is merged, the dependency errors on Travis should no longer be an issue.

rsimha · 2017-08-03T17:36:37Z

package.json

    "eslint": "3.18.0",
    "eslint-plugin-google-camelcase": "0.0.2",
    "express": "4.14.0",
+    "fetch-mock": "^5.12.1",


Can you also update yarn.lock by running yarn upgrade --save-dev and including the diffs in this PR?

/cc @erwinmombay

Edit: nvm, looks like you've done it in a new commit. Thanks.

Don't use yarn upgrade, use yarn install.

It's an error that happens on travis.

Because you upgraded the yarn.lock to use unsupported versions. Revert changes to yarn.lock, then run yarn install and it'll work again.

# Conflicts: # yarn.lock

googlebot · 2017-08-07T17:06:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-08-07T17:55:56Z

CLAs look good, thanks!

Raphaeljunior · 2017-08-07T18:15:05Z

@jridgewell PR ready for review.

dreamofabear

Nice work! Two more things:

Add some detail to the PR description -- what is the new default behavior? How do I use this new functionality?
Revert package-lock.json since we haven't switched to using it from yarn.lock yet.

dreamofabear · 2017-08-10T16:08:47Z

test/functional/test-xhrMock.js

+/**
+ * @fileoverview
+ * Asserts xhr fetch has been mocked out and under control
+ **/


Nit: Single *.

/** * */

dreamofabear · 2017-08-10T16:08:57Z

test/functional/test-xhrMock.js

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


dreamofabear · 2017-08-10T16:09:37Z

testing/describes.js

 import {setStyles} from '../src/style';
 import * as sinon from 'sinon';
-
+import fetchMock from 'fetch-mock';


Nit: Insert newline after this.

dreamofabear · 2017-08-10T16:09:46Z

testing/describes.js

 * @param {{
- *   win: !FakeWindowSpec,
+ *   win: !FakeWindow
+Spec,


dreamofabear · 2017-08-10T16:09:58Z

testing/describes.js

   * @param {string} name
-   * @param {!Object} spec
+   * @param {!Object} specconsole.log(fetchMock.sandbox());
+          debugger;


dreamofabear · 2017-08-10T16:18:01Z

testing/describes.js

+          env.expectFetch = function(url, response) {
+            return fetchMock.mock(url, response);
+          };
+        }


Let's move this block of code to a top-level helper function to avoid code dupe.

dreamofabear · 2017-08-10T16:18:43Z

build-system/tasks/presubmit-checks.js

    message: 'Use a sandbox instead to avoid repeated `#restore` calls',
  },
-  '(\\w*([sS]py|[sS]tub|[mM]ock|clock).restore)': {
+  '(\\w*([sS]py|[sS]tub|\\.[mM]ock|clock).restore)': {


Why this change?

To prevent the presubmit.js from blocking fetchMock.restore()

dreamofabear · 2017-08-10T16:18:58Z

extensions/amp-a4a/0.1/test/fetch-mock.js

@@ -1,4 +1,4 @@
-/**
+  /**


dreamofabear · 2017-08-10T16:19:47Z

extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js

        return initPromise.then(() => {
          expect(noContentSpy).to.be.calledWith(false);
          expect(iframeHandler.iframe).to.be.null;
+          done();


This shouldn't be necessary since we're returning a Promise in the test.

dreamofabear · 2017-08-10T16:20:26Z

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

      ampdoc: 'fie',
      runtimeOn: false,
    },
+    xhrMock: false,


Nit: How about mockFetch? Sounds more like an action/verb.

dreamofabear · 2017-08-10T16:23:59Z

@taymonbeal FYI, let us know if this works for you.

googlebot · 2017-08-18T21:37:09Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

dreamofabear · 2017-08-18T23:52:02Z

Now that @Raphaeljunior has left, closing this and forking into #11001 since it's a huge pain to patch with conflicts and ~80 commits.

Raphaeljunior added 3 commits August 1, 2017 14:13

Merge Sync

5726da0

Sync

0d2fc7c

xhr mocking feature

da79d92

googlebot added the cla: no label Aug 3, 2017

Raphaeljunior and others added 3 commits August 3, 2017 11:37

updates

2ec04a0

updates

befe36e

Merge branch 'xhr-mock' of github.com:Raphaeljunior/amphtml into xhr-…

6ba232b

…mock

googlebot added cla: yes and removed cla: no labels Aug 3, 2017

Raphaeljunior added 2 commits August 3, 2017 12:18

Travis fix

d032af6

Trial fix

509ccbf

This was referenced Aug 3, 2017

Fix errors due to missing dependencies #10673

Closed

Update yarn.lock and use node 6.0 on Travis #10671

Closed

rsimha reviewed Aug 3, 2017

View reviewed changes

Raphaeljunior added 13 commits August 3, 2017 14:19

Fix

eebc428

Merge Sync

7969297

Sync

c9396ef

xhr mocking feature

1e5ad50

updates

87eb5ce

Travis fix

b7b686f

Trial fix

24455b9

Fix

255384f

tmp

2ea25d0

Revert

d125a1b

Merge remote-tracking branch 'origin/xhr-mock' into xhr-mock

5d01f30

# Conflicts: # yarn.lock

xhr mock fixes

671b089

Fixed failing tests to conform to updates

e554b4b

Raphaeljunior added 8 commits August 7, 2017 12:58

Merge Sync

a08a01a

Sync

50fce55

Travis fix

514c9ef

Revert

5fd79bb

Fixed failing tests to conform to updates

75b50e8

Fixed presubmit

2f6c765

Merge conflicts fixed

e969993

Merge fixes

25b1948

googlebot added cla: no and removed cla: yes labels Aug 7, 2017

Fixes

2eef427

Raphaeljunior force-pushed the xhr-mock branch from 50d780e to 2eef427 Compare August 7, 2017 17:55

googlebot added cla: yes and removed cla: no labels Aug 7, 2017

dreamofabear self-requested a review August 10, 2017 16:06

dreamofabear reviewed Aug 10, 2017

View reviewed changes

dreamofabear requested a review from taymonbeal August 10, 2017 16:23

Github issues fixes

8916cce

jridgewell mentioned this pull request Aug 14, 2017

Create XhrMock test double #9338

Closed

revert indent

bf2ac05

googlebot added cla: no and removed cla: yes labels Aug 18, 2017

dreamofabear mentioned this pull request Aug 18, 2017

Integrate fetch-mock into describes.js #11001

Merged

dreamofabear closed this Aug 18, 2017

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

Conversation

Raphaeljunior commented Aug 3, 2017

Uh oh!

googlebot commented Aug 3, 2017

Uh oh!

googlebot commented Aug 3, 2017

Uh oh!

rsimha commented Aug 3, 2017

Uh oh!

rsimha Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Aug 7, 2017

Uh oh!

googlebot commented Aug 7, 2017

Uh oh!

Raphaeljunior commented Aug 7, 2017

Uh oh!

dreamofabear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Aug 10, 2017

Uh oh!

googlebot commented Aug 18, 2017

Uh oh!

dreamofabear commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rsimha Aug 3, 2017 •

edited

Loading