719609 - Basic unit tests for SPDY

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

We need some basic (to start) xpcshell unit tests for SPDY. They'll require having node.js 0.7.0 (or higher) installed locally (currently still in development).

Comment 1

[u408661]

Attachment: patch

Patch for node-spdy, our spdy server, and the xpcshell tests. Tests are disabled until we get node deployed on the infrastructure (will talk to releng & file a separate bug for that).

Comment 2

[Patrick McManus [:mcmanus]]

nick - brilliant!

I'd like to see a couple of POST cases - one with a small body and one with a larger one (128KB? It can be autogenerated 0's). And can we add a large GET response as well? right now we're never covering more than 2 data frames if I read those tests right.. (but I just read them quickly)

also make sure we can set a x-random-hdr in the request and it shows up on node?

I think with that small list of additions we have a basic litmus test in place and a framework for adding more.. which is the baseline we need.

awesome.

I want to run what you've got through in the debugger to make sure the multiplexing is happening.

Comment 3

[u408661]

Attachment: patch

Updated with the requested tests, and better still, all the tests pass!

Patrick, who should I r? for the the bits under testing/ ?

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Nick Hurley [:hurley] from comment #3)
>
> Patrick, who should I r? for the the bits under testing/ ?

I don't know for sure. Ted maybe?

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 590314 [details] [diff] [review]
patch

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

unfortunately the server isn't sending multiplexed streams.. this is the LOG3 for the multiplex test..

Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x5 Stream Ptr 0x2af759a8c060 Fin=0 Len=1
Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x5 Stream Ptr 0x2af759a8c060 Fin=0 Len=1
Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x5 Stream Ptr 0x2af759a8c060 Fin=1 Len=0
Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x7 Stream Ptr 0x2af759a8cc40 Fin=0 Len=1
Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x7 Stream Ptr 0x2af759a8cc40 Fin=0 Len=1
Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x7 Stream Ptr 0x2af759a8cc40 Fin=1 Len=0

::: netwerk/test/unit/xpcshell.ini
@@ +165,5 @@
>  # Bug 675039: test hangs consistently on Android
>  skip-if = os == "android"
> +[test_spdy.js]
> +# Bug 719609: this test has particular requirements
> +skip-if = true

I assume this is to be removed before its checked-in or when the test servers are .7 node enabled?

::: testing/moz-spdy/README.txt
@@ +1,1 @@
> +Test server for SPDY unit tests. To run it, you need node (not provided) and node-spdy (provided). Just run

please note that you need node .7

Comment 6

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 590314 [details] [diff] [review]
> patch
> 
> Review of attachment 590314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> unfortunately the server isn't sending multiplexed streams.. this is the
> LOG3 for the multiplex test..
> 
> Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x5 Stream Ptr
> 0x2af759a8c060 Fin=0 Len=1
> Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x5 Stream Ptr
> 0x2af759a8c060 Fin=0 Len=1
> Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x5 Stream Ptr
> 0x2af759a8c060 Fin=1 Len=0
> Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x7 Stream Ptr
> 0x2af759a8cc40 Fin=0 Len=1
> Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x7 Stream Ptr
> 0x2af759a8cc40 Fin=0 Len=1
> Start Processing Data Frame. Session=0x2af759a94c00 Stream ID 0x7 Stream Ptr
> 0x2af759a8cc40 Fin=1 Len=0

Hrm, maybe I need to send larger chunks of data. I've definitely seen multiplexing sending larger resources in chunks (alternating bits of images, watched them load appropriately). How did you get that log data, so I can double-check before uploading a new patch?

> ::: netwerk/test/unit/xpcshell.ini
> @@ +165,5 @@
> >  # Bug 675039: test hangs consistently on Android
> >  skip-if = os == "android"
> > +[test_spdy.js]
> > +# Bug 719609: this test has particular requirements
> > +skip-if = true
> 
> I assume this is to be removed before its checked-in or when the test
> servers are .7 node enabled?

Once the test servers have node 0.7.0, this will be removed.

> ::: testing/moz-spdy/README.txt
> @@ +1,1 @@
> > +Test server for SPDY unit tests. To run it, you need node (not provided) and node-spdy (provided). Just run
> 
> please note that you need node .7

