Change in osmo-ttcn3-hacks[master]: pcu: Refactor GPRS infrastructure to keep state and simplify tests

[fixeria]

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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 21 May 2020 18:50:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200521/0a3db995/attachment.htm>