| Message ID | 1459957959-15333-2-git-send-email-msuraev@sysmocom.de |
|---|---|
| State | Not Applicable |
| Headers | show |
A few things in this patch draw my attention: - the verbose parameters have to be passed to each function call instead of having a global verbosity switch? Can make sense, but does it? It doesn't look like anyone is ever using the verbosity anyway? - the function defs are below their use; not a problem in python but unusual as far as I'm concerned. - 5 functions are added, only two are used. Why add unused functions? Will they ever be used? - Two functions are factored out and three others are added afresh. Admitted, the second part of the log message mentions adding functions, yet the summary above sounds like it's only factoring out. Sorry if I'm getting on your case with these details, but it seems you've thrown in too many curiosities at the same time for my taste :) ~Neels On Wed, Apr 06, 2016 at 05:52:39PM +0200, msuraev@sysmocom.de wrote: > From: Max <msuraev@sysmocom.de> > > Introduce several python functions for sending various ipaccess > protocol messages. Convert existing test code to use them. > --- > openbsc/tests/vty_test_runner.py | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py > index 8db0825..1c4ecb4 100644 > --- a/openbsc/tests/vty_test_runner.py > +++ b/openbsc/tests/vty_test_runner.py > @@ -700,13 +700,13 @@ class TestVTYNAT(TestVTYGenericBSC): > self.assertEqual(data, "\x00\x01\xfe\x04") > > print "Going to send ID_RESP response" > - res = ussdSocket.send("\x00\x07\xfe\x05\x00\x04\x01\x6b\x65\x79") > + res = ipa_send_resp(ussdSocket, "\x6b\x65\x79") > self.assertEqual(res, 10) > > # initiating PING/PONG cycle to know, that the ID_RESP message has been processed > > print "Going to send PING request" > - res = ussdSocket.send("\x00\x01\xfe\x00") > + res = ipa_send_ping(ussdSocket) > self.assertEqual(res, 4) > > print "Expecting PONG response" > @@ -971,6 +971,31 @@ def add_nat_test(suite, workdir): > test = unittest.TestLoader().loadTestsFromTestCase(TestVTYNAT) > suite.addTest(test) > > +def ipa_send_pong(x, verbose = False): > + if (verbose): > + print "\tBSC -> NAT: PONG!" > + return x.send("\x00\x01\xfe\x01") > + > +def ipa_send_ping(x, verbose = False): > + if (verbose): > + print "\tBSC -> NAT: PING?" > + return x.send("\x00\x01\xfe\x00") > + > +def ipa_send_ack(x, verbose = False): > + if (verbose): > + print "\tBSC -> NAT: IPA ID ACK" > + return x.send("\x00\x01\xfe\x06") > + > +def ipa_send_reset(x, verbose = False): > + if (verbose): > + print "\tBSC -> NAT: RESET" > + return x.send("\x00\x12\xfd\x09\x00\x03\x05\x07\x02\x42\xfe\x02\x42\xfe\x06\x00\x04\x30\x04\x01\x20") > + > +def ipa_send_resp(x, tk, verbose = False): > + if (verbose): > + print "\tBSC -> NAT: IPA ID RESP" > + return x.send("\x00\x07\xfe\x05\x00\x04\x01" + tk) > + > def add_bsc_test(suite, workdir): > if not os.path.isfile(os.path.join(workdir, "src/osmo-bsc/osmo-bsc")): > print("Skipping the BSC test") > -- > 2.8.1 >
Thanks for review. Comments are inline On 04/07/2016 12:47 AM, Neels Hofmeyr wrote: > A few things in this patch draw my attention: > > - the verbose parameters have to be passed to each function call instead of > having a global verbosity switch? Can make sense, but does it? It doesn't > look like anyone is ever using the verbosity anyway? They were useful for debugging the tests but once it's ready the'd just clutter the output unnecessary. Still, they'll be useful again when we want to expand tests further so instead of adding and removing them every time I'd prefer to keep them. > > - the function defs are below their use; not a problem in python but unusual as > far as I'm concerned. Matter of taste I guess: I prefer to have all the functions not belonging to particular class to be in one place instead being spread over the file according to where it's used. > > - 5 functions are added, only two are used. Why add unused functions? > Will they ever be used? Perhaps :) I've added them while trying to mock BSC and MSC. It turned out that not all of them are actually necessary for current test-cases. Nevertheless I'd rather keep them because those are simple wrappers around magic hex constants for IPA protocol which are tried and tested, so next time we need more tests we can just use them right away instead of duplicating the efforts of finding out proper hex number sequences. > > - Two functions are factored out and three others are added afresh. Admitted, > the second part of the log message mentions adding functions, yet the summary > above sounds like it's only factoring out. > How would you summarize it?
On Thu, Apr 07, 2016 at 11:15:31AM +0200, Max wrote: > They were useful for debugging the tests but once it's ready the'd just > clutter the output unnecessary. Still, they'll be useful again when we > want to expand tests further so instead of adding and removing them > every time I'd prefer to keep them. would be nice to mention in the log message body... > > - 5 functions are added, only two are used. Why add unused functions? > > Will they ever be used? > > Perhaps :) also nice to mention in the log message body > > - Two functions are factored out and three others are added afresh. Admitted, > > the second part of the log message mentions adding functions, yet the summary > > above sounds like it's only factoring out. > > > How would you summarize it? "vty_test_runner: ipa_send: factor out 2, add 3 functions" :) BTW, just to set the tone, I would like to mention that my feedback is generally meant as friendly comment, in a happy office, from colleague to colleague ;) (I'd be glad if you'd return such reviews on my patches.) ~Neels
diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index 8db0825..1c4ecb4 100644 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -700,13 +700,13 @@ class TestVTYNAT(TestVTYGenericBSC): self.assertEqual(data, "\x00\x01\xfe\x04") print "Going to send ID_RESP response" - res = ussdSocket.send("\x00\x07\xfe\x05\x00\x04\x01\x6b\x65\x79") + res = ipa_send_resp(ussdSocket, "\x6b\x65\x79") self.assertEqual(res, 10) # initiating PING/PONG cycle to know, that the ID_RESP message has been processed print "Going to send PING request" - res = ussdSocket.send("\x00\x01\xfe\x00") + res = ipa_send_ping(ussdSocket) self.assertEqual(res, 4) print "Expecting PONG response" @@ -971,6 +971,31 @@ def add_nat_test(suite, workdir): test = unittest.TestLoader().loadTestsFromTestCase(TestVTYNAT) suite.addTest(test) +def ipa_send_pong(x, verbose = False): + if (verbose): + print "\tBSC -> NAT: PONG!" + return x.send("\x00\x01\xfe\x01") + +def ipa_send_ping(x, verbose = False): + if (verbose): + print "\tBSC -> NAT: PING?" + return x.send("\x00\x01\xfe\x00") + +def ipa_send_ack(x, verbose = False): + if (verbose): + print "\tBSC -> NAT: IPA ID ACK" + return x.send("\x00\x01\xfe\x06") + +def ipa_send_reset(x, verbose = False): + if (verbose): + print "\tBSC -> NAT: RESET" + return x.send("\x00\x12\xfd\x09\x00\x03\x05\x07\x02\x42\xfe\x02\x42\xfe\x06\x00\x04\x30\x04\x01\x20") + +def ipa_send_resp(x, tk, verbose = False): + if (verbose): + print "\tBSC -> NAT: IPA ID RESP" + return x.send("\x00\x07\xfe\x05\x00\x04\x01" + tk) + def add_bsc_test(suite, workdir): if not os.path.isfile(os.path.join(workdir, "src/osmo-bsc/osmo-bsc")): print("Skipping the BSC test")
From: Max <msuraev@sysmocom.de> Introduce several python functions for sending various ipaccess protocol messages. Convert existing test code to use them. --- openbsc/tests/vty_test_runner.py | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)