test-fullautomation / robotframework-testsuitesmanagement

Apache License 2.0
4 stars 2 forks source link

Clarification wanted: Implementation of version control in CConfig.py #272

Closed HolQue closed 2 months ago

HolQue commented 7 months ago

Hi Son,

(1)

In bValidateSubVersion an exception is thrown, including a reason for this exception:

raise Exception("Wrong format in version info")

But when you call bValidateSubVersion, you do not use this information. In BuiltIn().fatal_error() you create a new reason instead:

try:
    # verify the version info is a number
    return tuple(map(lambda x: CConfig.bValidateSubVersion(x), lVersion))
except Exception:
    BuiltIn().fatal_error(f"Provided version '{sVersion}' is not a correct version format.")

Why this? For me this makes no sense. I would prefer:

except Exception as reason:
    BuiltIn().fatal_error(f"{reason}")

(2)

BuiltIn().fatal_error() makes the test result FAIL. Here I have doubts. A user defines an invalid version format. No test is executed therefore. In this case I would expect the test result to be UNKNOWN.

(3)

I tried to reproduce this error (the error message above). Without success.

For example:

"Maximum_version" : "1.0xyz.0",

Result:

Parameter 'Maximum_version' with invalid value found in JSON configuration file! Reason: '1.0xyz.0' does not match '^\\d+\\.\\d+\\.\\d+$'

Test result is UNKNOWN. This is completely fine. But does not fit to the code listed above.

(4)

The docstring tells:

This tupleVersion also support version which contains Alpha (a), Beta (b) or
Release candidate (rc): E.g: "1.2rc3", "1.2.1b1", ...

Therefore I tried:

"Maximum_version" : "1.2.1b1",

With result:

Parameter 'Maximum_version' with invalid value found in JSON configuration file! Reason: '1.2.1b1' does not match '^\\d+\\.\\d+\\.\\d+$'

Now I am confused. Seems that the docstring tells something invalid. But the main question is: Is this version control mechanism still active? What is the desired behavior in this case? If the code cannot be reached any more - because already another code does the error handling - then the non reachable code should be removed.

namsonx commented 2 months ago

Hello Holger,

Regarding to the points:

Could you please help me review and verify again?

Thank you, Son

namsonx commented 2 months ago

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