Closed
Bug 749336
Opened 13 years ago
Closed 13 years ago
Implement mock AITC server in JS
Categories
(Web Apps Graveyard :: AppsInTheCloud, defect)
Web Apps Graveyard
AppsInTheCloud
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: anant, Assigned: gps)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•13 years ago
|
||
The mock server must comply to the API documented at: https://github.com/mozilla-services/docs/blob/master/source/aitc/apis-1.0.rst
Assignee | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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.
Updated•13 years ago
|
Assignee: hkirschner → gps
Assignee | ||
Comment 4•13 years ago
|
||
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).
Depends on: 485255
Assignee | ||
Comment 5•13 years ago
|
||
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•13 years ago
|
||
So this bug is tracking the implementation of the mock server, correct?
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Summary: AITC client needs mock tests → Implement mock AITC server in JS
Reporter | ||
Comment 8•13 years ago
|
||
For the individual client tests, I've filed bug 750948 to track this.
Assignee | ||
Comment 9•13 years ago
|
||
This AITC server is kinda usable. It passes some of the Python functional tests. Still failing about half of them.
Attachment #619718 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Assignee | ||
Comment 10•13 years ago
|
||
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.
Attachment #620515 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 11•13 years ago
|
||
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.
Attachment #626525 -
Attachment is obsolete: true
Attachment #628699 -
Flags: review?(rnewman)
Assignee | ||
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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?
Attachment #628699 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 14•13 years ago
|
||
(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.
Assignee | ||
Comment 15•13 years ago
|
||
I refactored the code to not rely on testing modules.
Assignee | ||
Comment 16•13 years ago
|
||
Whiteboard: [fixed in services]
Assignee | ||
Comment 17•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Comment 18•13 years ago
|
||
Mock server is meant for testing - does not need QA verification here.
Whiteboard: [qa-]
Updated•6 years ago
|
Product: Web Apps → Web Apps Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•