Bug 1033402 - waitForSocket() before connecting to marionette. #115
Conversation
| // the logic is lifted from http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#562 | ||
| var sockit = new sockittome.Sockit(); | ||
| var start = Date.now(); | ||
| var self = this; |
|
Can we test this logic? |
| } catch(err) { | ||
| if (Date.now() - this._beginConnect >= this.connectionTimeout) { | ||
| callback(err); | ||
| var self = this; |
There was a problem hiding this comment.
This line is useless. Will remove.
|
Not sure there is an easy way to test, but I'll see. |
|
@gaye do you know how we could test this on try? If it doesn't break the current tree. |
|
@hfiguiere I meant like writing a test that could be executed with the rest of the unit + integration suites. In order to test on try, you can probably publish an alpha version, require that in a fork of gaia-node-modules, and then submit a pr to gaia that changes the remote endpoint for gaia-node-modules and then set the gaia revision to your patch's sha. |
| var s = sockit.read(16).toString(); | ||
| sockit.close(); | ||
| if (s.indexOf(":") != -1) { | ||
| setTimeout(callback, 5000); |
There was a problem hiding this comment.
That one come from the python stuff logic (see URL above). I'm wondering if it is really necessary.
There was a problem hiding this comment.
Yup, removing this timeout.
|
Concerning the unit test, there is currently no unittest for tcp-sync. The tcp-sync is actually a simple test of the client with it. It should be enough and already test this one. I more extensive unit test would be some work. I run this patch with Gaia on Travis and Gaia-Try and all pass. |
| /** | ||
| * Utility to wait for the marionette socket to be ready. | ||
| * | ||
| * @method waitFor |
|
I guess my hope was that you would make a very small, fake tcp server (which is not altogether too many lines of code in nodejs) and have it respond to new connections by waiting for a little bit and then writing back the string your code is looking for. You could use that to test the waitForSocket method and then, for the connect method, you could start trying to connect, then start your tcp server after a delay, and then make sure that the tcp server receives a connection and that connect resolves appropriately. |
| callback(); | ||
| }.bind(this), 0); | ||
| callback(); | ||
| }.bind(this), 0); |
There was a problem hiding this comment.
nit: process.nextTick is more idiomatic then setTimeout(fn, 0) in node (I know this was here before)
|
While I agree that writing unit tests for these (and if we look at the current test we don't really unit test the driver, just the client using said driver - which means that we need more than just this to be tested), it is a bit like holding up a wanted feature that actually have been tested on our automation..... I don't mind filing a bug for that and working on getting it addressed ASAP. |
|
@gaye I also spoke to Mike Lee. It happens that this bug is blocking several critical bug that are needed for performance testing. We clearly understand the need to write the test, and are really willing to get them written in the close future, but we really need to get forward in running test-perf automation to collect more reliable dataset that would allow us to determine if we meet acceptation criteria for performance. Also, @KevinGrandon would like to have this in, to really see if it will fix the intermittent failures in the Gaia integration tests. I have run the whole test suite with this change and they all passed. Unless this break other thing, since I have addressed the nit above in my tree, I see nothing that really holds back. If you have other concerns, please let us know, but getting a r+ is ultimately what we want to push it ASAP. Thank you kindly. |
I really think writing tests for your business logic shouldn't be that hard. You could even use a less "realistic" mock if you're in a hurry.
That doesn't prove that the logic here is correct. It only proves that you didn't break anything... |
|
Replying cause I was pinged here. I believe this would solve the last blocker that was preventing us turning Gij on TBPL so I support this landing :) Tests are good, but if they're hard to write let's figure that out or do some knowledge sharing so it's easier in the future. I'd be happy with any of the three options: 1 - Figure out how to write a test for this. |
|
The test run better now. BUT, they still fail, without this change. The error is So.... |
|
The actual error message: |
|
@gaye there is now a test. It pass. Still the comment about failures above stick. Mine just pass before these. Thank you kindly. |
| @@ -0,0 +1,32 @@ | |||
| var net = require('net'); | |||
| var debug = require('debug'); | |||
There was a problem hiding this comment.
debug is a function to which you're supposed to pass a topic for your logs... see https://github.com/visionmedia/debug
Hey @hub. Have you tried using a gaia profile to see if how that works out? |
|
@hfiguiere See https://github.com/mozilla-b2g/mozilla-profile-builder#usage for how to use gaia profile instead of the thing we download (which appears to crap out at the ftu). |
|
I haven't tried the profile builder yet, but I just updated the PR with (most of) the feedback. |
|
@hfiguiere I think I'm still missing some bits here? |
|
yes, the infamous refactor which will require a very thorough testing of the whole thing. More than just with this change. Working on it. |
|
Like I said on IRC r=me assuming we have tests passing... I gave hub permissions to push to the registry |
Bug 1033402 - waitForSocket() before connecting to marionette. r=jlal
No description provided.