fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370 )
Change subject: pcu: Refactor GPRS infrastructure to keep state and simplify tests ...................................................................... Patch Set 5: Code-Review-1 (12 comments) https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn File pcu/PCU_Tests.ttcn: https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@a811 PS5, Line 811: bsn := 0; Are you sure? https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@259 PS5, Line 259: f_init_gprs_ms(); : ms := g_ms[0]; /* We only use first MS in this test */ 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. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@269 PS5, Line 269: f_ms_establish_ul_tbf 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. The new API is probably fine for complex test cases, but here it just complicates things. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@310 PS5, Line 310: f_ms_rx_imm_ass_ccch 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... https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@584 PS5, Line 584: if (match(dl_block, tr_RLCMAC_DUMMY_CTRL())) { : continue; : } : if (not match(dl_block, tr_RLCMAC_UL_ACK_NACK_GPRS(ul_tfi := ?)) and : not match(dl_block, tr_RLCMAC_UL_ACK_NACK_EGPRS(ul_tfi := ?))) { : setverdict(fail, "Failed to match Packet Uplink ACK / NACK:", dl_block); : f_shutdown(__BFILE__, __LINE__); : } This goes behind the scope of the test case. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@609 PS5, Line 609: f_shutdown Unrelated change. AFAIR, I intentionally did not break here. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@685 PS5, Line 685: f_shutdown Again unrelated. Please submit a separate patch, describe why it's needed and which problem it solves. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@700 PS5, Line 700: ms := g_ms[0]; /* We only use first MS in this test */ This is copy-pasted, again and again... https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@730 PS5, Line 730: f_shutdown Same. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@747 PS5, Line 747: ms := g_ms[0]; /* We only use first MS in this test */ While it could be just one line: var GprsMS ms := valueof(t_GprsMS_def) or so. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@835 PS5, Line 835: TC_countdown_procedure For this test case, the new API really does the magic. The impact is noticeable. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@1511 PS5, Line 1511: f_TC_egprs_pkt_chan_req Same here, we don't really need MS/TBF/... abstraction here. -- To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ttcn3-hacks Gerrit-Branch: master Gerrit-Change-Id: Ib3fee37580f0ea0530a659dec83656799bf57288 Gerrit-Change-Number: 18370 Gerrit-PatchSet: 5 Gerrit-Owner: pespin <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Thu, 21 May 2020 18:50:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
