Somewhere between bug 744323 being submitted for review and it landing, a few of the Python functional tests broke. This is a follow-up bug to address those regressions.

This patch makes the JS server pass the Python functional tests: python syncstorage/tests/functional/test_storage.py http://localhost:8080/ ...s...............s.............s. ---------------------------------------------------------------------- Ran 35 tests in 23.420s OK (skipped=3) Those older and newer changes were made during review when the server landed. I think the original logic was right and we were all just reading the code wrong. I think the _() slipped in during a qref. Oh, Mercurial.

Comment on attachment 629748 [details] [diff] [review] Make pass functional tests, v1 Review of attachment 629748 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/common/storageserver.js @@ +406,5 @@ > } > } > > if (options.older) { > + if (bso.modified >= options.older) { Don't forget to follow up with Ryan about spec changes and the additional test he wrote.

Yeah, Ryan already made the changes to the functional tests. The spec could still use a little rewording...

Patch requires bug 757860. No big deal. I'll just wait.

Since this hasn't landed yet, I'm going to overload it to include support for X-If-Unmodified-Since, which is currently missing from a few URI endpoints. This is related to bug 762147.

Comment on attachment 630909 [details] [diff] [review] Part 2: Add support for X-I-U-S on collection URIs I discovered an open issue in the implementation in bug 762147. Will wait for clarification from rfk (in the form of MOAR tests) then will resubmit for review once I've verified behaviour is sane.

Comment on attachment 630909 [details] [diff] [review] Part 2: Add support for X-I-U-S on collection URIs Review of attachment 630909 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/common/storageserver.js @@ -767,1 @@ > I see this as applying to services/common/storageserver.js, not services/common/tests/unit/storageserver.js. Intentional? @@ +791,5 @@ > + continue; > + } > + > + serverModified = bso.modified; > + } I don't think this logic is correct, in that it doesn't match the behavior of the production server. The modified time is a property of the collection as a whole. We don't care about individual records for POST: > 412 Precondition Failed: an object in the collection has been modified since the timestamp in the X-If-Unmodified-Since header. A closer approximation to the correct logic is to: let serverModified = 0; for (let bso of this._bsos) { if (bso.modified > serverModified) { serverModified = bso.modified; } } but note that this does not behave correctly for an empty collection, which has a modified time and can return 412. You might need to switch to tracking collection modified times.

You obviously didn't see the review cancellation and linked bug where I pointed out the implementation was likely wrong, did you? ;)

(In reply to Gregory Szorc [:gps] from comment #8) > You obviously didn't see the review cancellation and linked bug where I > pointed out the implementation was likely wrong, did you? ;) Curse you, Szorc.

New patch. Does things properly.

Comment on attachment 632804 [details] [diff] [review] Part 2: Add support for X-I-U-S on collection URIs, v2 Review of attachment 632804 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK to me… ::: services/common/storageserver.js @@ +772,5 @@ > } > > + if (request.hasHeader("x-if-unmodified-since")) { > + let requestModified = parseInt(request.getHeader("x-if-unmodified-since"), > + 10); Man, you're strict on these line lengths, huh? @@ +784,5 @@ > + "time is newer than client's time."); > + response.setStatusLine(request.httpVersion, "412", "Precondition Failed"); > + return; > + } > + Nit: surplus newline. @@ +809,5 @@ > + let serverModified = this.timestamp; > + > + this._log.debug("Request modified time: " + requestModified + > + "; Server modified time: " + serverModified); > + if (serverModified > requestModified) { Invert this conditional and the log statement to match the… hey, wait a minute! This code is entirely duplicated from the last method! Pull this code out into a separate method, willya? `if (!ensureUnmodifiedSince(request, response)) { return; }`? @@ +1246,5 @@ > /** > * HTTP response utility. > */ > respond: function respond(req, resp, code, status, body, headers, timestamp) { > + this._log.info("Response: " + code + " " + status); Debuggery?

(In reply to Richard Newman [:rnewman] from comment #11) > @@ +1246,5 @@ > > /** > > * HTTP response utility. > > */ > > respond: function respond(req, resp, code, status, body, headers, timestamp) { > > + this._log.info("Response: " + code + " " + status); > > Debuggery? Initially, yes. But, I think it is useful. A server should log and request and response parameters, right? Either way, this file isn't used in production, so meh.

Can those tests be run on tbpl?

(In reply to :Ms2ger from comment #14) > Can those tests be run on tbpl? Not easily, no. The problem is somewhat complicated because it involves the mess that is Python packaging of the server code. We don't want to burden buildbot with this dependency. Anyway, bug 760128 tracks a more robust, continuous integration solution.