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

Variables should not leak to lower level keywords #532

Closed
spooning opened this Issue Jun 29, 2014 · 10 comments

Comments

Projects
None yet
4 participants
@spooning
Copy link
Contributor

spooning commented Jun 29, 2014

Originally submitted to Google Code by @pekkaklarck on 14 Apr 2010

Currently this is passes:

*** Test Case ***
Example
    ${x}=    Set Variable    hello
    My keyword

*** Keywords ***
My keyword
    Should be equal    ${x}    hello
    ${y}=    Set Variable    world
    Second keyword

Second Keyword
    Should be equal    ${x}    hello
    Should be equal    ${y}    world

This has definitely been a bad design decision, and a person who
has designed it years ago I can call it a bug rather than a feature.
Instead of variables leaking to lower level keywords like this, they should
be passed as argument or, in special cases, set explicitly using Set Test
Variable keyword.

Even though this is a misfeature, we cannot remove it without deprecating
first it because existing tests may depend on it. Unfortunately the current
variable implementation is so naïve that deprecation cannot be easily done
without larger changes that don't fit into RF 2.5 scope. We thus probably
need to deprecate this in 2.6 and remove fully in 2.7.


EDIT: Deprecation wasn't easy in RF 2.9 where this bug finally was fixed and we decided not to do it. We can reconsider that if there are lot of complaints during the preview releases.

@spooning

This comment has been minimized.

Copy link
Contributor Author

spooning commented Jun 29, 2014

Originally submitted to Google Code by @pekkaklarck on 14 Apr 2010

See this discussion for an example of confusion caused by this behavior:
http://groups.google.com/group/robotframework-users/browse_thread/thread/f610fd0a9133fcd6

@spooning spooning added this to the 2.9 milestone Jun 29, 2014

@spooning

This comment has been minimized.

Copy link
Contributor Author

spooning commented Jun 29, 2014

Originally submitted to Google Code by cmalu... on 16 Apr 2010

Maybe, It will be useful if you can run Robot with a parameter for avoid the
"Variable leaking" for a while.
Something like: pybot -l test

@spooning

This comment has been minimized.

Copy link
Contributor Author

spooning commented Jun 29, 2014

Originally submitted to Google Code by @pekkaklarck on 16 Apr 2010

I think a good plan how to resolve variables in RF 2.6 is this:

  1. Don't put variables inside a test or keyword into the scope of the lower level
    keyword.
  2. Don't put variables in test, suite, or global level there either.
  3. When resolving variables, first look the local scope.
  4. If there is no variable in the local scope, look the scopes of the higher level
    keywords and test.
  5. If var is found from higher level scopes, use it but show a deprecation warning.
  6. If var still not found, look for it from test, suite, and global scopes in that order.
  7. If no var found, resolving failed.

In RF 2.7 we can then skip steps 4) and 5) altogether.

@spooning

This comment has been minimized.

Copy link
Contributor Author

spooning commented Jun 29, 2014

Originally submitted to Google Code by @pekkaklarck on 28 Jun 2010

Issue 579 has been merged into this issue.

@spooning

This comment has been minimized.

Copy link
Contributor Author

spooning commented Jun 29, 2014

Originally submitted to Google Code by @spooning on 2 Dec 2011

Initially descoped from 2.7 due to lack of time

@spooning

This comment has been minimized.

Copy link
Contributor Author

spooning commented Jun 29, 2014

Originally submitted to Google Code by kormbrek on 19 Nov 2012

This can lead to strange behavior if the lower keyword relies on "Using list variables as scalar variables" functionality.
For example:
If higher level keyword defines local ${items}, lower level keyword defines local @⁠{items}, and then lower level keyword tries to use the list as a scaler - ${items}, it has the value set in the higher keyword and the value is not related to @⁠{items}.

@edbrannin

This comment has been minimized.

Copy link
Contributor

edbrannin commented Feb 27, 2015

Thankfully, the previous comment's concern should be moot after #1905 is complete.

jussimalinen added a commit that referenced this issue Jun 11, 2015

Variables do not leak to lower level. #532
- Implementation and tests ought to be ready.
- VariableScopes needs a new home and cleanup before this issue
  can be considered done.
- Running user keywords should be handled by a separate runner,
  but that's not part of this issue. Hopefully before 2.9 anyway.

@jussimalinen jussimalinen self-assigned this Jun 11, 2015

pekkaklarck added a commit that referenced this issue Jun 14, 2015

Cleaned up variable scopes. Related to #532.
No more GLOBAL_VARIABLES!!
@pekkaklarck

This comment has been minimized.

Copy link
Member

pekkaklarck commented Jun 14, 2015

5ce6db7 implemented this and handling variable scopes was cleaned up in 87f0ce1 and 0078f2f. Still need to look what User Guide says about variable scopes but otherwise this issue ought to be done.

pekkaklarck added a commit that referenced this issue Jun 15, 2015

User Guide: Enhanced variable scope docs.
Especially enhanced docs about local variable scope after fixing #532.
@pekkaklarck

This comment has been minimized.

Copy link
Member

pekkaklarck commented Jun 15, 2015

Enhanced documentation in 2c1917b. Could you @jussimalinen review and close this issue when done?

Also noticed that the original description said we cannot change the behavior without a deprecation period. Deprecation would have been so much work that our plan is to "just do it". We can still reconsider this if there are enough complaints during preview releases.

@jussimalinen

This comment has been minimized.

Copy link
Member

jussimalinen commented Jun 15, 2015

Looks good. Lets hope this wont break too much old tests...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.