Will do

Comment 7

[u408661]

Attachment: patch

Here's a new version of the patch, along with more robust checking of actual multiplexing (it ensures we get the data in at least 2 chunks, and that the chunks we get are alternating). I was right, I had to send larger chunks of data to get the server to send the responses multiplexed.

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 590761 [details] [diff] [review]
patch

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

Test fails for me...

...

Start Processing Data Frame. Session=0x2b6204494c00 Stream ID 0x5 Stream Ptr 0x2b620448c060 Fin=0 Len=1
Start Processing Data Frame. Session=0x2b6204494c00 Stream ID 0x5 Stream Ptr 0x2b620448c060 Fin=0 Len=1
Start Processing Data Frame. Session=0x2b6204494c00 Stream ID 0x5 Stream Ptr 0x2b620448c060 Fin=1 Len=0
Start Processing Data Frame. Session=0x2b6204494c00 Stream ID 0x7 Stream Ptr 0x2b620448cc40 Fin=0 Len=1
Start Processing Data Frame. Session=0x2b6204494c00 Stream ID 0x7 Stream Ptr 0x2b620448cc40 Fin=0 Len=1
Start Processing Data Frame. Session=0x2b6204494c00 Stream ID 0x7 Stream Ptr 0x2b620448cc40 Fin=1 Len=0

TEST-PASS | /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js | [testOnStartRequest : 51] 200 == 200

TEST-PASS | /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js | [testOnStartRequest : 52] true == true

TEST-PASS | /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js | [null : 111] true == true

TEST-PASS | /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js | [null : 112] true == true

TEST-PASS | /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js | [null : 113] true == true

TEST-UNEXPECTED-FAIL | /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js | false == true - See following stack:
JS frame :: /home/mcmanus/src/mozilla2/wd/spdy2/testing/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/mcmanus/src/mozilla2/wd/spdy2/testing/xpcshell/head.js :: _do_check_eq :: line 556
JS frame :: /home/mcmanus/src/mozilla2/wd/spdy2/testing/xpcshell/head.js :: do_check_eq :: line 577
JS frame :: /home/mcmanus/src/mozilla2/wd/spdy2/testing/xpcshell/head.js :: do_check_true :: line 591
JS frame :: /home/mcmanus/src/mozilla2/wd/obj-debug-spdy/_tests/xpcshell/netwerk/test/unit/test_spdy.js :: <TOP_LEVEL> :: line 114

...

* there still seems to be 2 bytes of data in each stream
* if sending more data creates multiplexing please convince yourself that it is deterministic, we don't need more random orange
* anything that counts the number of times ODA is invoked is a recipe for random orange - multiplexing and ODA are independent.

* The "start processing data frame" bits are from NSPR logging. I think that works in xpcshell, but if it doesn't I'll admit I just turned them into printfs to run the test.

Comment 9

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> * there still seems to be 2 bytes of data in each stream

