471158 - Add getSharedState/setSharedState functionality to SJS

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Jeff Walden [:Waldo]]

Make state-saving functions save information by default only for the single SJS where they're used.  Saving and sharing across different handlers is likely less common, so we should make this more-dangerous behavior opt-in, with a more dangerous-sounding name.  This will make it harder for state-saving tests to not clean up correctly and affect execution of other tests.

Depending on the constraints the server imposes on the request's path, it may be possible to do this with a little munging of the request's path into the key provided to [gs]etState (and doing similar in [gs]etSharedState with a invalid "path", to avoid collisions between the two stores of data.  It's probably feasible to do this, but I haven't actually investigated implementation yet; I think using a "real" key consisting of <path>+"#"+key (or "shared#"+key) should work, but I haven't tried to code anything up yet.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

At the moment nothing except the server test exercises this, so there are no existing consumers to change.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So the only test i've written so far that uses getState/setState uses a separate sjs file to set a state, and then the main sjs file gets that state. If this is a common pattern than shared state will be the common case.

I don't feel strongly either way though.

Comment 3

[Jeff Walden [:Waldo]]

Seems like it should be possible to put a shortcut at the top of the test, e.g.:

function handleRequest(request, response)
{
  response.setHeader("Cache-Control", "no-cache", false);
  if (request.queryString === "retrieve-result")
  {
    response.setHeader("X-Test-Final-Result", getState("state"), false);
    return;
  }

  // ...
}

Or something like that.  Is that a reasonable way to take advantage of this?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, that'd work

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 354532 [details] [diff] [review]
Patch

This is cool, will save a lot of mistakes from test creators.

Few thinks:
nsIHttpServer.setState and getState methods would be better to call set/getPrivateState or PathState

Did you run the test? Maybe I overlook something but there seems to be few thinks wrong:
- in function done() you are testing private (path) state of all tested paths but you request "foopy" state and not the "private-state" you are setting/getting in all handlers
- /path-handler requests no longer sets the state (now shared) value to "back to SJS!" but only to "to private SJS!" but it is not reflected in the check (start_handler and start_handler_again functions)
- it would be imho better to use "foopy" for all the private and shared states to check it doesn't leak from shared to private or opposite (mainly after we change something)
- also would be good to use different values in each handler for private state than just "can't touch this" for all of them, again to check it doesn't leak among private states

Comment 6

[Jeff Walden [:Waldo]]

Attachment: More readable tests

Yeah, the tests are kind of a mess, with state not being immediately obvious from the URL being loaded.  How about this instead?  I think it's much clearer in that respect, and it's probably different enough I really should run it past you again.  Note that state1.sjs and state2.sjs are exactly identical files, which the diff in Bugzilla probably won't make clear.

I considered the [gs]etPrivateState name, but I think brevity is a slightly more compelling argument than explicitness, especially since including private in the name requires cognizance of the difference between private and shared methods even if only private is necessary.  With this naming scheme, only if you want to share across files is knowledge of what shared means necessary.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 357519 [details] [diff] [review]
More readable tests

+test = new Test("http://localhost:4444/state2.sjs?2" +
The "2" at the end is probably a left over?

All tests silently fail:
*** syntax error in SJS at d:\mozilla\mozilla-central\netwerk\test\httpserver\test\data\sjs\state2.sjs: SyntaxError: missing ] in index expression
*** errorCode == 500

You are missing the ']' at the end:
params[decodeURIComponent(match[1]) = decodeURIComponent(match[2]);


The tests are really transparent this way. Just a nit - why do you always clear the shared state at the end of every test for a path? It would be perfect to demonstrate the shared state passes across requests to different paths.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 357519 [details] [diff] [review]
More readable tests

One more detail, but it is just a warning:

*** mapping '/' to the location d:\mozilla\mozilla-central\netwerk\test\httpserver\test\data\sjs
../../../_tests/xpcshell-simple/test_necko//test/test_sjs_state.js:205: strict warning: redeclaration of function start_last:
../../../_tests/xpcshell-simple/test_necko//test/test_sjs_state.js:205: strict warning: function start_last(ch, cx)

../../../_tests/xpcshell-simple/test_necko//test/test_sjs_state.js:205: strict warning: .........^

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Passing tests don't necessarily work

Those tests passed on my machine, but a glance at the log shows they were clearly failing.  The big problem?  An exception thrown from onStartRequest is swallowed and aborts the load, but as the exception wasn't being thrown by a test-checking function, it wasn't making the tests fail.  A fix for that revealed the same bug preventing another test from working, although not nearly as catastrophically as this one.  With that change, and with a change to work around a bug where HTTP headers aren't added if the value of the header is empty, these tests pass and should actually do what they were intended to do.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 358284 [details] [diff] [review]
Passing tests don't necessarily work

Perfect.

Comment 11

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/1bd818d0e564

I'll land on 191 when I have time and the tree's green.