Patch Set 1: (9 comments)
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 exclam It could happen in the sense that at some point we also receive the "CallAdded" signal and we need to process and add it. But still I prefer having it added directly here before returning control to the test in order to not fail if the test request any operation with that path before we receive/process the Added signal. But indeed, I'll remove the exclamation mark. It was there from the beginning because I was testing the behavior of the interface. Line 586: def call_answer(self, call_id): > (could add a timeout arg to forward to wait() ... not necessarily in this p Good idea. I'll see if I add it with this patch or leave it for later. Line 594: call_dbus_obj = systembus_get(call_id) > validation that call_dbus_obj exists? what happens if there is no such call I guess it will throw an exception. I wouldn't invest time in this until we see this really happens and it's an issue. 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_sta I could add a "call_exists(call_id) which basically does systembus_get(call_id), catches exception and returns true/false. Line 612: self.dbg('Call already exists!') > (same as above; exclamation mark looks like a bad situation, yet this is ju Agree, I'll remove the "!". 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 k Agree, that's from the time I was testing behavior of the interface 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 I think we should have some kind of test-side library of function for steps which are re-used by several tests. Then just import it in tests willing to use it. By test-side I mean it's not necessarily a "public API" from osmo-gsm-tester, but just a bunch of functions outside of "osmo_gsm_tester.test" module. Line 40: mt_call_id = ms_mt.call_id_list()[0] > I guess the above two lines deserve to be in a AFAIR the VoiceCall interface has the number of the other endpoint in its properties. we could have ms_mt.call_wait_incoming(ms_or_msisdn) and then wait and iterate over active calls and look for the one which matches the msisdn. If ms_or_msisdn is None, then we take the first one (and/or make sure there's only one active to avoid unwanted race codnitions?) Agree with the assert part. 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 s You cannot do that because it may take a while (delay) for the other ms to close the call, that's why basically we use wait() relying on Timeout and not assert here. -- 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 <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-HasComments: Yes
