749336 - Implement mock AITC server in JS

Status: RESOLVED FIXED

(Web Apps Graveyard :: AppsInTheCloud, defect)

Description

[Anant Narayanan [:anant]]

A range of XPCShell tests for AITC client functionality is a pre-requisite for landing the client. Greg has already noted a few kinds of tests we'll need, which I will expand on:

- Test PUTting an app on the server
- Test GETting a list of apps, and proper receipt of 304 status codes
- Test if the client obeys X-Backoff and Retry-After headers
- Simulate error conditions and verify client behavior:
-- Client installs an app, AITC is down
-- Client installs an app, PUT never reaches server
-- Client installs an app, PUT reaches server but response is lost

Comment 1

[Anant Narayanan [:anant]]

The mock server must comply to the API documented at: https://github.com/mozilla-services/docs/blob/master/source/aitc/apis-1.0.rst

Comment 2

[Gregory Szorc [:gps]]

For the mock server, you should design the server as so it is reusable. Currently, this means writing something that looks like a JS module but lives as a xpcshell "head" file (which is included in other test files). I have patches pending in bug 748490 which will allow test-only JS modules to be installed and accessible via resource://testing/ in test code. See bug 744323 for an example.

Comment 3

[:Harald Kirschner :digitarald]

Gregory Szorc offered his XPCShell and mock server expertise to build the foundation for the AITC mock server; so its build on best practices and well integrated.

Comment 4

[Gregory Szorc [:gps]]

The test HTTP server will use a new path prefix handler API. This will make the code much nicer and compliant with the public IDL interface (rather than hacking internals, which is what Sync's test server does).

Comment 5

[Gregory Szorc [:gps]]

Attachment: Initial draft

Here is the skeleton for the server. I haven't tested that it actually works or that the JavaScript even parses! But, it should give you an idea of what the API will look like so you can be unblocked from writing tests that utilize it.

Comment 6

[Jason Smith [:jsmith]]

So this bug is tracking the implementation of the mock server, correct?

Comment 7

[Gregory Szorc [:gps]]

(In reply to Jason Smith [:jsmith] from comment #6)
> So this bug is tracking the implementation of the mock server, correct?

Yeah. Let's change the bug to be narrowly tailored. We can file additional bugs for other work.

Comment 8

[Anant Narayanan [:anant]]

For the individual client tests, I've filed bug 750948 to track this.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Semi-working server

This AITC server is kinda usable. It passes some of the Python functional tests. Still failing about half of them.

Comment 10

[Gregory Szorc [:gps]]

Attachment: AITC 1.0 Server, v1

More polished patch. Lots of functionality still missing.

The major change with this version is that the code is written as a JS module. So, you Cu.import it instead of merely loading the file wholesale into the current scope. You can also run a standalone server via `make -C services/common aitc-server`

This patch requires the patches in bug 755339, bug 755196, bug 757860, and bug 744323 to be applied to work. You can grab the latest/greatest versions from my Mercurial patch queue at https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc.

This bug will be blocked on landing by buildbot changes, which are being tracked in bug 755339 and bug 757460. I have an escape hatch if we really need it. But, I'd prefer to do it right from the start.

Comment 11

[Gregory Szorc [:gps]]

Attachment: AITC 1.0 Server, v2

This should be ready for review. It doesn't pass all the functional unit tests yet. It is mainly missing support for the devices API. There are also some bugs in the Python server and/or spec that I discovered when running the functional tests against this server. Once those are fixed, a few more Python functional tests should pass.

Comment 12

[Gregory Szorc [:gps]]

With new AITC patches pulled, the 2 server/test/spec test failures have disappeared. The only remaining test failures are in the devices API \o/. If the reviewer so wishes, I could implement these. Otherwise, I'm content with leaving them to be a follow-up.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 628699 [details] [diff] [review]
AITC 1.0 Server, v2

Review of attachment 628699 [details] [diff] [review]:
-----------------------------------------------------------------

Fewer tests than I normally expect from you, but that's fine -- there's a whole separate test suite.

Thumbs up for continuing to submit patches that could be framed and hung on a wall.

Note that I have *not* verified this against the spec. I trust that the functional test suite will suffice for that.

::: services/common/aitcserver.js
@@ +51,5 @@
> +  /**
> +   * Obtain the apps for this user.
> +   *
> +   * This is a generator of objects representing the app. Returns the original
> +   * app object normally or an abbreviated version if minimal is truish.

Each use of "app" should be "apps", no?

And backticks around `minimal` to indicate that it's a token.

@@ +54,5 @@
> +   * This is a generator of objects representing the app. Returns the original
> +   * app object normally or an abbreviated version if minimal is truish.
> +   */
> +  getApps: function getApps(minimal) {
> +    let result

Semicolon.

@@ +164,5 @@
> +   * Calls the specified callback when the server is stopped.
> +   */
> +  stop: function stop(cb) {
> +    let handler = {
> +      onStopped: function onStopped() { cb(); }

onStopped: cb

@@ +210,5 @@
> +    if (path.indexOf("/1.0/") != 0) {
> +      throw new Error("generalHandler invoked improperly.");
> +    }
> +
> +    let rest = request.path.substr(5);

"/1.0/".length, not 5.

And lift out the version into a const at the top.

@@ +265,5 @@
> +    } else if (!remaining.indexOf("apps/")) {
> +      let id = remaining.substr(5);
> +      //if (!this.ID_REGEX.test(id)) {
> +      //  throw HTTP_404;
> +      //}

…

@@ +365,5 @@
> +    let handlers = {
> +      GET: this._appsAppGetHandler,
> +      PUT: this._appsAppPutHandler,
> +      DELETE: this._appsAppDeleteHandler,
> +    };

Can't we use `handlers` in place of `allowed`? You don't use the value in `allowed`, only presence of the verb.

Also, how 'bout making `handlers` into `this._appsAppHandlers`, and avoiding the overhead of a new object on every call of this method?

@@ +367,5 @@
> +      PUT: this._appsAppPutHandler,
> +      DELETE: this._appsAppDeleteHandler,
> +    };
> +
> +    return handlers[request.method].bind(this)(user, id, request, response);

Super swish, but isn't this equivalent to

  return handlers[request.method].call(this, user, id, request, response);

but with the added overhead of instantiating a bound function?

@@ +410,5 @@
> +      throw HTTP_415;
> +    }
> +
> +    let requestBody = CommonUtils.readBytesFromInputStream(
> +                                    request.bodyInputStream);

One line, please.

@@ +494,5 @@
> +  },
> +
> +  _devicesHandler: function _devicesHandler(user, path, request, response) {
> +    // TODO need to support full API.
> +    // For now, we just assume it is a request for /.

File a follow-up for this and the other TODOs?

@@ +502,5 @@
> +    response.setStatusLine(request.httpVersion, 200, "OK");
> +    response.bodyOutputStream.write(body, body.length);
> +  },
> +
> +  // Surely this exists elsewhere in the Mozilla source tree...

Alas.

@@ +521,5 @@
> +    }
> +
> +    return params;
> +  },
> +};

