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 1: Code-Review-1
(18 comments)
The result is exactly as I expect it, but I have various suggestions to rejigger the code structure. I'd rather combine the Result / Report classes, keep the junit stuff in one py file instead of spreading, and also have some ideas to improve the structure from before this patch (instead of building up on previous stupid structuring I put in there).
Let me know what you think...
https://gerrit.osmocom.org/#/c/2669/1/selftest/suite_test.ok
File selftest/suite_test.ok:
Line 74: ERROR: [test_error.py] ([TS_ISO8601], 0 sec) type:'AssertionError' message: AssertionError()
interesting, "TS_ISO8601"?
https://gerrit.osmocom.org/#/c/2669/1/selftest/suite_test.ok.ign
File selftest/suite_test.ok.ign:
Line 3: [0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2} [TS_ISO8601]
ah of course :)
https://gerrit.osmocom.org/#/c/2669/1/src/osmo-gsm-tester.py
File src/osmo-gsm-tester.py:
Line 198: trials_run.append((current_trial.name(), report))
now that I look at it, also applies to my code from before this patch: it might make more sense to place the reporting in the SuiteRun and Trial classes instead. IMHO the TrialReport should be implicit within the Trial class. It could log 'FAIL' or 'PASS' as well as write the report in the __exit__ function. Below log_report() should also happen directly in there. The suite run should do all its reporting in the run_tests() function.
I'm now pretty certain that I should never have added the ability to run several trials at once. I guess we should remove that (at some point, or maybe in a patch leading up to this patch).
https://gerrit.osmocom.org/#/c/2669/1/src/osmo_gsm_tester/suite.py
File src/osmo_gsm_tester/suite.py:
Line 98: return stats[st]
python and enums is always a bit of a mess ... but to simplify this, we could rather use
UNKNOWN = 'UNKNOWN'
SKIP = 'SKIP'
...
and use the constants as strings directly. AFAICT we never compare the number values with < or >, so that would work as-is. Replace 'status2str()' with the constant itself, or
def status2str(st):
return st
:)
Line 135: msg += '\n' + 'Current resource state:\n' + repr(suite_run.reserved_resources)
Looks a bit messy. Instead of using isinstance on exceptions, one would usually do separate 'except' handlers. e.g.
try:
try:
try:
...
except NoResourceExn:
self.err('current resource state...')
raise
except:
self.log_exn()
raise
except Failure as e:
self.set_fail()
except:
self.set_error()
and have the 'if status == UNKNOWN' somehow handled in the set_fail() or set_error() implicitly.
(I know log.py does much more weird things, but would be good to constrain that madness to log.py)
NoResourceExn: IMHO it suffices to have the list of resources in the log; otherwise: nicer could be to have the current resource state in the NoResourceExn directly so it is included in the report implicitly. i.e. require passing current resource dict to NoResourceExn.__init__(); I think in resource.py e.g. in Resources.find(), we can just pass 'self' to the NoResourceExn? (but is it worth the effort?)
Line 187: return testcase
I think it would be better to keep all et.Element related stuff in the TrialReport class.
Line 239: class Results:
since we have a TrialReport, maybe this should become a SuiteReport? Combine both in a file called report.py and keep all the junit stuff constrained to that file?
Line 241: self.suite_run = suite_run
looks like you're only using suite_run.name(), so let's just pass the in here name then?
Line 245: self.ts_end = time.time()
ts?
Can we set end time = None until we know what the end time is?
Line 246: self.time = 0
with above timestamps, what is this for? Ah, the total time. Since 'time' is both a python module name and a bit ambiguous, let's name it 'duration' or even 'duration_in_seconds'?
Line 260: self.skipped_ctr += 1
let's have an if.. elif.. else here to catch an invalid status ... or check that in conclude() or something. Whichever way, make sure nothing can fall thru the cracks
Line 263: self.ts_end = time.time()
ah seems to me we don't even need to to store the end timestamp
Line 264: self.time = round(self.ts_end - self.ts_start)
is rounding required?
Line 271: testsuite.set('timestamp', datetime.fromtimestamp(round(self.ts_start)).isoformat())
is rounding required?
https://gerrit.osmocom.org/#/c/2669/1/src/osmo_gsm_tester/trial_report.py
File src/osmo_gsm_tester/trial_report.py:
Line 1: # osmo_gsm_tester: trial: directory of binaries to be tested
trial report?
Line 53: msg += '\n '.join(str(result) for result in self.results)
(that's interesting, so far I thought a list comprehension like that always required [] braces ... well, learning never ends)
https://gerrit.osmocom.org/#/c/2669/1/suites/debug/error.py
File suites/debug/error.py:
Line 5: assert False
in the docs, at least in the example, I propose 'assert' as a way to fail the test (as in not classify it as test error). I kind of like the simplicity of just calling assert, and/or just have any exception in the test run end up as a test failure. we could push the 'error' classification all the way up to things like "no space left" or "config file not found"... If we want an 'error' classification, we could raise a specific 'TestError' exception (in specific places in our osmo_gsm_tester/ files as well as in test scripts if needed), and all others are counted as 'failure' -- does that make sense? (we can also tweak that later)
https://gerrit.osmocom.org/#/c/2669/1/suites/debug/pass.py
File suites/debug/pass.py:
Line 5: assert True
heh, a nop. The file might as well be empty.
It could have a "print('success')" instead?
--
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: 1
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: neels <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes