Patch Set 1: Code-Review-1 (8 comments)
some code structuring issues, looks good otherwise. https://gerrit.osmocom.org/#/c/2676/1/src/osmo_gsm_tester/ofono_client.py File src/osmo_gsm_tester/ofono_client.py: Line 84: def modem_is_powered(modem_obj, val): first reflex: if it requires a modem_obj and is part of the Modem class, then this is clearly not a @staticmethod. modem_obj == self. Later I notice: you mean the dbus_obj! I think that should be separate from this class then (also see below) and would be good to have dbus_ in the arg's name. Line 89: def modem_has_val_in_property(modem_obj, iface, prop_name, val): same Line 94: def build_imsi_modem_map(log_obj): we have 'list_modems()' above; this is on the same semantic level as list_modems(), so let's either also have this function separate, outside of the Modem() class, or also have list_modems() as @staticmethod in here. I think I would prefer to move this as well as the global imsi_modem_map out of the Modem class to the list_modems() level. (because a Modem is used in the tests API, and the other imsis and modems possibly assigned to other suite runs are of no concern to individual tests that were assigned their individual modems.) hint: imsi_modem_map = None def get_imsi_modem_map(): global imsi_modem_map # <-- hint: clearly mark as global if imsi_modem_map is not None: return imsi_modem_map ... Line 96: test.poll() (if you ever come up with a better idea than my test.poll() that would be welcome ... it works for now but isn't particularly beautiful to call the API inteded for test scripts, just to find the currently active suite_run to call poll() for) Line 143: self.path = conf.get('path') there is no 'path' in the conf anymore, though? Are you allowing both, either specific path or by imsi? I think we should have only one way to prevent feature overload (like me allowing multiple trial runs just complicating things...) Line 148: raise RuntimeError('Unable to find modem with IMSI %s (%s)' % (self.imsi, self.label)) rather use self.raise_exn(), see log.Origin.raise_exn(). (and yes, neither did I in the old imsi()) Line 156: def imsi(self): I guess we can drop the function entirely? Or can we? Let's also look at ki. Are we going to copy all the values from conf.* here, or are we just going to use conf.*? Previously, the code choice was to not copy. IMHO it's better to keep it that way, i.e. use imsi() and always look up in conf(). If you disagree, then change that for all conf.* values ... and we'd also do that for all future new conf.* items ... I think rather not? Line 160: return self.conf.get('ki') (from above: this is still using conf.*) -- To view, visit https://gerrit.osmocom.org/2676 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib9f4de81abc18e8db0c15df965e4811b6513e1b1 Gerrit-PatchSet: 1 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