Trailing comma is the style now, mm?

::: services/common/tests/unit/test_aitc_server.js
@@ +107,5 @@
> +      server.stop(run_next_test);
> +    });
> +  });
> +});
> +

Can we get a test for a request with a verb that's not acceptable?

Comment 14

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #13)
> Fewer tests than I normally expect from you, but that's fine -- there's a
> whole separate test suite.

Those were my thoughts as well. I have little desire to duplicate existing functionality.

> @@ +54,5 @@
> > +   * This is a generator of objects representing the app. Returns the original
> > +   * app object normally or an abbreviated version if minimal is truish.
> > +   */
> > +  getApps: function getApps(minimal) {
> > +    let result
> 
> Semicolon.

*shriek* I can't believe I did that. I wish there were a mode (like strict) that enforced this.
 
> @@ +164,5 @@
> > +   * Calls the specified callback when the server is stopped.
> > +   */
> > +  stop: function stop(cb) {
> > +    let handler = {
> > +      onStopped: function onStopped() { cb(); }
> 
> onStopped: cb

*facepalm*


> @@ +521,5 @@
> > +    }
> > +
> > +    return params;
> > +  },
> > +};
> 
> Trailing comma is the style now, mm?

Yes. If you leave it out, that line needs to get touched if you add something else to the prototype/object. I think that is silly. You should just be able to add/delete lines wholesale without worrying about their context in the parent object.

Comment 15

[Gregory Szorc [:gps]]

I refactored the code to not rely on testing modules.

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/bc696dba534d

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/bc696dba534d

Comment 18

[Jason Smith [:jsmith]]

Mock server is meant for testing - does not need QA verification here.