<p>Patch set 1:<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/+/21217">View Change</a></p><p>30 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/+/21217/1//COMMIT_MSG">Commit Message:</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/+/21217/1//COMMIT_MSG@9">Patch Set #1, Line 9:</a> <code style="font-family:monospace,monospace">To expand the test capacities we would like to introduce</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm really lacking here some description on the setup to understand how the android phones are used, how are they hooked to OGT.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py">File src/osmo_gsm_tester/core/process.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/+/21217/1/src/osmo_gsm_tester/core/process.py@391">Patch Set #1, Line 391:</a> <code style="font-family:monospace,monospace">        self.remote_port = remote_port</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add the port param as a different patch prior to the android one (this patch).</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/+/21217/1/src/osmo_gsm_tester/core/process.py@400">Patch Set #1, Line 400:</a> <code style="font-family:monospace,monospace">                    self.remote_user = 'root'</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">All of this needs to get out from here. In the end is simply setting the remote_user to root which can be passed as an argument...</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/+/21217/1/src/osmo_gsm_tester/core/process.py@419">Patch Set #1, Line 419:</a> <code style="font-family:monospace,monospace">        if is_ue_cmd:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is not really UE/android specific, please introduce the port feature generically.</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/+/21217/1/src/osmo_gsm_tester/core/process.py@464">Patch Set #1, Line 464:</a> <code style="font-family:monospace,monospace">    run_dir = run_dir.new_dir(name)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is a separate patch.</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/+/21217/1/src/osmo_gsm_tester/core/process.py@476">Patch Set #1, Line 476:</a> <code style="font-family:monospace,monospace">    run_dir = run_dir.new_dir(name)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This too, in the same patch as line 464.</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/+/21217/1/src/osmo_gsm_tester/core/process.py@480">Patch Set #1, Line 480:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Different patch.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/remote.py">File src/osmo_gsm_tester/core/remote.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/+/21217/1/src/osmo_gsm_tester/core/remote.py@31">Patch Set #1, Line 31:</a> <code style="font-family:monospace,monospace">    def __init__(self, run_dir, remote_user = 'root', remote_host = 'localhost', remote_cwd=None, remote_port=None):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This all goes into the generic "add port" patch.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/enb_srs.py">File src/osmo_gsm_tester/obj/enb_srs.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/+/21217/1/src/osmo_gsm_tester/obj/enb_srs.py@117">Patch Set #1, Line 117:</a> <code style="font-family:monospace,monospace">        MainLoop.sleep(3)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What's this?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/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/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py@269">Patch Set #1, Line 269:</a> <code style="font-family:monospace,monospace">        if self._run_node.is_local() and ue.ue_serial:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This most probably needs to go into a subclass of RemoteProcess or Process or alike.</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/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py@291">Patch Set #1, Line 291:</a> <code style="font-family:monospace,monospace">                                          None, self._run_node.ssh_port())</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This goes into the "add port patch".</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@1">Patch Set #1, Line 1:</a> <code style="font-family:monospace,monospace"># Author: Nils Fürste <nils.fuerste@softwareradiosystems.com></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add a Copyright header with license.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@38">Patch Set #1, Line 38:</a> <code style="font-family:monospace,monospace">class AndroidUE(MS, srslte_common):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please split all the methods in this class according to their scope. I know it's not done perfectly in lots of places but let's do it here since there's lots of code. Use the following categories, see obj/bts.py as an example:</p><p style="white-space: pre-wrap; word-wrap: break-word;">##############<br># PROTECTED<br>##############</p><p style="white-space: pre-wrap; word-wrap: break-word;">########################<br># PUBLIC - INTERNAL API<br>########################</p><p style="white-space: pre-wrap; word-wrap: break-word;">###################<br># PUBLIC (test API included)<br>###################</p><p style="white-space: pre-wrap; word-wrap: break-word;">You can get an idea of what is what by looking at other ms classes and which methods are used by tests, or by other classes inside obj/ or core/.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@75">Patch Set #1, Line 75:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Drop space</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@80">Patch Set #1, Line 80:</a> <code style="font-family:monospace,monospace">        if self.enable_pcap:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use this boolean when calling this method, not here. Or at least use early return.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if not self.enable_pcap:<br>    return</pre></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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@85">Patch Set #1, Line 85:</a> <code style="font-family:monospace,monospace">                diag_parser_proc.launch()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">tis launch needs to go after the if/else. Actually.... you don't need to scp back if you are running locally... so you can drop the if and the else condition altogether..</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@97">Patch Set #1, Line 97:</a> <code style="font-family:monospace,monospace">                time.sleep(2)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Mainloop.sleep</p><p style="white-space: pre-wrap; word-wrap: break-word;">You have actually Mainloop.wait(), you should use it here instead of this selfmade timeout + sleep you use here</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@103">Patch Set #1, Line 103:</a> <code style="font-family:monospace,monospace">            if not self._run_node.is_local():</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Again, if you are scping the file it's because you are running remotely, so makes no sense to have this check here.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@116">Patch Set #1, Line 116:</a> <code style="font-family:monospace,monospace">        self.rx_monitor_proc.terminate()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You shouldn't need this if you make OGT "remember" to have it killed. You need that to make sure the process doesn't end up dangling when the test is done successfuly or fails.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@122">Patch Set #1, Line 122:</a> <code style="font-family:monospace,monospace">        if self.diag_monitor_proc:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It probably makes sense to have DiagMonitor as a different class either in this same file or a separate file. See for instance the pcap_recorder.py, or AbisIpFind, etc.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@134">Patch Set #1, Line 134:</a> <code style="font-family:monospace,monospace">        self.scp_back_pcap()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if not is local and pcap enabled, then self.scp_back_pcap()</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@152">Patch Set #1, Line 152:</a> <code style="font-family:monospace,monospace">        popen_args_clear_run_dir = ['sudo', 'rm', '-rf', '-v', str(self.remote_run_dir) + '/*', '||', 'true']</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please describe somewhere why is that needed but I'm not liking having a sudo rm -rf /* here ;)<br>Feel free to contact me and explain me your needs so we can work out the best way.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@167">Patch Set #1, Line 167:</a> <code style="font-family:monospace,monospace">        self.run_netns_wait("kill-adb",  ['sudo', 'adb', 'kill-server'])</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why is this run on a netns? I lack a description of the setup in the commit message.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@182">Patch Set #1, Line 182:</a> <code style="font-family:monospace,monospace">            emm_conn_chk_proc.launch()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">launch below if/else?</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@218">Patch Set #1, Line 218:</a> <code style="font-family:monospace,monospace">            self.rx_monitor_proc.launch()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">All these processes you launch should be monitored by OGT using self.testenv.remember_to_stop(self.process).</p><p style="white-space: pre-wrap; word-wrap: break-word;">And please, rather split some code into different classes if possible.</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/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@227">Patch Set #1, Line 227:</a> <code style="font-family:monospace,monospace">            DIAG_PARSER = 'osmo-gsm-tester_androidue_diag_parser.sh'</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">QcDiag class?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/run_node.py">File src/osmo_gsm_tester/obj/run_node.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/+/21217/1/src/osmo_gsm_tester/obj/run_node.py@33">Patch Set #1, Line 33:</a> <code style="font-family:monospace,monospace">    def __init__(self, type=None, run_addr=None, ssh_user=None, ssh_addr=None, run_label=None, remote_port=None):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">All of this goe sto the "add port" commit.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/default-suites.conf">File sysmocom/default-suites.conf:</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/+/21217/1/sysmocom/default-suites.conf@175">Patch Set #1, Line 175:</a> <code style="font-family:monospace,monospace">- 4g:androidue@mi5g+srsenb-rftype@uhd+mod-enb-nprb@25+mod-enb-txmode@1</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">That's definetly going to fail in our setup ;)<br>Better simply describe it as an example in the commit description.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/scenarios/androidue%40.conf">File sysmocom/scenarios/androidue@.conf:</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/+/21217/1/sysmocom/scenarios/androidue%40.conf@3">Patch Set #1, Line 3:</a> <code style="font-family:monospace,monospace">  - label: ${param1}</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">name of the scenario doesn't make much sense, since the "label" is generic.<br>Rather name it "ms-label@.conf" or similar.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/utils/bin/osmo-gsm-tester_androidue_conn_chk.sh">File utils/bin/osmo-gsm-tester_androidue_conn_chk.sh:</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/+/21217/1/utils/bin/osmo-gsm-tester_androidue_conn_chk.sh@2">Patch Set #1, Line 2:</a> <code style="font-family:monospace,monospace">while true; do</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this some monitoring tool? Please describe what is it used for here.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217">change 21217</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/+/21217"/><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: Iab37a0de59d6643d21bced34ddca06ff40fa62df </div>
<div style="display:none"> Gerrit-Change-Number: 21217 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: srs_andre <andre@softwareradiosystems.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Nils <nils.fuerste@softwareradiosystems.com> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 17 Nov 2020 17:59:57 +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>