[ 2375000 ] Code clarification wanted

[HolQue]

Hi Son,

for me the following code looks very long winded:

if not RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel1:
    if sTestsuiteCfgFile != '':
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel2 = True
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel4 = False
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.sTestSuiteCfg = os.path.abspath(sTestsuiteCfgFile)
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.loadCfg(RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig)
    else:
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel3 = True
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel4 = False
        RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.loadCfg(RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig)

if RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel1:
    logger.info('Running with configuration level 1')
elif RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel2:
    logger.info('Running with configuration level 2')
elif RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel3:
    logger.info('Running with configuration level 3')
else:
    logger.warn("Running with configuration level 4!")

Is it really required to have a separate boolean level flag for every level? During runtime only one certain level can be chosen. Why not a single integer variable holding the level number? This would prevent you from maintaining four different variables (including the need to make sure that they have consistent values - only one of them can be True, but not more than one).

And this:

else:
    logger.warn("Running with configuration level 4!")

is in my opinion an invalid assumption. Your code also contains bLevel4. Therefore you have to ask also this variable which configuration level is active.

At least I would expect an explicit check like:

elif RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.rConfigFiles.bLevel4:
    logger.warn("Running with configuration level 4!")
else:
    BuiltIn().unknown("internal error")

It is also not necessary to have

RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig.loadCfg(RobotFramework_TestsuitesManagement.CTestsuitesCfg.oConfig)

twice in the code. Move this line to outside if/else.

[HolQue]

Addendum:

Hi Son,

please find below a piece of pseudo code, that would be in my opinion a better solution, because this is much more well arranged and easier to understand and to maintain than the current code and avoids a lot of redundancy. Therefore please try to integrate this concept into your current implementation.

class CConfigurationLevel(Enum):
    LEVEL_1 = 1
    LEVEL_2 = 2
    LEVEL_3 = 3
    LEVEL_4 = 4

nLevel = CConfigurationLevel.LEVEL_4 # default
<check command line>
if ( (parameter configuration file is provided in command line) and (name of the variant is provided in command line) ):
   <error handling because of duplicate command line elements>
   <result UNKNOWN>

if parameter configuration file is provided in command line:
   if file error:
      <error handling>
      <result UNKNOWN>
   nLevel = CConfigurationLevel.LEVEL_1
elif name of the variant is provided in command line:
   if variant name error:
      <error handling>
      <result UNKNOWN>
   nLevel = CConfigurationLevel.LEVEL_2
else:
   <check config folder>
   if config folder error:
      <error handling>
      <result UNKNOWN>
   if parameter configuration files within a folder config:
      if config file error:
         <error handling>
         <result UNKNOWN>
      nLevel = CConfigurationLevel.LEVEL_3

if nLevel == CConfigurationLevel.LEVEL_4:
   logger.warn(f"Running with fallback configuration level {nLevel} (this should be avoided)")
else:
   logger.info(f"Running with configuration level {nLevel}")

[namsonx]

Hello Holger,

Thank you very much for the hint! I improved the code related to configuration level as your suggestion.
I pushed the commit 703d4e566f to stabi branch.

Thank you,
Son

[namsonx]

Ready for sprint 0.13.1 [06.09.24](jsonpreprocessor / tsm)

[HolQue]

Hi Son,

the code is improved now a bit, but unfortunately is still long winded and difficult to understand.

Example:

bConfigLevel2 = True
if self.configLevel != TM.CConfigLevel.LEVEL_1:
    if self.configLevel==TM.CConfigLevel.LEVEL_2:
        bConfigLevel2 = self.__loadConfigFileLevel2()

If it's level 2, it cannot be level 1. Therefore there is no need to spend two lines of code:

if self.configLevel != TM.CConfigLevel.LEVEL_1:
    if self.configLevel==TM.CConfigLevel.LEVEL_2:

And please rename bConfigLevel2.

Everyone who reads this

bConfigLevel2 = self.__loadConfigFileLevel2()

will assume that bConfigLevel2 is the config level. But this variable does not contain the config level itself, it is an acknowledge indicating that self.__loadConfigFileLevel2() was able to load the configuration or not. Why do you need a level specific variable like bConfigLevel2 for that? Either it was able to load a configuration or not, no matter what level.

Later in the code you implement the following mapping:

if not bConfigLevel2: # Loading configuration level 2 failed, method self.__loadConfigFileLevel2() return False
    self.bLoadedCfg = False

Why is this necessary? Why not in this way:

if self.configLevel==TM.CConfigLevel.LEVEL_2:
    self.bLoadedCfg = self.__loadConfigFileLevel2()

In

#271 (comment)

I already gave a clear recommendation about how to implement the configuration load in a plausible and easy to understand way.

Please follow this recommendation and rework your code again.

[namsonx]

Hello Holger,

Thank you for your suggestions! Due to numerous changes in the requirements, the structure and flow of the code are not well-organized. I will refactor the code for this method.

Thank you,
Son

[HolQue]

Hi Son,

please consider both CSetup and CConfig, when you rework your code. In CSetup/testsuite_setup you have lots of code doing something with the configuration (TM.CConfigLevel.LEVEL_). But you have such kind of code also in CConfig.

