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 <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
