test-fullautomation / robotframework-testsuitesmanagement

Apache License 2.0
4 stars 2 forks source link

Alignments of code and error messages wanted #267

Open HolQue opened 5 months ago

HolQue commented 5 months ago

Hi Son,

in __loadConfigFileLevel2 you use

logger.error(f"Loading of JSON configuration file failed! Reason: {CConfig.sLoadedCfgError}")

followd by

return False

In loadCfg() you use

BuiltIn().unknown('Loading of JSON configuration file failed!')

followed by

raise Exception

This is hard to capture and should be aligned. BuiltIn().unknown() has a high impact (test result). To place this inside a method followed by raise Exception, is strange. Is the line containing this raise reached? I assume that already BuiltIn().unknown() quits the computation. Please check for side effects.

In first example (__loadConfigFileLevel2) you repeat the error message with BuiltIn().unknown() outside __loadConfigFileLevel2. Outcome is this kind of redundancy:

ERROR - Loading of JSON configuration file failed!
UNKNOWN - Loading configuration level 2 failed!

Please try to avoid this.

I would prefer a solution like that (generally in all methods in a unique way):

def my_funk():
   acknowledge = True
   result = "unknown"
   ...
   if error:
      acknowledge = False
      result = "error message"
      return acknowledge, result
   ...

acknowledge, result = my_funk()
if acknowledge is not True:
   logger.error(f"{result}")
   BuiltIn().unknown('Loading of JSON configuration file failed!') # that is the final impact of all error message before and quits the computation

(something like that)

HolQue commented 4 months ago

Addendum - with some more details:

Sometimes

BuiltIn().unknown(...)

is placed at the end of an if case.

Sometimes in an if case BuiltIn().unknown(...)

is followed by

return

Sometimes

BuiltIn().unknown(...)

is followed by

raise Exception

This is confusing, hard to capture and hard to maintain.

BuiltIn().unknown(...) immediately quits the computation. Therefore it makes no sense to let further lines of code follow after this.

I would prefer the following:

No usage of BuiltIn().unknown(...) and BuiltIn().fatal_error() any more inside methods. Let every method return a flag indicating if the function has been executed with or without errors. Together with an error message if required.

At top level you receive the return values from method calls and decide about consequences. Top level are the keywords testsuite_setup, testsuite_teardown, testcase_setupand testcase_teardown.

In other words: A premature end of execution (unknown, fatal error) should only be allowed inside keyword definitions, but not in any other methods.

Code proposal:

def log_unknown(error=None, impact=None):
    default_error  = "An unknown error happened."
    default_impact = "The test execution will be aborted now."
    if error is None:
        error = default_error
    else:
        error = f"{error}"
    if impact is None:
        impact = default_impact
    else:
        impact = f"{impact}"
    logger.error(error)
    BuiltIn().unknown(impact)

@keyword
def testsuite_setup(self, sTestsuiteCfgFile=''):

    bSuccess, sResult = do_something(input_param)
    if bSuccess is not True:
        log_unknown(sResult, "testsuite_setup failed; execution will be aborted now")
    else:
        BuiltIn().log(f"{sResult}", "INFO")

It can really be so much easy and well arranged.

Please rework your sw architecture.