Why is it necessary to have all these if conditions belonging to the configuration, present in two different files?

My actual understanding is that in CConfig the configuration is loaded, and in CSetup this loading is triggered. Under this circumstance I would not expect that CSetup has the need to contain several if conditions regarding TM.CConfigLevel.LEVEL_.

For example this code:

if TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_1:
    logger.info('Running with configuration level 1')
elif TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_2:
    logger.info('Running with configuration level 2')
elif TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_3:
    logger.info('Running with configuration level 3')
else:
    logger.warn("Running with configuration level 4!")

Why is this not done in CConfig immediately? Because there you load the configuration. Why not giving feedback about something immediately at the position, where this happened?

[namsonx]

Hello Holger,

I pushed the refactoring code to stabi branch. Could you please help me review and give your feedback?

Thank you,
Son

[HolQue]

Hi Son,

[1]

Please remove the invalid 'three dots' example in the docstring of class CConfig(). The updated feature is already explained properly in the common part of the documentation of TestsuitesManagement.

[2]

Please shorten the code. This really hurts:

if TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_1:
    logger.info('Running with configuration level 1')
elif TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_2:
    logger.info('Running with configuration level 2')
elif TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_3:
    logger.info('Running with configuration level 3')
else:
    logger.warn("Running with configuration level 4!")

And caution: The value of TM.CTestsuitesCfg.oConfig.configLevel is e.g. CConfigLevel.LEVEL_2 (and not 2). You'll need to use the value property of the enum to get the value: TM.CTestsuitesCfg.oConfig.configLevel.value

Then you can do this (and this is much shorter):

msg = f"Running with configuration level {TM.CTestsuitesCfg.oConfig.configLevel.value}"
if TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_4:
    logger.warn(msg)
else:
    logger.info(msg)

[3]

I still have doubts w.r.t. #271 (comment)

Do you see any possibility to have a better separation of concerns between CConfig and CSetup?

[HolQue]

Little goodie wanted:

Sometimes, when I am not so deep inside the TestsuitesManagement stuff any more, it is difficult for me to get the meaning of messages like:

"Running with configuration level 3" (What is the meaning of 3?)

This is similar to those magic numbers in code, that are also not wanted.

With the help of something like

levels_info = {1 : "configuration file in command line",
               2 : "variant name in command line",
               3 : "configuration file in local config folder",
               4 : "default configuration (fallback solution)"}

is is possible to provide feedback about the configuration level in this format:

"Running with configuration level 3 (configuration file in local config folder)"

That is in my opinion more helpful - and is already realized in:

test-fullautomation/RobotFramework_AIO#235 (comment)

If you make levels_info a part of your TestsuitesManagement configuration (e.g. TM.CTestsuitesCfg.oConfig) then you can use this information also at other positions where you write the level number to log files.

[namsonx]

Hello Holger,

The code

if TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_1:
    logger.info('Running with configuration level 1')
elif TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_2:
    logger.info('Running with configuration level 2')
elif TM.CTestsuitesCfg.oConfig.configLevel==TM.CConfigLevel.LEVEL_3:
    logger.info('Running with configuration level 3')
else:
    logger.warn("Running with configuration level 4!")

was removed and replaced by new code to present data table format as attached file.
datatable_log.log

I also updated the level info as your suggestion above.

Thank you,
Son

[test-fullautomation]

Hi @namsonx , @HolQue ,
what is the status of this issue?
Thank you,
Thomas

[HolQue]

Hi Son,

the code is improved a lot and I really would prefer to close the issue because it has already become too big.

Nevertheless, I still would prefer to have the code more plausible and easier to understand.

This is an extract of some lines in CConfig:

if self.configLevel == TM.CConfigLevel.LEVEL_1:
...
else:
    if self.configLevel==TM.CConfigLevel.LEVEL_2:
    ...
    else:
        ...
            self.configLevel = TM.CConfigLevel.LEVEL_3
            ...
                self.configLevel = TM.CConfigLevel.LEVEL_4
        ...
        if self.configLevel==TM.CConfigLevel.LEVEL_4:

This is a curious mixture of checking already detected levels (if self.configLevel==TM.CConfigLevel.LEVEL_2) and setting levels (self.configLevel = TM.CConfigLevel.LEVEL_3). Why is it implemented in this way? In my opinion this is confusing and very difficult to maintain.

Intuitively I would have expected something like this:

# 1. detection
self.configLevel = DetectConfigLevel(...)
# 2. reaction
if self.configLevel == TM.CConfigLevel.LEVEL_1:
    ...
elif self.configLevel == TM.CConfigLevel.LEVEL_2:
    ...
elif self.configLevel == TM.CConfigLevel.LEVEL_3:
    ...
elif self.configLevel == TM.CConfigLevel.LEVEL_4:
    ...
else:
    exception because of internal error while detecting the config level

I already made a corresponding proposal here:
#271 (comment)

The idea behind this proposal is to have a good separation between the detection of the config level (= checking
command line parameters and the existence of config files and folders) and the reaction (= reading the selected config file
and give feedback in console and log).

Please give this proposal a try. It would make the code much easier to understand and to maintain.