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 3: Code-Review-1
(14 comments)
Thanks! I still have some complaints though, sorry if I'm a bit pedantic, but trying to make optimal choices for later code maintenance...
https://gerrit.osmocom.org/#/c/2669/3/src/osmo_gsm_tester/suite.py
File src/osmo_gsm_tester/suite.py:
Line 124: if self.status == Test.UNKNOWN:
Imagine for some obscure reason we have a status != UNKNOWN, but still an exception occurs. This exception should be stronger than anything else, and we should always make sure the exception's message ends up in the failure report. We can just drop this 'if', can't we?
Line 133: self.set_fail(ftype, fmsg, False)
I still think using try:..except: with class names would be nicer than isinstance(); but I'm not going to delay more because of personal preference on cosmetics.
Line 168: testcase = et.Element('testcase')
can't we have the junit related stuff separate, in one place? I mean, you have a hierarchy of Trial->suite_run->tests, each storing their own results. It should'nt be too much effort to merely feed the root trial to one function which translates the whole tree to junit? I see junit xml as a kind of separate feature and would like it to stay separate code wise. Would that be fine with you?
It could make sense to have a function that returns all the values needed for junit in one standardized tuple for all of the separate classes, but still I'd like the XML composition to be in its own py file. So that here we have the pure values that any reporter could use, and there we have the code that puts it in a specially formatted file which happens to be junit.
Line 191: FAIL = 'FAIL'
(could use global values and share across Test and SuiteRun)
Line 211: self.ts_start = time.time()
let's write out the ts. Is it 'timestamp'? 'test suite'? IMHO either start_timestamp or suite_started...
Line 212: self.ts_end = time.time()
again: why store ts_end?
Line 213: self.time = 0
again: duration?
Line 251: self.init_test_vars()
we're not planning to use a suite_run more than once, and init_test_vars() happened in the constructor. It's fine to safeguard, but then the function should be called 'clear_test_vars' instead.
Ah, is this about the starting timestamp?
Line 266: self.time = round(self.ts_end - self.ts_start)
again no need to store end timestamp.
https://gerrit.osmocom.org/#/c/2669/3/src/osmo_gsm_tester/trial.py
File src/osmo_gsm_tester/trial.py:
Line 38: FAIL = 'FAIL'
(add to "could re-use", now the list is: Test, SuiteRun, Trial)
Line 67: self.junitfname = self.get_run_dir().new_file(self.name()+'.xml')
(junit_path? junit_file_path? junit_output_path?)
Line 190: def run_suites(self, names=None):
spotting a stupidity in my design ... created https://osmocom.org/issues/2271
Line 193: if suite_run.run_tests(names) == suite.SuiteRun.FAIL:
(because I was kind of not seeing it for a minute, I'd prefer if the big call were more visible, separate from the result eval...
result = suite_run.run_tests(names)
if result...
)
Line 195: elif self.status == suite.SuiteRun.UNKNOWN:
Trial.UNKNOWN!
It works but is semantically wrong. Would be fixed for free if we re-use constants across all.
--
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: 3
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