Hrm, this is probably because of the compression (right now I'm sending a huge string of '0'). If I look at the amount of data in the buffer when onStopRequest is called, I get the amount I expect:

TEST-INFO | /Users/hurley/src/mozilla-central/obj-ff-dbg/_tests/xpcshell/netwerk/test/unit/test_spdy.js | buffer len for stream 3 = 204800

I guess I'll have to come up with something a bit more random for this test to defeat the compression.

> * if sending more data creates multiplexing please convince yourself that it
> is deterministic, we don't need more random orange

I'm quite convinced of this, having seen it (literally) in action with the images as I described above.

> * anything that counts the number of times ODA is invoked is a recipe for
> random orange - multiplexing and ODA are independent.

Fair enough, will have to figure something else out here.

> * The "start processing data frame" bits are from NSPR logging. I think that
> works in xpcshell, but if it doesn't I'll admit I just turned them into
> printfs to run the test.

That should work for me, then.

Comment 10

[Patrick McManus [:mcmanus]]

that's not compressed content - there is only 2 bytes of message body data. message bodies are self-complete gzip'd documents.. you can't make a gzip document in 2 bytes.

Comment 11

[u408661]

There's definitely something amiss on your end, then. Did you restart the server after updating the code? Node won't automagically re-load the server.

I'm rebuilding on my end right now (for some reason it decided to do a full rebuild), so I'll see if I get similar results to yours whenever it finishes. If so, there's something very, VERY strange going on here.

Comment 12

[Patrick McManus [:mcmanus]]

(In reply to Nick Hurley [:hurley] from comment #11)
> There's definitely something amiss on your end, then. Did you restart the
> server after updating the code? Node won't automagically re-load the server.
> 

I hadn't, you're right.

How are you going to make sure that happens as part of automated testing? For instance, the websockets server is stopped and started as part of the run-mochitests framework.

In any event, the test is marked as pass now - and I see a whole lot more data. But I don't see any multiplexing. I see syn-reply=5 , a long run of data id=5, a fin id=5, syn-reply=7, followed by a long run of data id=7, and a fin id=7.

Comment 13

[u408661]

I'm planning on doing something similar to websockets, I just didn't put that in the patch since we don't have anything on the infrastructure yet.

I'm also with you on the multiplexing, this obviously isn't working as I remembered it. I'll have to poke at it more to figure out what went wrong.

Comment 14

[u408661]

Attachment: patch

OK, I'd screwed up the multiplexing on the server end, this patch fixes that (as well as getting rid of dependency on ODA for checking, we just assume the server multiplexed properly, which it does every single time I've run this code so far).

Let me know if you think this looks good, if so, I'll r? you and Ted so we can get this rolling farther.

Comment 15

[u408661]

Comment on attachment 590877 [details] [diff] [review]
patch

Ted - r? on the bits under testing, feel free to hand it off to someone else if you're not the right person for this.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 590877 [details] [diff] [review]
patch

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

So this looks like a decent start. To make this more useful we'll have to have a way to auto-start the node server.

We can add a commandline argument to runxpcsheltests.py that passes the path to node, and set a key in the info dictionary appropriately (hasNode = True), so that you can do like:
skip-if = !hasNode

Then it's either a matter of:
a) Getting node installed on the test machines
or
b) Committing prebuilt binaries to the repo.

Comment 17

[Patrick McManus [:mcmanus]]

Can the progress on this bug be updated please?

Comment 18

[u408661]

Attachment: part 2 - harness changes

Here's part 2 (finally), which makes changes to runxpcshelltests.py to start the node spdy server before running our tests, and kill the server once we're done. This also sets the hasNode property in mozinfo appropriately once we know whether or not we have node available. The path was taken from discussions with Armen in bug 729392.

Comment 19

[u408661]

Attachment: part 2 - harness changes

Oops, had made some changes after making that patch that never got into the uploaded version. Here's the appropriate one.

Comment 20

[u408661]

Attachment: part 2 - harness changes

Slight update - per Armen in bug 729392, we aren't going to use a hard-coded path at all, so this just removes that possibility when trying to find node. Otherwise, it's the same as the previous instance of the patch.

Comment 21

[u408661]

Attachment: part 1 - new stuff

This is the same as the previous version of this patch, but with moz-spdy and node-spdy now living under testing/xpcshell/ instead of under testing/, so they get unzipped properly for running the xpcshell tests on the test servers.

Carry forward r+

Comment 22

[u408661]

Attachment: part 2 - harness changes

This updates finding moz-spdy for the changes made to the part 1 patch.

Comment 23

[u408661]

Attachment: part 1 - new stuff

OK, I'm obviously dumber than I look. Here's an update to the first part to actually package up node-spdy and moz-spdy with the xpcshell stuff so it'll actually be available on the test servers. The change to testing/xpcshell/Makefile.in is the only change between this one and the last version.

Comment 24

[u408661]

Attachment: part 1 - new stuff

Almost the same as the previous part 1, but allows another way of exiting the node process (which will be used by the new part 2, forthcoming)

Comment 25

[u408661]

Attachment: part 2 - harness changes

Fixes shutdownNode to work with the version of python available on the test servers, and hooks up the node child process to accept that.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 614936 [details] [diff] [review]
part 1 - new stuff

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

