Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug #111 Create Session kw prints Arg type #112

Merged
merged 3 commits into from May 26, 2016

Conversation

roimvargas
Copy link
Contributor

Convert the max_retries argument to integer using native python method int(), instead of the Robot convert_to_integer keyword. This avoid that garbage message is printed to Robot logs as: Argument types are: <type 'unicode'>

Convert the max_retries argument to integer using native python method int(), instead of the Robot convert_to_integer keyword. This avoid that garbage message is printed to Robot logs as: Argument types are: <type 'unicode'>
@@ -80,7 +80,7 @@ def _create_session(
s.auth = auth if auth else s.auth
s.proxies = proxies if proxies else s.proxies

max_retries = self.builtin.convert_to_integer(max_retries)
max_retries = int(max_retries)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any failing scenario/example test without using self.builtin.convert_to_integer(max_retries)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With current code the Robot Test Case itself does not fail, just garbage messages are printed to Robot Framework logs.
A really simple test case is:

*** Settings ***
Library           RequestsLibrary

*** Test Cases ***
testCreateSession
    Create Session    v_url    http://www.google.com/    max_retries=1

It generates the following output.xml log:

<kw type="kw" name="RequestsLibrary.Create Session">
    <doc>Create Session: create a HTTP session to a server</doc>
    <arguments>
        <arg>v_url</arg>
        <arg>http://www.google.com/</arg>
        <arg>max_retries=1</arg>
    </arguments>
    <msg timestamp="20160512 11:08:44.226" level="INFO">Creating Session using : alias=v_url, url=http://www.google.com/, headers={},                     cookies=None, auth=None, timeout=None, proxies=None, verify=False,                     debug=0 </msg>
    <msg timestamp="20160512 11:08:44.229" level="INFO">Argument types are:
&lt;type 'unicode'&gt;</msg>
    <status status="PASS" endtime="20160512 11:08:44.229" starttime="20160512 11:08:44.225"/>
</kw>

And the debug log shows:

20160512 11:08:44.226 - INFO - +------ START KW: RequestsLibrary.Create Session [ v_url | http://www.google.com/ | max_retries=1 ]
20160512 11:08:44.226 - INFO - Creating Session using : alias=v_url, url=http://www.google.com/, headers={},                     cookies=None, auth=None, timeout=None, proxies=None, verify=False,                     debug=0
20160512 11:08:44.226 - DEBUG - Creating session: v_url
**20160512 11:08:44.229 - INFO - Argument types are:
<type 'unicode'>**
20160512 11:08:44.229 - INFO - +------ END KW: RequestsLibrary.Create Session (4)

The message is confusing and not required.
It is the expected behavior of convert_to_integer keyword of Robot Framework, which is not meant to be used by another keyword.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any docs on why convert_to_integer should not be used by another keyword ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The “Convert To Integer” is a Robot BuiltIn keyword. They “can” be called from other keywords but will print misleading log messages that are not related to the task of keyword calling it (in this case the Create Session keyword).

Another way to avoid the misleading log messages is to invoke the inner method _convert_to_integer that do not print to the logs, instead of its wrapper convert_to_integer.

Therefore, the line of code would be like:
max_retries = self.builtin._convert_to_integer(max_retries)

If you prefer this solution, I can do a Pull Request with that change instead.

Use internal robot builtin method _convert_to_integer in order to avoid unwanted entries in Robot log and output files.
@roimvargas
Copy link
Contributor Author

Hi @oleduc,

That would be nice to have a more meaningful error message if an error occurs while converting the max_retries input parameter to integer. Something like “Error converting max_retries parameter: ”. But this was not my original intention when raising this issue.

Anyways, we can do that using the below code:

        try:
            max_retries = self.builtin._convert_to_integer(max_retries)
        except RuntimeError as err:
            raise RuntimeError("Error converting max_retries parameter: %s"   % err)

If the Create Session is invoked like:
Create Session | v_url | http://www.google.com | max_retries=one
The below error message will be displayed:
FAIL : Error converting max_retries parameter: 'one' cannot be converted to an integer: ValueError: invalid literal for int() with base 10: 'one'

@bulkan, do you agree with this change? Can it be committed to the main code?

Thank you.

@oleduc
Copy link
Contributor

oleduc commented May 24, 2016

I don't think that using the private method self.builtin_convert_to_integer is the right solution here. It might differ from version to version. I think your initial solution where you did a cast using int() in combination with a custom exception handler might be the best solution here.

@roimvargas
Copy link
Contributor Author

I agree @oleduc.

In this case the code should be:

        try:
            max_retries = int(max_retries)
        except ValueError as err:
            raise ValueError("Error converting max_retries parameter: %s"   % err)

And the error message displayed will be:
ValueError: Error converting max_retries parameter: invalid literal for int() with base 10: 'one'

@bulkan, any comment?

@bulkan
Copy link
Collaborator

bulkan commented May 25, 2016

@roimvargas yep that sounds good.

Convert max_retries param to integer using int() casting function in order to avoid unwanted log message and add more meaningful error message if conversion to integer fails. Solution discussed at Pull Request MarketSquare#112 with @bulkan and @oleduc.
@oleduc
Copy link
Contributor

oleduc commented May 25, 2016

Looks good to me.

@bulkan bulkan merged commit a7a92a8 into MarketSquare:master May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants