Change in osmo-msc[master]: Add tests for transaction routines

[Max]

Max has uploaded this change for review. ( https://gerrit.osmocom.org/12556


Change subject: Add tests for transaction routines
..

Add tests for transaction routines

Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713
---
M configure.ac
M tests/Makefile.am
M tests/testsuite.at
A tests/trans/Makefile.am
A tests/trans/trans_test.c
A tests/trans/trans_test.err
A tests/trans/trans_test.ok
7 files changed, 242 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/56/12556/1

diff --git a/configure.ac b/configure.ac
index a4979cf..d955187 100644
--- a/configure.ac
+++ b/configure.ac
@@ -257,6 +257,7 @@
 tests/Makefile
 tests/atlocal
 tests/smpp/Makefile
+tests/trans/Makefile
 tests/sms_queue/Makefile
 tests/msc_vlr/Makefile
 doc/Makefile
diff --git a/tests/Makefile.am b/tests/Makefile.am
index dc5194c..883df23 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,6 +1,7 @@
 SUBDIRS = \
sms_queue \
msc_vlr \
+   trans \
$(NULL)

 if BUILD_SMPP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index f27b60c..f3cb740 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -9,6 +9,13 @@
 AT_CHECK([$abs_top_builddir/tests/smpp/smpp_test], [], [expout], [experr])
 AT_CLEANUP

+AT_SETUP([trans])
+AT_KEYWORDS([trans])
+cat $abs_srcdir/trans/trans_test.ok > expout
+cat $abs_srcdir/trans/trans_test.err > experr
+AT_CHECK([$abs_top_builddir/tests/trans/trans_test], [], [expout], [experr])
+AT_CLEANUP
+
 AT_SETUP([sms_queue_test])
 AT_KEYWORDS([sms_queue_test])
 cat $abs_srcdir/sms_queue/sms_queue_test.ok > expout
diff --git a/tests/trans/Makefile.am b/tests/trans/Makefile.am
new file mode 100644
index 000..1a05307
--- /dev/null
+++ b/tests/trans/Makefile.am
@@ -0,0 +1,31 @@
+AM_CPPFLAGS = \
+   $(all_includes) \
+   -I$(top_srcdir)/include \
+   -I$(top_srcdir)/src/libmsc \
+   $(NULL)
+
+AM_CFLAGS = \
+   -Wall \
+   -ggdb3 \
+   $(LIBOSMOCORE_CFLAGS) \
+   $(LIBOSMOGSM_CFLAGS) \
+   $(LIBOSMOSCCP_CFLAGS) \
+   $(LIBOSMOABIS_CFLAGS) \
+   $(COVERAGE_CFLAGS) \
+   $(NULL)
+
+AM_LDFLAGS = $(COVERAGE_LDFLAGS)
+
+EXTRA_DIST = trans_test.ok trans_test.err
+
+trans_test_LDADD = \
+   $(top_builddir)/src/libmsc/libmsc.a \
+   $(top_builddir)/src/libvlr/libvlr.a \
+   $(LIBOSMOCORE_LIBS) \
+   $(LIBOSMOGSM_LIBS) \
+   $(LIBOSMOABIS_LIBS) \
+   $(LIBOSMOSIGTRAN_LIBS) \
+   $(LIBOSMOMGCPCLIENT_LIBS) \
+   $(LIBOSMOGSUPCLIENT_LIBS) \
+   -ldbi \
+   $(NULL)
diff --git a/tests/trans/trans_test.c b/tests/trans/trans_test.c
new file mode 100644
index 000..42b39c6
--- /dev/null
+++ b/tests/trans/trans_test.c
@@ -0,0 +1,184 @@
+/*
+ * (C) 2019 sysmocom s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+
+static bool wrong_tran(const struct gsm_trans *t, uint8_t ref_id)
+{
+   if (!t) {
+   printf("Failed to obtain transaction with expected ID %u.\n", 
ref_id);
+   return true;
+   }
+
+   if (t->transaction_id != ref_id) {
+   printf("Wrong transaction obtained with ID %u != %u 
(expected)\n", t->transaction_id, ref_id);
+   return true;
+   }
+
+   return false;
+}
+
+static struct gsm_trans *make_tran(struct gsm_network *net, struct vlr_subscr 
*vsub, uint8_t proto, uint32_t callref)
+{
+   int trans_id = trans_assign_trans_id(net, vsub, proto, 0);
+   if (trans_id < 0) {
+   printf("Failed to get next transaction ID.\n");
+   return NULL;
+   }
+
+   return trans_alloc(net, vsub, proto, trans_id, callref);
+}
+
+static struct ran_conn *make_conn(struct gsm_network *net, struct vlr_subscr 
*vsub)
+{
+   struct ran_conn *ran_c = ran_conn_alloc(net, OSMO_RAT_UNKNOWN, 69);
+   if (ran_c)
+   ran_c->vsub = vlr_subscr_get(vsub);
+   return ran_c;
+}
+
+static void test_tran_single(struct gsm_network *net, struct vlr_subscr *vsub, 
uint32_t base_callref)
+{
+   struct gsm_trans *t, *x;
+   struct ran_conn *ran_c;
+   uint8_t p = GSM48_PDISC_GROUP_CC;
+

Change in osmo-msc[master]: Add tests for transaction routines

[Harald Welte]

Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 1: Code-Review+2


--
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: 1
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Comment-Date: Mon, 14 Jan 2019 14:34:39 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes

Change in osmo-msc[master]: Add tests for transaction routines

[Max]

Max has posted comments on this change. ( https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 9:

This change is ready for review.


--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Comment-Date: Mon, 14 Jan 2019 17:16:12 +
Gerrit-HasComments: No
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Harald Welte]

Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 9: Code-Review+2


--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Comment-Date: Mon, 14 Jan 2019 21:54:07 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes

Change in osmo-msc[master]: Add tests for transaction routines

[Vadim Yanitskiy]

Vadim Yanitskiy has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 9: Code-Review-1

(13 comments)

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.h (which should be 
also moved to 'include' in some separate 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@34
PS9, Line 34: ref_id
This name looks a bit confusing. What is the meaning of 'ref_id'?
Reference ID? What is the reference then?

As far as I can see, you're always passing transaction ID, so let's name it 
'trans_id' then?


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 would really like this test case 
to cover both '0'B and '1'B cases.


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?
Why not to use a static counter?


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@79
PS9, Line 79: return
I would rather use OSMO_ASSERT here.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@83
PS9, Line 83: return
... and here.


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. This test would just 
fail, and one would have to investigate *why*, most likely by adding debug 
printf()s or asserts.

This is why I also recommend to use OSMO_ASSERT here too.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@92
PS9, Line 92: return
... and here.


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


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's a known issue (at least to me), that trans_alloc() would happily allocate 
a transaction with duplicate callref. And I expected that this test case would 
disclose this problem...


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@151
PS9, Line 151: \n\tCleared   %u transactions.
Let's split up this message into 2 printf() calls.


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.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.err@2
PS9, Line 2:
Also, I expected to see debug messages here, like:

  (ti %02x sub %s callref %x) New transaction

transaction.c (for some reason) is using DCC, so
you could set its level to DEBUG.



--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Mon, 14 Jan 2019 23:42:41 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes

Change in osmo-msc[master]: Add tests for transaction routines

[Max]

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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 09:45:02 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Max]

Hello Vadim Yanitskiy, Harald Welte, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/12556

to look at the new patch set (#10).

Change subject: Add tests for transaction routines
..

Add tests for transaction routines

Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713
---
M configure.ac
M tests/Makefile.am
M tests/testsuite.at
A tests/trans/Makefile.am
A tests/trans/trans_test.c
A tests/trans/trans_test.err
A tests/trans/trans_test.ok
7 files changed, 272 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/56/12556/10
--
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: newpatchset
Gerrit-Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713
Gerrit-Change-Number: 12556
Gerrit-PatchSet: 10
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 

Change in osmo-msc[master]: Add tests for transaction routines

[Max]

Max has posted comments on this change. ( https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 10:

(1 comment)

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@134
PS9, Line 134: base_callref
> It's a known issue (at least to me)

Btw, do we have a ticket for this?



--
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: 10
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 10:01:40 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Vadim Yanitskiy]

Vadim Yanitskiy has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 9:

(4 comments)

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@79
PS9, Line 79: return
> I don't. I'd rather see as many errors as possible in a single run.
It doesn't make sense to continue execution of the test case if at least one 
step fails IMHO. Moreover, ran_conn_alloc() may return NULL without any debug 
messages.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@83
PS9, Line 83: return
> ... and here.
trans_alloc() also may return NULL without any debug output.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@88
PS9, Line 88: return
> trans_has_conn() returns pointer to a struct. […]
It would explicitly indicate why *and where* the test case has failed. The 
current code would silently skip the pending code and start testing 
test_tran_overflow().


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@134
PS9, Line 134: base_callref
Sorry, but I am not getting this. The key idea is that allocating multiple 
transactions with same callref is wrong, and we shouldn't do this at least in 
tests. Vice versa, this test should ensure that trans_alloc() prevents this.

> Changing allocation logic

You don't need to change the logic, just do:

  callref = base_callref + i



--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 10:02:14 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Vadim Yanitskiy]

Vadim Yanitskiy has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 9:

> (1 comment)

Yup, please see: https://osmocom.org/issues/3294


--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 10:03:08 +
Gerrit-HasComments: No
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Max]

Max has posted comments on this change. ( https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 10:

(1 comment)

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@134
PS9, Line 134: base_callref
> The key idea is that allocating multiple transactions with same callref is 
> wrong, and we shouldn't do this at least in tests.

I strongly disagree. We should never try to "fix" issues by making tests behave 
differently from the code. The right way is to add test which illustrate 
current code behavior (even if it's wrong), than fix the code and update the 
test accordingly.



--
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: 10
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 10:36:10 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Max]

Hello Vadim Yanitskiy, Neels Hofmeyr, Harald Welte, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/12556

to look at the new patch set (#11).

Change subject: Add tests for transaction routines
..

Add tests for transaction routines

Related: OS#3294
Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713
---
M configure.ac
M tests/Makefile.am
M tests/testsuite.at
A tests/trans/Makefile.am
A tests/trans/trans_test.c
A tests/trans/trans_test.err
A tests/trans/trans_test.ok
7 files changed, 272 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/56/12556/11
--
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: newpatchset
Gerrit-Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713
Gerrit-Change-Number: 12556
Gerrit-PatchSet: 11
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Vadim Yanitskiy 

Change in osmo-msc[master]: Add tests for transaction routines

[Vadim Yanitskiy]

Vadim Yanitskiy has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 9:

(3 comments)

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@68
PS9, Line 68: base_callref
> What's the point of using static counter? What would be the advantage 
> compared to explicitly passing […]
We would avoid one meaningless parameter ;)

As a bonus, you could reflect values of this static counter in the logging 
output,
instead of counting the amount of allocated transactions manually.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@94
PS9, Line 94: vlr_subscr_name
> I don't think we have function to print ran-conn itself and there's 1:1 
> mapping between VLR subscrib […]
Ok, I see. What about this?

  LOG_RAN_CONN_CAT(t->conn, DCC, LOGL_DEBUG, "Connection assigned to 
transaction\n");

Off-topic: Now I think that we need something like LOG_TRANS and 
LOG_TRANS_CAT() for transactions.


https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@134
PS9, Line 134: base_callref
>> The key idea is that allocating multiple transactions with
>> same callref is wrong, and we shouldn't do this at least
>> in tests.
>
> I strongly disagree. We should never try to "fix" issues by
> making tests behave differently from the code.

Actually, your test case behaves differently from the code.
You won't see any existing code parts, which *intentionally
use same callref* for new transactions. Despite potentially
this may happen (e.g. triggered by external MNCC / EUSE).

So, I don't ask you to fix trans_alloc(), I ask you not to use
the same callref value for multiple transactions.

> The right way is to add test which illustrate current code
> behavior (even if it's wrong), than fix the code and update
> the test accordingly.

Yes, and this is what I would like to see in this (or in a
subsequent) change. Please illustrate the current behavior
(e.g. in a separate test_trans_callref function) by allocating
a few transactions with same callref, and print some warning
until trans_alloc() is fixed.



--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 12:10:41 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

Change in osmo-msc[master]: Add tests for transaction routines

[Vadim Yanitskiy]

Vadim Yanitskiy has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 11: Code-Review-1

Keeping my -1 because I am against using same callref for multiple 
transactions, and not all my comments were addressed.


--
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: 11
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Tue, 15 Jan 2019 12:13:11 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes

Change in osmo-msc[master]: Add tests for transaction routines

[neels]

neels has abandoned this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/12556 )

Change subject: Add tests for transaction routines
..


Abandoned
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/12556
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I78dfb7cd35073a305cf668beda7d9d58d5a5a713
Gerrit-Change-Number: 12556
Gerrit-PatchSet: 11
Gerrit-Owner: Max 
Gerrit-Assignee: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: fixeria 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 
Gerrit-MessageType: abandon

Change in osmo-msc[master]: Add tests for transaction routines

[Harald Welte]

Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/12556 )

Change subject: Add tests for transaction routines
..


Patch Set 11:

I presume the MSC has changed too far to still make use of this? In that case, 
please aandon.


--
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: 11
Gerrit-Owner: Max 
Gerrit-Assignee: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Fri, 10 May 2019 10:03:11 +
Gerrit-HasComments: No
Gerrit-HasLabels: No