r- just for an answer on the cert stuff. Overall this looks sane.

::: netwerk/test/unit/test_spdy.js
@@ +1,2 @@
> +var Ci = Components.interfaces;
> +var Cc = Components.classes;

I'm not really going to review this test file, presumably you'll get review from a Necko peer.

@@ +332,5 @@
> +  do_get_profile();
> +  addCertOverride("localhost", 4443,
> +                  Ci.nsICertOverrideService.ERROR_UNTRUSTED |
> +                  Ci.nsICertOverrideService.ERROR_MISMATCH |
> +                  Ci.nsICertOverrideService.ERROR_TIME);

This seems unfortunate. Can't you get the cert in the right format and have your test add it as trusted? (You could even have a head_spdy.js that did that for you.)

::: testing/xpcshell/moz-spdy/moz-spdy.js
@@ +1,1 @@
> +var spdy = require('../node-spdy/lib/spdy.js');

I don't know the first thing about node, so presumably you'll get someone else to look at this?

::: testing/xpcshell/moz-spdy/spdy-ca.pem
@@ +7,5 @@
> +A3vk+3j3hjJjIm79rwIDAQABoAAwDQYJKoZIhvcNAQEFBQADgYEAiNWhz6EppIVa
> +FfUaB3sLeqfamb9tg9kBHtvqj/FJni0snqms0kPWaTySEPHZF0irIb7VVdq/sVCb
> +3gseMVSyoDvPJ4lHC3PXqGQ7kM1mIPhDnR/4HDA3BhlGhTXSDIHgZnvI+HMBdsyC
> +hC3dz5odyKqe4nmoofomALkBL9t4H8s=
> +-----END CERTIFICATE REQUEST-----

How did you generate these certs? Is it documented anywhere? Do they have an expiration date? If possible, it'd be really great to piggyback this on our existing cert setup that we use for Mochitest:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/Makefile.in#78
http://mxr.mozilla.org/mozilla-central/source/build/pgo/genpgocert.py.in
http://mxr.mozilla.org/mozilla-central/source/build/pgo/certs/

Comment 27

[u408661]

(In reply to Ted Mielczarek [:ted] from comment #26)

I should've mentioned in the review request, Patrick already went through all of the test and the node stuff, this r? from you was just to make sure you were cool with the changes in this associated with being able to control the process from runxpcshelltests.py, so all the other stuff is good (though the whole thing with adding cert exceptions is, in fact, unfortunate, but it's what we have)

> ::: testing/xpcshell/moz-spdy/spdy-ca.pem
> @@ +7,5 @@
> > +A3vk+3j3hjJjIm79rwIDAQABoAAwDQYJKoZIhvcNAQEFBQADgYEAiNWhz6EppIVa
> > +FfUaB3sLeqfamb9tg9kBHtvqj/FJni0snqms0kPWaTySEPHZF0irIb7VVdq/sVCb
> > +3gseMVSyoDvPJ4lHC3PXqGQ7kM1mIPhDnR/4HDA3BhlGhTXSDIHgZnvI+HMBdsyC
> > +hC3dz5odyKqe4nmoofomALkBL9t4H8s=
> > +-----END CERTIFICATE REQUEST-----
> 
> How did you generate these certs? Is it documented anywhere? Do they have an
> expiration date? If possible, it'd be really great to piggyback this on our
> existing cert setup that we use for Mochitest:
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/Makefile.in#78
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/genpgocert.py.in
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/certs/

These certs are the example ones that node-spdy supplies. It seemed easiest from the node end to just make use of those (given that node-spdy expects the cert, key, and ca all as separate pem-encoded files). We can definitely try to piggyback on the existing setup, though it looks (at first glance) like that might be more trouble than it's worth. I could easily be wrong, though.

Comment 28

[Patrick McManus [:mcmanus]]

