Max has posted comments on this change. ( https://gerrit.osmocom.org/12556 )
Change subject: Add tests for transaction routines ...................................................................... Patch Set 9: (8 comments) > you could set its level to DEBUG Good point. I'll update it in next revision. https://gerrit.osmocom.org/#/c/12556/9/tests/trans/Makefile.am File tests/trans/Makefile.am: https://gerrit.osmocom.org/#/c/12556/9/tests/trans/Makefile.am@4 PS9, Line 4: -I$(top_srcdir)/src/libmsc > Looks useless to me. The only header in there is smpp_smsc. […] Agreed. Feel free to add me as reviewer if you make such a patch. https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c File tests/trans/trans_test.c: https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@51 PS9, Line 51: trans_assign_trans_id > You're always calling it with ti_flag = 0. […] I'm not sure how to test this because we're always using 0. Anyway, the tests could (and will) be expanded in follow-up patches. https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@68 PS9, Line 68: base_callref > What is the point of passing callref? […] What's the point of using static counter? What would be the advantage compared to explicitly passing the argument? https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@79 PS9, Line 79: return > I would rather use OSMO_ASSERT here. I don't. I'd rather see as many errors as possible in a single run. https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@88 PS9, Line 88: return > Let's just imagine that trans_has_conn() returns X != 0. […] trans_has_conn() returns pointer to a struct. I don't see how using assert on it would be better than returning from the test. https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@94 PS9, Line 94: vlr_subscr_name > You say 'connection', but print subscriber name? o_O I don't think we have function to print ran-conn itself and there's 1:1 mapping between VLR subscribers and connections. Having said that, I'm open for improvements. What do you suggest? https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@134 PS9, Line 134: base_callref > Hmm, allocating multiple transactions with same callref? […] It does: we print amount of transactions allocated explicitly. Changing allocation logic would be immediately reflected in the test output. https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.err File tests/trans/trans_test.err: https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.err@1 PS9, Line 1: Unknown RAN type > Cosmetic: you could use any 'known' RAN type to avoid this. What for? We're testing transaction.h functions in here so I'd rather avoid the need to deal with TX/RX. -- To view, visit https://gerrit.osmocom.org/12556 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713 Gerrit-Change-Number: 12556 Gerrit-PatchSet: 9 Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Comment-Date: Tue, 15 Jan 2019 09:45:02 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
