<p style="white-space: pre-wrap; word-wrap: break-word;">This is starting to look good, there's only a few things here and there which needs polishing.</p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py">File src/osmo_gsm_tester/obj/android_apn.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@30">Patch Set #3, Line 30:</a> <code style="font-family:monospace,monospace"> schema.register_resource_schema('apn', resource_schema)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is probably not correct, as this should rpobably be under "modem" object or alike. So modem will probably end up adding this by using AndroidApn.schema() on its on_register_schemas()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@71">Patch Set #3, Line 71:</a> <code style="font-family:monospace,monospace"> def from_conf(cls, conf):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This one is PUBLIC INTERNAL API</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@75">Patch Set #3, Line 75:</a> <code style="font-family:monospace,monospace"> @classmethod</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This one is PUBLIC INTERNAL API</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@87">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace"> proc = self.run_androidue_cmd("set-apn", [qry_carrier_cmd])</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"get-carrier-id" instead of "set-apn"? or is set-apn the binary? Not sure now if this is a description string or the binary name.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py">File src/osmo_gsm_tester/obj/android_host.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@31">Patch Set #3, Line 31:</a> <code style="font-family:monospace,monospace"> def __init__(self, name, testenv, conf):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what's the point in passing these params to constructor if you don't use them? You can remove them</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@42">Patch Set #3, Line 42:</a> <code style="font-family:monospace,monospace"> log.dbg("Deleted the following files: %s" % clear_run_dir_proc.get_stdout())</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You should better inherit from log.Origin in AndroidHost and use self.log. See how other classes do it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@50">Patch Set #3, Line 50:</a> <code style="font-family:monospace,monospace"> self.dbg("Deleted the following files: %s" % clear_diag_logs_proc.get_stdout())</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see, you already expect to be inheriting from log.Origin, but you are not ;)<br>This probably works because subclasses are inheriting from log.Origin. You should be inheriting here instead, and call this constructor from child classes instead.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@72">Patch Set #3, Line 72:</a> <code style="font-family:monospace,monospace"> if self._run_node.is_local():</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would welcome some description here too, specially what's the main difference with the method above and the rationale for having both.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/iperf3.py">File src/osmo_gsm_tester/obj/iperf3.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/iperf3.py@297">Patch Set #3, Line 297:</a> <code style="font-family:monospace,monospace"> elif self._run_node.ssh_port():</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So the main difference between this branch and the one below is that you need to force "root" user?<br>I'm not liking checking here based on whether ssh port is set or not. We should flag this somehow in the class implementing when passing its run_node by means of this class set_run_node().</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/iperf3.py@315">Patch Set #3, Line 315:</a> <code style="font-family:monospace,monospace"> self.process = process.Process(self.name(), self.run_dir, popen_args, env={})</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add a new AdbProcess sublcass similar to NetNSProcess to hide this adb command stuff.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/ms_android.py">File src/osmo_gsm_tester/obj/ms_android.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/ms_android.py@43">Patch Set #3, Line 43:</a> <code style="font-family:monospace,monospace"> resource_schema['apn.%s' % key] = val</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what I wrote in the first file of this patch: This is done here, so no need to have it in register_schemas() on the AndroidApn file.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/ms_android.py@257">Patch Set #3, Line 257:</a> <code style="font-family:monospace,monospace"> ip_prefix = "172.16.0"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This API is probably PROTECTED?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/qc_diag.py">File src/osmo_gsm_tester/obj/qc_diag.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/qc_diag.py@81">Patch Set #3, Line 81:</a> <code style="font-family:monospace,monospace"> while timer > 0:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think in general you should be using event_loop.py's wait() method for this kind of sleeping loops.<br>See for instance:<br>"""<br>MainLoop.wait(self.dbus.has_interface, *req_ifaces, timeout=10)<br>"""</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302">change 21302</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-gsm-tester </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I79a5d803e869a868d4dac5e0d4c2feb38038dc5c </div>
<div style="display:none"> Gerrit-Change-Number: 21302 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: ninjab3s <nils.fuerste@softwareradiosystems.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 30 Nov 2020 17:08:58 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>