(In reply to Ted Mielczarek [:ted] from comment #26)

> This seems unfortunate. Can't you get the cert in the right format and have
> your test add it as trusted? (You could even have a head_spdy.js that did
> that for you.)
> 

I'm CERTainly (hah!) not opposed to that, but let's keep a close eye on the ROI if this is a significant work item. What are the benefits and expected costs?

These tests are meant to cover spdy protocol items that are currently untested. To do that, TLS is a requirement. However base TLS functionality is unit-tested separately, and the spdy code uses the same base TLS code so it already shares that piece of the coverage... 

otoh if the suggestion is the current incantation will be unstable that's a differnet story.

Comment 29

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #28)
> otoh if the suggestion is the current incantation will be unstable that's a
> differnet story.

FWIW, if this incantantation is unstable, then there are other tests that will have problems (it seems to be a not uncommon incantation in xpcshell tests). Not saying it's great, of course, but it's not unprecedented.

Comment 30

[(not currently active) Ted Mielczarek]

We had issues with the test certs for the Mochitest harness expiring, so I'd definitely like to prevent that from happening.

I'm certainly not trying to block you by making you implement more than you need, I just want to make sure that we're not cutting corners here that will come back to bite us.

Do you expect that you'll need to use >1 cert in the future here, or will we simply run those tests in Mochitest (by hooking up the same node-spdy server there, possibly)?

Comment 31

[u408661]

Patrick may have other ideas here, but I don't imagine needing more than one cert in the future. Like Patrick said in comment 28, the TLS code is orthogonal to the SPDY code, so really having TLS here is purely a function of SPDY requiring TLS. Perhaps there's something coming down the SPDY pipeline that I'm unaware of, though.

Comment 32

[(not currently active) Ted Mielczarek]

Comment on attachment 614937 [details] [diff] [review]
part 2 - harness changes

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

I have mostly nits, but one big comment.

::: testing/xpcshell/runxpcshelltests.py
@@ +392,5 @@
>      """
>      return ['-e', 'const _TEST_FILE = ["%s"];' %
>                replaceBackSlashes(name)]
>  
> +  def setupNode(self):

Maybe name this trySetupNode, given that it will fail if node isn't present?

@@ +410,5 @@
> +        # including filename.
> +        nodeBin = localPath
> +      else:
> +        # Next, try assuming they gave us just the directory. We'll look
> +        # for both 'node' AND 'node.exe' in the directory.

This seems a little far-reaching. Can't we just require the full path to be set?

@@ +423,5 @@
> +    if nodeBin:
> +      self.log.info('Found node at %s' % (nodeBin,))
> +      myDir = os.path.split(os.path.abspath(__file__))[0]
> +      mozSpdyDir = os.path.join(myDir, 'moz-spdy')
> +      mozSpdyJs = os.path.join(mozSpdyDir, 'moz-spdy.js')

You can shortcut this to:
mozSpdyJs = os.path.join(myDir, 'moz-spdy', 'moz-spdy.js')

@@ +430,5 @@
> +        # OK, we found our SPDY server, let's try to get it running
> +        self.log.info('Found moz-spdy at %s' % (mozSpdyJs,))
> +        stdout, stderr = self.getPipes()
> +        try:
> +          nodeProc = Popen([nodeBin, mozSpdyJs], stdin=PIPE, stdout=PIPE,

You should mention here that you're piping stdin because moz-node will exit upon stdin being finished. (I had to go read moz-node to figure that out.)

Also, how verbose is moz-node? Is it worth not piping stderr here so that if something fails you have diagnostic output? In other circumstances, we've piped stderr, but echoed it to stdout if an error occurred (you could check resultcode, for example.)

@@ +436,5 @@
> +        except OSError, e:
> +          # This occurs if the subprocess couldn't be started
> +          self.log.error('Could not run node SPDY server: %s' % (str(e),))
> +
> +        if nodeProc and nodeProc.poll() is None:

I've never seen anyone check .poll(). You generally can assume that if spawning the Popen didn't raise an exception that your process is running. You still probably have a race condition here, in that you need moz-spdy to actually be listening on its listen port by the time the tests want to hit it. In Mochitest, we make httpd.js write a file to indicate that it has opened the socket for listening, and the Python harness waits on that. Crude but effective.

You could just stick this code inside the try block after the Popen and avoid having to check this at all.

@@ +450,5 @@
> +  def shutdownNode(self):
> +    """
> +      Shut down our node process, if it exists
> +    """
> +    if self.nodeProc:

You should probably initialize self.nodeProc to None in the constructor or class definition. It feels a little weird to rely on startNode setting it to None.

Comment 33

[u408661]

(In reply to Ted Mielczarek [:ted] from comment #32)
> Comment on attachment 614937 [details] [diff] [review]
> part 2 - harness changes
> 
> Review of attachment 614937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have mostly nits, but one big comment.
> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +392,5 @@
> >      """
> >      return ['-e', 'const _TEST_FILE = ["%s"];' %
> >                replaceBackSlashes(name)]
> >  
> > +  def setupNode(self):
> 
> Maybe name this trySetupNode, given that it will fail if node isn't present?

Makes sense; done.

> @@ +410,5 @@
> > +        # including filename.
> > +        nodeBin = localPath
> > +      else:
> > +        # Next, try assuming they gave us just the directory. We'll look
> > +        # for both 'node' AND 'node.exe' in the directory.
> 
> This seems a little far-reaching. Can't we just require the full path to be
> set?

Seems sensible to me. Also done.

> @@ +423,5 @@
> > +    if nodeBin:
> > +      self.log.info('Found node at %s' % (nodeBin,))
> > +      myDir = os.path.split(os.path.abspath(__file__))[0]
> > +      mozSpdyDir = os.path.join(myDir, 'moz-spdy')
> > +      mozSpdyJs = os.path.join(mozSpdyDir, 'moz-spdy.js')
> 
> You can shortcut this to:
> mozSpdyJs = os.path.join(myDir, 'moz-spdy', 'moz-spdy.js')

Yep. I think the mozSpdyDir was leftovers from a previous run at this where I screwed something up :)

