<p>Patch set 5:<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-ttcn3-hacks/+/18370">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn">File pcu/PCU_Tests.ttcn:</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-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@a811">Patch Set #5, Line 811:</a> <code style="font-family:monospace,monospace">bsn := 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are you sure?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@259">Patch Set #5, Line 259:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">f_init_gprs_ms();<br> ms := g_ms[0]; /* We only use first MS in this test */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am still not happy about this approach. There is no real need to keep the MS state in component we're running on. All functions are supposed to accept a 'pointer' to ms anyway. If a test case needs more than one MS... it just allocates an array of GprsMS, locally.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@269">Patch Set #5, Line 269:</a> <code style="font-family:monospace,monospace">f_ms_establish_ul_tbf</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Uhhhh, such an overhead... This is what I was afraid of, the API is loosing simplicity and flexibility. We don't really want a TBF here, we don't want +100500 more checks, we don't even need an MS here - we just need to send RACH.ind and check Timing Advance in the response. Everything else is behind the scope of this test case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The new API is probably fine for complex test cases, but here it just complicates things.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@310">Patch Set #5, Line 310:</a> <code style="font-family:monospace,monospace">f_ms_rx_imm_ass_ccch</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same problem here, what would be the failure reason? Right, "failed to match Immediate Assignment". In this case, again, you're adding more (less than in previous test case, but still) complexity to a simple test case...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@584">Patch Set #5, Line 584:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (match(dl_block, tr_RLCMAC_DUMMY_CTRL())) {<br> continue;<br> }<br> if (not match(dl_block, tr_RLCMAC_UL_ACK_NACK_GPRS(ul_tfi := ?)) and<br> not match(dl_block, tr_RLCMAC_UL_ACK_NACK_EGPRS(ul_tfi := ?))) {<br> setverdict(fail, "Failed to match Packet Uplink ACK / NACK:", dl_block);<br> f_shutdown(__BFILE__, __LINE__);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This goes behind the scope of the test case.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@609">Patch Set #5, Line 609:</a> <code style="font-family:monospace,monospace">f_shutdown</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unrelated change. AFAIR, I intentionally did not break 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-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@685">Patch Set #5, Line 685:</a> <code style="font-family:monospace,monospace">f_shutdown</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Again unrelated. Please submit a separate patch, describe why it's needed and which problem it solves.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@700">Patch Set #5, Line 700:</a> <code style="font-family:monospace,monospace">ms := g_ms[0]; /* We only use first MS in this test */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is copy-pasted, again and again...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@730">Patch Set #5, Line 730:</a> <code style="font-family:monospace,monospace">f_shutdown</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@747">Patch Set #5, Line 747:</a> <code style="font-family:monospace,monospace">ms := g_ms[0]; /* We only use first MS in this test */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">While it could be just one line: var GprsMS ms := valueof(t_GprsMS_def) or so.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@835">Patch Set #5, Line 835:</a> <code style="font-family:monospace,monospace">TC_countdown_procedure</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">For this test case, the new API really does the magic. The impact is noticeable.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@1511">Patch Set #5, Line 1511:</a> <code style="font-family:monospace,monospace">f_TC_egprs_pkt_chan_req</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here, we don't really need MS/TBF/... abstraction here.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370">change 18370</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-ttcn3-hacks/+/18370"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-ttcn3-hacks </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ib3fee37580f0ea0530a659dec83656799bf57288 </div>
<div style="display:none"> Gerrit-Change-Number: 18370 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 21 May 2020 18:50:08 +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>