Patch Set 3:

(13 comments)

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:
> ah I was assuming the failure is coming in here as an exception? ... it wou
Handling this duality is not a big issue and provides some way for tests to 
mark it as failure and continue cleaning/processing whatever if needed 
afterwards.


Line 133:                 self.set_fail(ftype, fmsg, False)
> only usually one looks at try:except clauses to figure out which exceptions
I'm not braking that convention, as expressed in "except Exception", all 
Exception are catched. I just check the type to improve the information 
provided on the message.


Line 191:     FAIL = 'FAIL'
> it's not like it is a C enum with benefits (like warning about missing case
Well as they are part of class, then it's clear status returned by that class 
can be of one of the status defined inside the same class. This way we easily 
now suite_run() cannot return SKIP for instance.


https://gerrit.osmocom.org/#/c/2669/5/src/osmo_gsm_tester/suite.py
File src/osmo_gsm_tester/suite.py:

Line 182:             ret += " (%s, %d sec)" % 
(datetime.fromtimestamp(round(self.ts_start)).isoformat(), self.time)
> actually, just 'clear()' or rather 'start()' would be an even better name?
I like the current name, but if you prefer one of those I'll change it.


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 'i
Sorry I forgot to apply this change, I'll do now. Too much code refactoring 
made me it missing this.


https://gerrit.osmocom.org/#/c/2669/4/src/osmo_gsm_tester/trial_report.py
File src/osmo_gsm_tester/trial_report.py:

Line 26
> since it reports on trial, suites and tests, maybe call this just 'Report' 
OK


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. N
Ok I'll remove the classes.

I prefer to have all of them top level to be able to re-use them if needed to 
print a suite. For instance for the text one it is used in selftest.


Line 32
> this composes the same path as below, which is a) a source of errors and b)
Sorry, this should not be there indeed.


Line 34
> should we really log the junit XML markup to the log file? if yes, please c
The log function is intended to explicitly log stuff so it makes no sense to 
remove the log or make it dbg I think. 

It is not used right now but as I was already writing it for TestReportText 
class I also put it here for completeness. As it's not used I'll remove it.


Line 37
> I think trial.py should feed the file name.
OK


Line 38
> I think trial.py should do this logging. This code should strictly compose 
OK


Line 75
> (same as for TrialReportJunit above applies here)
OK


Line 79
> Osmocom code usually avoids double blank lines *everywhere*.
OK


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

Reply via email to