> @@ +430,5 @@
> > +        # OK, we found our SPDY server, let's try to get it running
> > +        self.log.info('Found moz-spdy at %s' % (mozSpdyJs,))
> > +        stdout, stderr = self.getPipes()
> > +        try:
> > +          nodeProc = Popen([nodeBin, mozSpdyJs], stdin=PIPE, stdout=PIPE,
> 
> You should mention here that you're piping stdin because moz-node will exit
> upon stdin being finished. (I had to go read moz-node to figure that out.)

Done.

> Also, how verbose is moz-node? Is it worth not piping stderr here so that if
> something fails you have diagnostic output? In other circumstances, we've
> piped stderr, but echoed it to stdout if an error occurred (you could check
> resultcode, for example.)

moz-spdy should only print one thing, to stdout (a notice that it's running on port 4443). Perhaps I should use that to ensure the server has actually bound to the port, instead of using a file as you suggest below?

> @@ +436,5 @@
> > +        except OSError, e:
> > +          # This occurs if the subprocess couldn't be started
> > +          self.log.error('Could not run node SPDY server: %s' % (str(e),))
> > +
> > +        if nodeProc and nodeProc.poll() is None:
> 
> I've never seen anyone check .poll(). You generally can assume that if
> spawning the Popen didn't raise an exception that your process is running.
> You still probably have a race condition here, in that you need moz-spdy to
> actually be listening on its listen port by the time the tests want to hit
> it. In Mochitest, we make httpd.js write a file to indicate that it has
> opened the socket for listening, and the Python harness waits on that. Crude
> but effective.

See my above comment on using the first line of moz-spdy's stdout

> You could just stick this code inside the try block after the Popen and
> avoid having to check this at all.

Sensible. Done.

> @@ +450,5 @@
> > +  def shutdownNode(self):
> > +    """
> > +      Shut down our node process, if it exists
> > +    """
> > +    if self.nodeProc:
> 
> You should probably initialize self.nodeProc to None in the constructor or
> class definition. It feels a little weird to rely on startNode setting it to
> None.

Why I didn't do that in the first place, I honestly have no clue. Done.

Comment 34

[(not currently active) Ted Mielczarek]

Okay. In that case, the sole changes I'd like to see in that integration patch is that you:
a) Document where those certs came from just for sanity's sake.
b) Convert the CA cert to a NSS-readable format and explicitly use the PSM APIs to add it as a trusted cert instead of jumping through the "this cert is untrusted then add it as trusted" hoops.

