This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes