Bug 1033402 - Wait for adb port to be available before starting #1
Conversation
|
|
||
| Hubert Figuiere <hub@mozilla.com> | ||
| Sahaja James Lal <jlal@mozilla.com> | ||
| Eli Perelman <eli@mozilla.com> |
|
Commit message needs a bug number. Also you totally ignore the changes in https://github.com/hfiguiere/marionette-device-host/compare/bug1027232 (yes this is taken straight from the bug comments) And unecessary rewrapping obscure the actual change. Please split these commits. I'll take over Monday anyway, pushing the other changes, etc. |
|
I didn't realise there was a separate bug open from bug 1027232. Even more important to not needlessly reindent as this will make it harder to rebase 1027232. |
| spawn = require('child_process').spawn, | ||
| debug = require('debug')('marionette-device-host'); | ||
| var tcpPort = require('tcp-port-used'); | ||
| var _ = require('lodash'); |
There was a problem hiding this comment.
seriously we need even more bloat?
There was a problem hiding this comment.
@hfiguiere Hey calm down! Eli is very nicely trying to help you out here!
There was a problem hiding this comment.
More seriously I don't find any justification of this for the actual bug.
Also '_' as a symbol?
Not that I'm back from PTO I'll return to actually fixing the issue.
There was a problem hiding this comment.
The justification was simplification and reuse of the code that was generating the options object. We technically need to have it done twice, and with a handy utility method from lodash, it was much more simplified.
In every codebase that uses underscore or lodash, the conventional symbol is the _, just like jQuery is synonymous with $.
|
eliperelman
changed the title
There shouldn't be any conflicts since 1027232 is only the gaia change needed to support this change, and this patch doesn't touch any gaia. Once this lands, then the gaia patch from 1027232 can land. |
|
I was talking about this change https://github.com/hfiguiere/marionette-device-host/compare/bug1027232 It is NOT in gaia either. |
|
We don't need a first run flag. It just work. The only thing missing from my change above is effectively waiting on the connection to be up which is bug 1033402 but should actually be integral part of bug 1027232 because otherwise it doesn't work (short of using the timeout, which I do not like) |
|
I've tried running it without the flag, and while B2G does restart, it restarts in between EACH test. Not just between apps, but between every test. That's because for some reason, marionette-device-host is instantiated between every test, causing B2G to restart every time. We need the flag for this. With the patch I have proposed here, we don't need the |
|
The setTimeout is stupid. It was a quick and dirty hack I wanted to fix in the next iteration with the goal of get things rolled in before I went on PTO. Restarting before each test (but not each run) isn't a bad idea IMHO. |
|
@hfiguiere yeah, the way it's currently set up is that it restarts before every test, every run. Luckily node modules are cached so we can fence that problem with a flag in this module. |
|
Definitely not every run. |
|
Agreed, it was making my tests insanely long. 😄 |
|
The functional tests restart every test on every run. It's the only way to get a clean profile and clean slate to run the tests. Admittedly the perf tests are different (and longer) but this pull could benefit the functional tests if you gave it the flexibility to do both. |
|
Testing on Hamachi/Buri, I need to change sleep 2 to sleep 5 - the same delay I had in my shell script hack. This mean that the "wait for adb port to be available" doesn't work. |
|
@hfiguiere In some fashion this needs to happen repeatably and not based on timing / randomness. If knowing that the marionette port is up is not enough to begin interacting with b2g, then let's think of additional data points. |
|
That's the gist of my comment. I don't like the timer thing and I was just demonstrating how bad it was by just testing on a device Eli didn't test on (he doesn't have one). |
|
Note: I was trying to figure a proper solution yesterday. |
No description provided.