Comment 35

[(not currently active) Ted Mielczarek]

Also, sorry for the delay on the reviews!

Comment 36

[u408661]

(In reply to Ted Mielczarek [:ted] from comment #34)
> Okay. In that case, the sole changes I'd like to see in that integration
> patch is that you:
> a) Document where those certs came from just for sanity's sake.

Simple enough.

> b) Convert the CA cert to a NSS-readable format and explicitly use the PSM
> APIs to add it as a trusted cert instead of jumping through the "this cert
> is untrusted then add it as trusted" hoops.

Looking at this, it seems as if the certs provided are already expired, so just adding the CA cert as a trusted root won't help. I could always generate new certs, with an expiration say 100 years in the future (if these tests are still being used then, I will be both shocked and proud of myself for writing something with such staying power).

I remember discussing the proper approach for the certs with bsmith and honza back when I started working on the tests, and we came to the consensus that doing what I'm doing now was actually going to be the most reliable thing to do, at least for programmatically making sure the cert would be accepted. I may be misremembering, it was a while ago. CC'd both of them to get their input.

If it's just a matter of generating certs with far-future expiry and adding the CA as trusted using the PSM APIs, then cool. If not, we might have to figure out how to piggyback this all on the mochitest cert infrastructure, as you originally suggested.

Comment 37

[(not currently active) Ted Mielczarek]

Adding new non-expired certs seems okay, as long as you also document how they were generated. (I think the Mochitest certs are set to expire in 10 years or something.)

Comment 38

[u408661]

(In reply to Ted Mielczarek [:ted] from comment #37)
> Adding new non-expired certs seems okay, as long as you also document how
> they were generated. (I think the Mochitest certs are set to expire in 10
> years or something.)

So I just talked to bsmith, and it seems we currently don't have any way (other than using the cert override service, which is what the current iteration of the test does) to make xpcshell tests accept whatever cert we want (which is why I did it this way in the first place). There is apparently work being done to make it possible, but we don't want to block this on that work, given this is more important (spdy is on by default in 13 and 14, meaning we'll have to request approval for both those releases to get some actual testing done). Given all these constraints, as well as the fact that we already use the cert override service in *other* xpcshell tests for similar issues, we want to just get this committed as-is (I'll make the changes to the README for documentation, of course), and then we can fix *all* the uses of the cert override service at some point in the future when the fix lands (there is a bug already open for this, but I'm not 100% sure of the bug number... perhaps Brian can chime in with that info).

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Bug 466524.

Comment 40

[u408661]

Attachment: part 1 - new stuff

Here's the part 1 patch with an updated README about the source of the certs.

Ted, I'm carrying forward the r+ that Patrick (and you) gave me on the first round of this, based on the results of the cert situation. If you disagree, r- this so we can get it figured out (I don't plan on landing this until tomorrow, at which point it will be landed on m-c and approval requested for both aurora and beta).

Comment 41

[u408661]

Attachment: part 2 - harness changes

Here's the part 2 updated to address Ted's comments. Carrying forward r+

Comment 42

[u408661]

There's a try build running with these changes at https://tbpl.mozilla.org/?tree=Try&rev=0e57865ceab6 that I am going to have releng run with their changes (see bug 729392) to make sure I didn't screw anything up with my updates above. Assuming it all checks out, we'll get this landed and approved for 13+ so we can finally have tests for SPDY.

Comment 43

[(not currently active) Ted Mielczarek]

Okay, that's depressing, but I accept your assessment.

Comment 44

[u408661]

OK, integration between this and releng changes in bug 729392 look all good, let's go ahead and land this on m-c, then we can get approval for aurora & beta.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/301bf79e9029
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d3c5935c19

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/301bf79e9029
https://hg.mozilla.org/mozilla-central/rev/d0d3c5935c19

Comment 47

[Alex Keybl [:akeybl]]

Please nominate these NPOTB changes for Aurora/Beta when you get the chance, but we only track issues for specific releases of Firefox.