Patch Set 5: (10 comments)
will my comments ever end!? I feel apologetic to complain about so many things ... but I'd rather have a high coding standard. (Please be as strict with me in turn) I promise to merge soon so we can carry on with other tasks. https://gerrit.osmocom.org/#/c/2669/5/src/osmo_gsm_tester/suite.py File src/osmo_gsm_tester/suite.py: Line 182: def init_test_vars(self): actually, just 'clear()' or rather 'start()' would be an even better name? https://gerrit.osmocom.org/#/c/2669/5/src/osmo_gsm_tester/trial.py File src/osmo_gsm_tester/trial.py: Line 193: if suite_run.run_tests(names) == suite.SuiteRun.FAIL: (IIRC you agreed to take it out to a separate line but it's still in the 'if' condition) Line 195: elif self.status == suite.SuiteRun.UNKNOWN: (I'm sure this must be Trial.UNKNOWN as stated above) https://gerrit.osmocom.org/#/c/2669/5/src/osmo_gsm_tester/trial_report.py File src/osmo_gsm_tester/trial_report.py: Line 27: there is a mix here of using self.trial vs. passing trial as an argument. None of the *_to_junit() actually use class members, so all of this could be plain functions at root level; caller invokes a simple 'trial_to_junit(trial)' function or something, which calls all the rest; a separate function may write to a file ... or something. No class instance needed. Python also supports function defs inside a function, so if you like you could even make it def report_junit(_trial): def test_to_junit(_test): return foo def suite_to_junit(_suite): test_to_junit(_suite.test) return bar suite_to_junit(_trial.suite) return baz (well, up to your taste, just saying that it can be a unit of code without the need for a class and writing @staticmethod everywhere) Line 32: junit_path = self.trial.get_run_dir().new_file(self.trial.name()+'.xml') this composes the same path as below, which is a) a source of errors and b) will probably not work because new_file makes sure to avoid overwriting an existing file. Line 34: self.trial.log(elements.tostring()) should we really log the junit XML markup to the log file? if yes, please call 'self.dbg()' instead. Can we just drop the log() function? Line 37: junit_path = self.trial.get_run_dir().new_file(self.trial.name()+'.xml') I think trial.py should feed the file name. Line 38: self.trial.log('Storing JUnit report in', junit_path) I think trial.py should do this logging. This code should strictly compose the content. Line 75: class TrialReportText: (same as for TrialReportJunit above applies here) Line 79: Osmocom code usually avoids double blank lines *everywhere*. -- 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: 5 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
