Incompatibility with forthcoming RF 2.9

[pekkaklarck]

I noticed earlier that Se2Lib wasn't compatible with RF 2.9 and reported it as robotframework/SeleniumLibrary#429. That problem was fixed, but now @rama-bornfree has commented that there are other problems. Based on the traceback and a quick look at the code, it seems that the issue is actually in this library.

I hope you have a change to analyze what's causing you problems. We have rafactored lot of code during 2.9 development, and, for example, variables have been pretty much rewritten. Please let us know ASAP if we've done something that is causing you lot of problems. We try to create RF 2.9 rc 1 this week, and have still a possibility to fix backwards incompatibilities.

We'd also like to hear if there are APIs that you'd like to be declared stable, APIs that could be improved, etc.

[zephraph]

(Copy pasta'd from the issue I was making before @pekkaklarck ninja'd me 😄)

@rama-bornfree originally reported in S2L that pageobjects was failing with RF 2.9b2.

RF 2.9a3 refactored a lot of the robot.variables module and I assume init_global_variables was removed as a part of that change. The stack trace @rama-bornfree gave us referenced that. I've found it here and here.

Really all I think you need to do is replace those references with something like a BuiltIn call like I did for S2L#431.

Hope this helps!

[hellmanj]

Thanks @pekkaklarck and @zephraph, we certainly do want to be 2.9 compatible! @frishberg, I tried a quick fix for this in a2de8c9 but there's one failed test in CI https://travis-ci.org/ncbi/robotframework-pageobjects/builds/70304804 which looks to be an out-of-robot issue. Any ideas?

[dpsfrishberg]

I'll take a look...

[dpsfrishberg]

Hmm. For me, robot.variables.scopes doesn't exist, so I can't run any tests in your branch.

[zephraph]

yeah, robot.variables is a private api and they made some big changes in it to fix how variable scoping works.

[pekkaklarck]

Yes, variable related code was pretty much re-written to support dict variables, fix variable scopes, avoid global state, etc. Hopefully changes didn't break too much external code too badly. We can consider changes to make external usage easier still before 2.9 is released.

[hellmanj]

@frishberg, you have to install the development version of robotframework to test the change:

pip install --upgrade --pre robotframework

[dpsfrishberg]

Thanks. Now I see. Yeah, this is tricky. To decide where to output screenshots, Selenium2Library's capture_page_screenshot calls _get_log_dir:https://github.com/rtomac/robotframework-selenium2library/blob/v1.7.0/src/Selenium2Library/keywords/_logging.py#L15
_get_log_dir used to check this GLOBAL_VARIABLES list, which we were populating in https://github.com/ncbi/robotframework-pageobjects/blob/master/robotpageobjects/context.py#L73 by calling https://github.com/robotframework/robotframework/blob/2.8.7/src/robot/variables/__init__.py#L36.

It appears that now, to retrieve the path to which to log screenshots, Selenium2Library calls BuiltIn().get_variables() (https://github.com/rtomac/robotframework-selenium2library/blob/master/src/Selenium2Library/keywords/_logging.py#L15), which requires that an execution context exist.

In short, we were relying on code in Selenium2Library that was accidentally not relying on executing in Robot. Now that that code uses the BuiltIn() library, we can't assume that it will work outside Robot anymore. Perhaps we could modify or monkey-patch BuiltIn so that it can retrieve variables outside Robot. What do you think, Jared?

On Sat, Jul 11, 2015 at 10:11 PM, Jared Hellman notifications@github.com
wrote:

@frishberg https://github.com/frishberg, you have to install the
development version of robotframework to test the change:

pip install --upgrade --pre robotframework

—
Reply to this email directly or view it on GitHub
#54 (comment)
.

[hellmanj]

Do we really need to reference robot variables at all (except for S2L screenshot paths) when outside robot? I tried removing this code altogether in b91121d and got the same test results. So could we just patch either _get_log_dir or _get_screenshot_paths ?

[zephraph]

If there's anything I can do on S2L to make this easier, just let me know.

[dpsfrishberg]

It would be better if we didn't refer to robot variables, but it's the S2L code that's making the reference. Yes, we could override _get_log_dir or _get_screenshot_paths`. The code you removed in b91121d was actually there in the first place to make code that refers to Robot's global variables think that it was running in Robot, so it's a workaround. Perhaps there is some way of improving that, but I don't see how other than overriding the aforementioned S2L method(s).

I am happy to get started on that change. It might make things easier for us to make the variable references in S2L more generic (i.e. not assume Robot), if @zephraph and others are open to the kind of decoupling we are trying to do.

[hellmanj]

I doubt decoupling was ever meant to be in scope for S2L so I wouldn't want to impose that job there. I think it would be simplest to override _get_log_dir to check the context, and when out of robot, just use the cwd (as we currently do with po_log.txt).

[pekkaklarck]

Overriding _get_log_dir ought to work, but because that's a private method it may change in the future. To prevent such problems, it would thus be best to have a designed way to configure this behavior in S2L.

@zephraph, could S2L be changed so that if getting the log directory from Robot fails due to RobotNotRunningError exception, it would return a predefined default log directory? This default could be passed to __init__ or be a public (and documented) class attribute that extending classes can set. Its default value could be just ..

Notice that S2L could also use Robot's BuiltIn.get_variable_value to resolve a single variable instead of getting all variables using BuiltIn.get_variables. Both of these currently raise RobotNotRunningError if Robot isn't running, but I could easily change the former to return the given default value instead. I'm not sure would that help too much, though, because caller can already now easily catch that exception and handle the situation as needed

[zephraph]

I can add an argument to S2L like screenshot_root_directory which will default to None. If the RobotNotRunningError occurs then it'll just use the current directly.

Sound acceptable?