Create XhrMock test double by taymonbeal · Pull Request #9338 · ampproject/amphtml

taymonbeal · 2017-05-15T16:02:38Z

dvoytenko · 2017-05-15T17:21:25Z

src/service/xhr-impl.js

    /** @const {!Window} */
    this.win = win;
+    /** @type {function(this:Xhr, string, !FetchInitDef): !Promise<!FetchResponse>} */
+    this.fetchImpl = win.fetch ? this.nativeFetch_ : fetchPolyfill;


Can it be private?

xhr-mock.js mutates it. Should it be private given that?

dvoytenko · 2017-05-15T17:22:32Z

src/service/xhr-impl.js

 *
 * See https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch
+ *
+ * @record


Why is this not in externs? Or at least defined as @interface?

What I was trying to do here was create a structural interface that would be satisfied by all three kinds of response object: browser-native Response, FetchPolyfillResponse as defined in this file, and MockFetchResponse as defined in xhr-mock.js. I'm not sure it entirely worked, since I still had to put type assertions in a couple places. How would you advise doing this?

I would just leave this as it was, and do proper unit testing of the mock implementation.

dvoytenko · 2017-05-15T17:23:45Z

src/service/xhr-impl.js

-   * Create a copy of the response and return it.
-   * @return {!FetchResponse}
-   */
  clone() {


All of these should have /** @override */

dvoytenko · 2017-05-15T17:24:07Z

src/service/xhr-impl.js

+ * interface is implemented by native `Headers` and by the polyfill and mock
+ * response headers types.
+ *
+ * @record


Ditto, @interface

dvoytenko · 2017-05-15T17:24:29Z

src/service/xhr-impl.js

-   * @param {string} name
-   * @return {string}
-   */
  get(name) {


Ditto: /** @override */

dvoytenko · 2017-05-15T17:26:55Z

@taymonbeal The double itself looks good. I'd still like to see how and where it helps in testing.

taymonbeal · 2017-05-15T18:11:51Z

I designed this while simultaneously using it to write new tests for PR #9040. Those tests haven't been pushed yet because they necessarily rely on this PR, but here's an excerpt:

  const expectRequest = (serviceName, cachebusterKid = null) =>
      ({method, url, headers}) => {
        expect(method).to.equal('GET');
        let expectedUrl = endpoints[serviceName];
        if (cachebusterKid) {
          expectedUrl += '?kid=' + cachebusterKid;
        }
        expect(url).to.equal(expectedUrl);
        expect(headers['Origin']).to.equal(origin);
      };
  const makeResponse = (serviceName, kids) => ({
    status: 200,
    headers: {'Content-Type': 'application/jwk-set+json'},
    body:
        JSON.stringify({'keys': kids.map(kid => publicJwks[serviceName][kid])}),
  });
  const entry = (serviceName, kids, cachebusterKid = null) => ({
    request: expectRequest(serviceName, cachebusterKid),
    response: Promise.resolve(makeResponse(serviceName, kids)),
  });

  it('should verify a signature', () => {
    xhrMock.use(origin, [entry('service-1', ['key-1'])]);
    verifier.loadKeyset('service-1');
    return expect(verifier.verify(
                      'service-1', 'key-1', signatures['service-1']['key-1'][0],
                      creatives[0]))
        .to.eventually.be.null;
  });

  it('should verify multiple signatures with only one network request', () => {
    xhrMock.use(origin, [entry('service-1', ['key-1'])]);
    verifier.loadKeyset('service-1');
    return verifier
        .verify(
            'service-1', 'key-1', signatures['service-1']['key-1'][0],
            creatives[0])
        .then(failure => {
          expect(failure).to.be.null;
          verifier.loadKeyset('service-1');
          return expect(verifier.verify(
                            'service-1', 'key-1',
                            signatures['service-1']['key-1'][1], creatives[1]))
              .to.eventually.be.null;
        });
  });

  it('should verify a signature from a newly added key', () => {
    xhrMock.use(origin, [
      entry('service-1', ['key-1']),
      entry('service-1', ['key-2'], 'key-2'),
    ]);
    verifier.loadKeyset('service-1');
    return expect(verifier.verify(
                      'service-1', 'key-2', signatures['service-1']['key-2'][0],
                      creatives[0]))
        .to.eventually.be.null;
  })

jridgewell · 2017-05-15T20:59:25Z

src/service/xhr-impl.js

 *
 * See https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch
+ *
+ * @record


I would just leave this as it was, and do proper unit testing of the mock implementation.

jridgewell · 2017-05-15T21:02:18Z

testing/xhr-mock.js

+   */
+  constructor(baseUrl, entries) {
+    /** @private {!Location} */
+    this.baseUrl_ = typeof baseUrl == 'string' ?


This should always be a string.

dvoytenko · 2017-05-16T00:43:59Z

@taymonbeal I see. I'd like to pass it on to @jridgewell and @aghassemi - they are more knowledgeable in tests and mocks.

jridgewell · 2017-05-16T16:03:44Z

Looking into this, why don't we go with an npm module? fetch-mock is active, and has a much nicer API.

taymonbeal · 2017-05-16T16:41:33Z

I recall having looked at that at one point. Will it work with our Xhr services that sometimes use XMLHttpRequest instead?

jridgewell · 2017-05-16T16:48:45Z

Looks like we need to set it up with our fetch polyfill:

fetchMock.setImplementations(fetchPolyfill);

Then, as long as there's a window.fetch, XHR will use it.

taymonbeal · 2017-05-24T19:12:18Z

This is now a blocking issue for #9040, because that's going to get broken up into multiple PRs (it got too unwieldy to ship all at once and some parts are needed sooner for Origin Trials) and I need to refactor the existing tests to make them not depend on whether the old vs. new implementation is used.

I tried integrating fetch-mock myself but rapidly got lost due to not understanding how to make it work with the existing test doubles like in describes.js. @erwinmombay and I talked about this yesterday. Erwin, what would you say is the ETA on integrating fetch-mock? If it's not soon or not a priority, is there anything I can do to help? I have time to spend on this, but not expertise in AMP test doubles.

dvoytenko · 2017-08-14T23:33:36Z

Close or still wip?

jridgewell · 2017-08-14T23:48:52Z

Raph took over this in #10776, though he's gone now.

taymonbeal · 2017-08-29T19:58:31Z

Obsoleted by #11001.

Create XhrMock test double

d083192

googlebot added the cla: yes label May 15, 2017

dvoytenko suggested changes May 15, 2017

View reviewed changes

taymonbeal added 2 commits May 15, 2017 14:07

Add @OverRide to Response impls

3f49b6f

Add @OverRide to MockFetchResponse and ...Headers

07ac4a0

jridgewell reviewed May 15, 2017

View reviewed changes

dvoytenko requested a review from aghassemi May 16, 2017 00:44

taymonbeal mentioned this pull request Jun 27, 2017

Testing: Easier stubbing of services #10128

Closed

dreamofabear mentioned this pull request Jul 17, 2017

Support XHR mocking in describes.js #10477

Closed

aghassemi removed their request for review July 24, 2017 15:58

taymonbeal closed this Aug 29, 2017

taymonbeal deleted the mock-xhr branch August 29, 2017 19:58

Conversation

taymonbeal commented May 15, 2017

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!

dvoytenko commented May 15, 2017

Uh oh!

taymonbeal commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented May 16, 2017

Uh oh!

jridgewell commented May 16, 2017

Uh oh!

taymonbeal commented May 16, 2017

Uh oh!

jridgewell commented May 16, 2017

Uh oh!

taymonbeal commented May 24, 2017

Uh oh!

dvoytenko commented Aug 14, 2017

Uh oh!

jridgewell commented Aug 14, 2017

Uh oh!

taymonbeal commented Aug 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taymonbeal commented May 15, 2017 •

edited

Loading