Added Android RNDIS and gateway ping.

Added Android RNDIS and gateway ping. R=klm@google.com BUG=

[klm, 2014-04-22 19:40:13]

https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js File agent/js/src/adb.js (right): https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode364 agent/js/src/adb.js:364: * @return {webdriver.promise.Promise} Resolves to a dict of name to info, e.g.: nit: "dict" is a Python-ism, I'd just say "map". https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode375 agent/js/src/adb.js:375: var fields = line.split(/\s+/); FYI in the TradeFedHost we now trim() the line before splitting, so that empty first field (i.e. leading spaces) would fail the format check (# of fields) here, instead of resulting in an empty interface name and blowing up later on. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode381 agent/js/src/adb.js:381: entry.ifname = fields[0]; just "name"? https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode382 agent/js/src/adb.js:382: entry.is_up = (fields[1] == 'UP'); isUp https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode383 agent/js/src/adb.js:383: var ipaddr = fields[2].replace(/\/\d+$/, ''); "ipAddress" or simply "ip" -- like you named "mac". https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode405 agent/js/src/adb.js:405: var ifnames = Object.keys(netcfg).filter(function(ifname) { This iterates over the keys, but then indexes right back to the whole object. Do you ever actually need a *map* of interface descriptors returned from scheduleGetNetworkConfiguration(), as opposed to just an array? If it were an array, you could just iterate over values here. I see this pattern in all other places in this file that iterate over the interface descriptors. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode450 agent/js/src/adb.js:450: */ compute... https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode451 agent/js/src/adb.js:451: Adb.prototype.ComputeMacForRndis = function(ifname) { // jshint unused:false This method shouldn't be public. I'd even make it a plain function: function computeSyntheticMacForDeviceSerial(serial) {...} Since it's private, there is no need to future-proof its prototype -- i.e. no need for an unused interface name arg. Can always be added later if/when we decide it's necessary. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode465 agent/js/src/adb.js:465: Adb.prototype.scheduleIsRndisEnabled = function(ifname, mac) { private? https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode491 agent/js/src/adb.js:491: * @param {string=} ifname interface name, defaults to either 'rndis0' or Why have this arg? It seems it's never passed in reality, and it has lots of conditionals for it. I'd just rip all that out. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode511 agent/js/src/adb.js:511: return (!!ifname ? s == ifname : (/^(rndis|usb)0$/).test(s)); PTAL @ the logic in the TradeFedHost (and review comments). We might actually have both usb0 and rndis0, in which case we should use rndis0, but netcfg does not guarantee the order, so it could end up printing usb0 first, and then it seems this could would end up using usb0 instead of rndis0. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode520 agent/js/src/adb.js:520: var has_wifi = Object.keys(netcfg).some(function(s) { hasWifi https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode537 agent/js/src/adb.js:537: (!!mac ? mac : this.ComputeMacForRndis(ifname))]); See TradeFedHost -- Joon says this sometimes randomly fails, and he has refresh+retry logic. It's ok not to do this here, if we would ultimately fail the ping test and not ask for a job and then get here again on the next poll attempt -- an implicit retry essentially. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode542 agent/js/src/adb.js:542: throw new Error('Offline or no DHCP offer: ' + e.message); This apparently also sometimes randomly fails. Same comment about (implicit) retries. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode547 agent/js/src/adb.js:547: var dns1 = '8.8.8.8'; Can you instead get it from the properties that get it from actual DHCP, like TradeFedHost does? Private deployments of the test agent may want their internal DNS servers that know their Intranet hostnames -- similar to our corp. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode572 agent/js/src/adb.js:572: return this.shell(['ip', 'route', 'show']).then(function(stdout) { Does this work in ICS? https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... File agent/js/src/browser_android_chrome.js (right): https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... agent/js/src/browser_android_chrome.js:674: this.adb_.scheduleIsRndisEnabled().then(function(is_enabled) { isEnabled https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... agent/js/src/browser_android_chrome.js:676: if (!shouldAttemptRecovery) { Why guard this with a flag -- why not just unconditionally do it whenever useRndis is true? Actually, it seems in reality shouldAttemptRecovery is always true, so I'd just remove the arg, avoid all the unused warnings in the other modules, and make this code unconditional. https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... agent/js/src/browser_android_chrome.js:676: if (!shouldAttemptRecovery) { I'm not sure I understand how the recovery logic works overall... This little block just re-enables RNDIS if it's disabled, but doesn't recover from a nonworking ping -- i.e. an enabled but wedged RNDIS.

[wrightt, 2014-05-16 16:47:10]

Run retry code will be in a separate review. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js File agent/js/src/adb.js (right): https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode364 agent/js/src/adb.js:364: * @return {webdriver.promise.Promise} Resolves to a dict of name to info, e.g.: On 2014/04/22 19:40:13, klm wrote: > nit: "dict" is a Python-ism, I'd just say "map". Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode375 agent/js/src/adb.js:375: var fields = line.split(/\s+/); On 2014/04/22 19:40:13, klm wrote: > FYI in the TradeFedHost we now trim() the line before splitting, so that empty > first field (i.e. leading spaces) would fail the format check (# of fields) > here, instead of resulting in an empty interface name and blowing up later on. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode381 agent/js/src/adb.js:381: entry.ifname = fields[0]; On 2014/04/22 19:40:13, klm wrote: > just "name"? Done; renamed to "ifc.name" https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode382 agent/js/src/adb.js:382: entry.is_up = (fields[1] == 'UP'); On 2014/04/22 19:40:13, klm wrote: > isUp Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode383 agent/js/src/adb.js:383: var ipaddr = fields[2].replace(/\/\d+$/, ''); On 2014/04/22 19:40:13, klm wrote: > "ipAddress" or simply "ip" -- like you named "mac". Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode405 agent/js/src/adb.js:405: var ifnames = Object.keys(netcfg).filter(function(ifname) { On 2014/04/22 19:40:13, klm wrote: > This iterates over the keys, but then indexes right back to the whole object. Do > you ever actually need a *map* of interface descriptors returned from > scheduleGetNetworkConfiguration(), as opposed to just an array? If it were an > array, you could just iterate over values here. I see this pattern in all other > places in this file that iterate over the interface descriptors. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode450 agent/js/src/adb.js:450: */ On 2014/04/22 19:40:13, klm wrote: > compute... Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode451 agent/js/src/adb.js:451: Adb.prototype.ComputeMacForRndis = function(ifname) { // jshint unused:false On 2014/04/22 19:40:13, klm wrote: > This method shouldn't be public. I'd even make it a plain function: > > function computeSyntheticMacForDeviceSerial(serial) {...} > > Since it's private, there is no need to future-proof its prototype -- i.e. no > need for an unused interface name arg. Can always be added later if/when we > decide it's necessary. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode465 agent/js/src/adb.js:465: Adb.prototype.scheduleIsRndisEnabled = function(ifname, mac) { On 2014/04/22 19:40:13, klm wrote: > private? Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode491 agent/js/src/adb.js:491: * @param {string=} ifname interface name, defaults to either 'rndis0' or On 2014/04/22 19:40:13, klm wrote: > Why have this arg? It seems it's never passed in reality, and it has lots of > conditionals for it. I'd just rip all that out. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode511 agent/js/src/adb.js:511: return (!!ifname ? s == ifname : (/^(rndis|usb)0$/).test(s)); On 2014/04/22 19:40:13, klm wrote: > PTAL @ the logic in the TradeFedHost (and review comments). We might actually > have both usb0 and rndis0, in which case we should use rndis0, but netcfg does > not guarantee the order, so it could end up printing usb0 first, and then it > seems this could would end up using usb0 instead of rndis0. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode520 agent/js/src/adb.js:520: var has_wifi = Object.keys(netcfg).some(function(s) { On 2014/04/22 19:40:13, klm wrote: > hasWifi Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode537 agent/js/src/adb.js:537: (!!mac ? mac : this.ComputeMacForRndis(ifname))]); On 2014/04/22 19:40:13, klm wrote: > See TradeFedHost -- Joon says this sometimes randomly fails, and he has > refresh+retry logic. It's ok not to do this here, if we would ultimately fail > the ping test and not ask for a job and then get here again on the next poll > attempt -- an implicit retry essentially. Done; added a tail "assert rndis is available". https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode542 agent/js/src/adb.js:542: throw new Error('Offline or no DHCP offer: ' + e.message); On 2014/04/22 19:40:13, klm wrote: > This apparently also sometimes randomly fails. Same comment about (implicit) > retries. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode547 agent/js/src/adb.js:547: var dns1 = '8.8.8.8'; On 2014/04/22 19:40:13, klm wrote: > Can you instead get it from the properties that get it from actual DHCP, like > TradeFedHost does? > > Private deployments of the test agent may want their internal DNS servers that > know their Intranet hostnames -- similar to our corp. Done. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode572 agent/js/src/adb.js:572: return this.shell(['ip', 'route', 'show']).then(function(stdout) { On 2014/04/22 19:40:13, klm wrote: > Does this work in ICS? Done; modified to use `cat /proc/net/route`. https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... File agent/js/src/browser_android_chrome.js (right): https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... agent/js/src/browser_android_chrome.js:674: this.adb_.scheduleIsRndisEnabled().then(function(is_enabled) { On 2014/04/22 19:40:13, klm wrote: > isEnabled Done; changed to assert. https://codereview.appspot.com/90340044/diff/1/agent/js/src/browser_android_c... agent/js/src/browser_android_chrome.js:676: if (!shouldAttemptRecovery) { Done; created separate functions for checking if online vs recovery. New recovery pseudocode: try { assert is ready return false // we were already online } catch (e) { enable rndis // throws error if "adb: device is offline" assert is ready // throws error if can't ping return true // caller should retry prev run (b/c was offline) }

[klm, 2014-05-16 17:23:24]

lgtm Only minor things left. https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js File agent/js/src/adb.js (right): https://codereview.appspot.com/90340044/diff/1/agent/js/src/adb.js#newcode383 agent/js/src/adb.js:383: var ipaddr = fields[2].replace(/\/\d+$/, ''); BTW just realized there might be IPv6 -- Android is certainly IPv6 capable, and it's just a coincidence that we only do v4 in our lab. That's just to keep in mind for the future, not necessarily for this CL. https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js File agent/js/src/adb.js (right): https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:410: }); Neat! https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:435: throw new Error('Device temperature not available'); Wouldn't this make the agent completely incompatible with devices that don't provide temp or have a different way of providing temp? Maybe just log it at error level but don't actually fail? https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:507: var badNames = netcfg.filter(function(o) { o https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:511: badNames.length !== 0 ? 'Non-rndis ' + badNames.join(' ') + ' is UP' : I'd rather see more verbose and more repetitive, but more readable logic for constructing this message. Maybe have a single loop through the interfaces (instead of filter->map) and build up a list of per-interface messages based on individual "bad" conditions, and then if the list is not empty, throw new Error('; '.join(errors)). Although there is no explicit provision for conditional expressions in the JS style guide, I agree with the Python style guide that it's ok for one liners, but can be hard to read if the expression is long -- which it most definitely is here. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Conditional_Ex... https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:549: var hasWifi = netcfg.some(function(ifc2) { I'd rather rename ifc above to rndisIfc or ifcRndis, and then there won't have to be an artificial "2" here. In general, whenever I find myself wanting to add to "2" to a name, I try to do more specifically descriptive names instead. https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:553: this.su(['wpa_cli', 'terminate']); Does wpa_cli still work in general? Please ask Joon -- at least bringing WiFi *up* purportedly no longer works in JB+, and TF uses an app that calls proper platform APIs to bring WiFi up (and down?). https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:568: // Enable DHCP -- this will timeout after 10s. // There is an implicit retry, see "Sanity check" comment below. https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:577: this.su(['ndc', 'resolver', 'setifdns', ifname].concat(ips)); Is that how TF sets DNS for the RNDIS interface? Please check with Joon. https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:612: // Decode '4E38220C' to '12.34.56.78' -- there's likely a better way: This looks decent -- reads well, and IDK if there is a more compact way to write it. Ultimately it's a 32-bit number broken into 4 base-256 digits, and each digit is then written as a base-10 number. https://codereview.appspot.com/90340044/diff/20001/agent/js/src/adb.js#newcod... agent/js/src/adb.js:612: // Decode '4E38220C' to '12.34.56.78' -- there's likely a better way: Again not for this CL but for the future -- IPv6 support. Doing proper IPv6 support in the agent could be a more sweeping cleanup than just a couple of places here.

[wrightt, 2014-12-02 17:59:27]

I addressed most of the above issues and pushed: https://github.com/WPO-Foundation/webpagetest/commit/477488fae4d6d7d65d7f064a...