<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">you could set its level to DEBUG</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Good point. I'll update it in next revision.</p><p><a href="https://gerrit.osmocom.org/12556">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/Makefile.am">File tests/trans/Makefile.am:</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/12556/9/tests/trans/Makefile.am@4">Patch Set #9, Line 4:</a> <code style="font-family:monospace,monospace">-I$(top_srcdir)/src/libmsc</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looks useless to me. The only header in there is smpp_smsc. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Agreed. Feel free to add me as reviewer if you make such a patch.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c">File tests/trans/trans_test.c:</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/12556/9/tests/trans/trans_test.c@51">Patch Set #9, Line 51:</a> <code style="font-family:monospace,monospace">trans_assign_trans_id</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You're always calling it with ti_flag = 0. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@68">Patch Set #9, Line 68:</a> <code style="font-family:monospace,monospace">base_callref</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What is the point of passing callref? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What's the point of using static counter? What would be the advantage compared to explicitly passing the argument?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@79">Patch Set #9, Line 79:</a> <code style="font-family:monospace,monospace">return</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would rather use OSMO_ASSERT here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't. I'd rather see as many errors as possible in a single run.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@88">Patch Set #9, Line 88:</a> <code style="font-family:monospace,monospace">return</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Let's just imagine that trans_has_conn() returns X != 0. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@94">Patch Set #9, Line 94:</a> <code style="font-family:monospace,monospace">vlr_subscr_name</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You say 'connection', but print subscriber name? o_O</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@134">Patch Set #9, Line 134:</a> <code style="font-family:monospace,monospace">base_callref</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Hmm, allocating multiple transactions with same callref? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It does: we print amount of transactions allocated explicitly. Changing allocation logic would be immediately reflected in the test output.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.err">File tests/trans/trans_test.err:</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/12556/9/tests/trans/trans_test.err@1">Patch Set #9, Line 1:</a> <code style="font-family:monospace,monospace">Unknown RAN type</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Cosmetic: you could use any 'known' RAN type to avoid this.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What for? We're testing transaction.h functions in here so I'd rather avoid the need to deal with TX/RX.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12556">change 12556</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/12556"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713 </div>
<div style="display:none"> Gerrit-Change-Number: 12556 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 15 Jan 2019 09:45:02 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>