Patch Set 7: Code-Review+1 (4 comments)
I like this now. Minor comments... https://gerrit.osmocom.org/#/c/2669/7/src/osmo_gsm_tester/suite.py File src/osmo_gsm_tester/suite.py: Line 109: self.fail_message = '' set these to None instead? Line 151: self.duration = round(time.time() - self.start_timestamp) you are rounding because...? for cosmetic reasons? Rather ceil() so we don't make it shorter than it was ... or just keep the entire float value? Line 186: def clear_test_vars(self): still '_test_vars' sounds like it is related to the environment for a test script (like test.py:setup()). My favorites now are 'start()' or 'mark_start()'. Not blocking because of this. Still the argument remains that SuiteRun is intended to be run only once. We instantiate the class right when we start the suite run, so this should actually move into __init__? If it's cubersome to adjust the regression tests to this, though, I agree to keep this separate. Line 227: self.clear_test_vars() why call in the constructor *and* here? -- To view, visit https://gerrit.osmocom.org/2669 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedf6d912b3cce3333a187a4ac6d5c6b70fe9d5c5 Gerrit-PatchSet: 7 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-Reviewer: neels <[email protected]> Gerrit-HasComments: Yes
