This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Patch Set 1: Code-Review+2
(12 comments)
yay, I'm really glad that we finally feature a voice test!
And I didn't even raise a finger for it :)
I have a few questions and some hints to tighten, but I'll leave it up to you, it's already good enough to be merged.
https://gerrit.osmocom.org/#/c/4150/1/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:
Line 583: self.log('Dial returned already existing call!')
is it an error when the call object is already known? The log has an exclamation mark but isn't self.err(). Preferably not send mixed signals, and possibly return False or raise if it is an error...
Line 586: def call_answer(self, call_id):
(could add a timeout arg to forward to wait() ... not necessarily in this patch)
Line 594: call_dbus_obj = systembus_get(call_id)
validation that call_dbus_obj exists? what happens if there is no such call?
Line 601: call_dbus_obj = systembus_get(call_id)
what happens if there is no such call? Say I had a loop that polls call_state() and at some point the call has ceased to exist, is the only way to notice then to catch an exception?
Line 602: props = call_dbus_obj.GetProperties()
and will it be a NoneTypeException here?
Line 612: self.dbg('Call already exists!')
(same as above; exclamation mark looks like a bad situation, yet this is just a dbg() log)
Line 619: self.dbg('Trying to remove non-existing call!')
why all the shouting? :) It may be a personal preference of mine, but I'm kind of against exclamations in logs, prefer calm remarks on what the situation is. If they deserve serious attention, they should be raised exceptions.
https://gerrit.osmocom.org/#/c/4150/1/src/osmo_gsm_tester/schema.py
File src/osmo_gsm_tester/schema.py:
Line 85: if val in ('sms', 'gprs', 'voice', 'ussd'):
nice :)
https://gerrit.osmocom.org/#/c/4150/1/suites/voice/mo_mt_call.py
File suites/voice/mo_mt_call.py:
Line 35: wait(msc.subscriber_attached, ms_mo, ms_mt)
(btw, unrelated to this particular patch, the above is a common prelude to pretty much any test that doesn't test registration or startup itself ... I think at some point we should think about having a suite start up a core net in an initial setup script, and each subsequent test just using those binaries without having to restart them, as in https://osmocom.org/issues/2208 ; and/or, if we positively need to restart everything all the time, have a simple one-liner to startup everything instead of code dupping in each test)
Line 38: wait(lambda: mo_call_id in ms_mo.call_id_list())
should this wait maybe be part of call_dial()?
Line 40: mt_call_id = ms_mt.call_id_list()[0]
I guess the above two lines deserve to be in a
mt_call_id = ms_mt.call_wait_incoming()
...there is no way to know the incoming call_id in advance, right? Can we wait for a call from a specific caller ID?
Ah, and before we start, above call_dial(), we should assert that there is no call left over from before that could mess up the wait here:
assert not ms_mt.call_id_list()
Line 53: wait(lambda: len(ms_mo.call_id_list()) == 0 and len(ms_mt.call_id_list()) == 0)
and since we're waiting for an empty list here, we should also, before we start calling above, do
assert not ms_mo.call_id_list()
--
To view, visit https://gerrit.osmocom.org/4150
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib402effc830db293f27a877658894e454a93a606
Gerrit-PatchSet: 1
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes