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

Get wrong browser object for non top view in hierarchy #132

Open
oshtaier opened this issue Dec 18, 2018 · 7 comments
Open

Get wrong browser object for non top view in hierarchy #132

oshtaier opened this issue Dec 18, 2018 · 7 comments
Assignees
Labels

Comments

@oshtaier
Copy link

What we do:

class AirgunBrowser(Browser):
    def wait_for_element(...):
        return super(AirgunBrowser, self).wait_for_element(...)

Then we do:

class MyView(View):
    def is_displayed(self):
        return self.browser.wait_for_element(...)

class MyOtherView(View):
    class MyTab(Tab):
        view = MyView()

In result when we call view we have wrong browser object type in its is_displayed method:
self.browser == BrowserParentWrapper not AirgunBrowser that will get us to
super(type, obj): obj must be an instance or subtype of type exception in wait_for_element method

@abalakh
Copy link

abalakh commented Dec 18, 2018

So basically we want to extend wait_for_element with one extra argument. To avoid copy pasting entire method body we just call super() and extend result with our stuff.

For top-level views calls to wait_for_element() via self.browser.wait_for_element() work ok, but for nested structure like the one @oshtaier provided - it fails on super() call (obj must be an instance or subtype of type), as self.browser for all nested views is BrowserParentWrapper, not the Browser itself.

Using it through self.root_browser.wait_for_element() works, but that doesn't sound like a right option to use root browser for every overwritten method to me.
Is that expected behavior and we aren't supposed to extend any browser's method or that's an issue with BrowserParentWrapper proxy implementation?

@RonnyPfannschmidt
Copy link
Collaborator

RonnyPfannschmidt commented Dec 18, 2018

def __getattr__(self, attr):
"""Route all other attribute requests into the parent object's browser. Black magic included
Here is the explanation:
If you call ``.elements`` on this object directly, it will correctly inject the parent
locator. But if you call eg. ``element``, what will happen is that it will invoke the
original method from underlying browser and that method's ``self`` is the underlying browser
and not this wrapper. Therefore ``element`` would call the original ``elements`` without
injecting the parent.
What this getter does is that if you pull out a method, it detects that, unbinds the
pure function and rebinds it to this wrapper. The method that came from the browser object
is now executed not against the browser, but against this wrapper, enabling us to intercept
every single ``elements`` call.
"""
value = getattr(self._browser, attr)
if inspect.ismethod(value):
function = six.get_method_function(value)
# Bind the function like it was defined on this class
value = function.__get__(self, BrowserParentWrapper)
return value
handles the locator mapping, it seems its not yet correctly supporting subclassing and needs to be extended in some way

@RonnyPfannschmidt
Copy link
Collaborator

this change may necessitate a structural shift for wrapping browsers and locator using objects to sort out,
its possible we cant get around it before the new year

right now i don't have a idea for a solution

@RonnyPfannschmidt
Copy link
Collaborator

please also show the use-case you want to implement, due the nature of the issue (structural) it may be easier in the short term to find a different way to implement the use-case simply to avoid having to sort out the underlying issue in a timely manner

@oshtaier
Copy link
Author

@RonnyPfannschmidt actually that was real use case.

I did it here:
https://github.com/SatelliteQE/airgun/pull/263/files#diff-b5de464875aa71a973dee2e139a855c9R130

but teammates said proper comment that it will be necessary to fix all (>50) places where we have not top level view.

@mshriver mshriver added the bug label Dec 18, 2018
@mfalesni
Copy link
Contributor

@RonnyPfannschmidt Perhaps a custom super wrapper that "unwraps" the underlying browser from its wrapping would do. Instead of super(MyBrowser, self) one would call something like self.super(MyBrowser) and it would:

  1. Figure the unwrapped object
  2. call super(...) with it
  3. Rewrap it again with the current wrapper.

And then the use would be like with normal super.

@mfalesni
Copy link
Contributor

mfalesni commented Jan 3, 2019

Actually not. The base Browser was designed as an immutable set of operations which other components rely on.

If specific things are needed, you can:

  • add new methods that implement specific things in your browser subclass
  • request a backwards-compatible change in WT browser method (add a parameter or whatever).
  • use an existing hook in plugin class
  • add a new hook into the Plugin class and add the hook calls in the particular browser methods, that way extensions altering the behaviour of the browser can be done without harming vanilla browser's behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants