Patch Set 3: Code-Review-1 (5 comments)
marking -1 for some of the comments; Really close to a +2. https://gerrit.osmocom.org/#/c/3129/3/src/osmo_gsm_tester/esme.py File src/osmo_gsm_tester/esme.py: Line 98: lambda pdu: self.dbg('message sent unhandled resp:', pdu.sequence) ) clarify ... "message we are sending contains an unhandled response"? can't figure out what this means. Line 116: def message_received_handler(self, pdu, *args): might be good to indicate that this is not part of the intended testing API but an internal handler, by prepending an underscore to the name? Line 126: self.log('FIXME: wait_receipt disabled because receipts are not received, see OsmoNITB #2353') hmm, we don't have an expected-to-fail mechanism yet. this would qualify to be an xfail... Line 166: def process_pdus_pending(self, pdu, **kwargs): public API or internal? https://gerrit.osmocom.org/#/c/3129/3/suites/aoip_smpp/esme_ms_sms_storeforward.py File suites/aoip_smpp/esme_ms_sms_storeforward.py: Line 52: wait(esme.receipt_was_received, umref) ...like with xfail: wait(emse.receipt_was_received, umref, timeout=30) but we don't have an xfail yet. Generally we will often need an xfail of sorts. Not necessarily in this patch, just thinking because it's the first time this shows up. -- To view, visit https://gerrit.osmocom.org/3129 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c0c325fee14143deca8310312fc530cd9ce92e Gerrit-PatchSet: 3 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-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-HasComments: Yes
