Selenium2Library new architecture by aaltat · Pull Request #770 · robotframework/SeleniumLibrary

aaltat · 2017-03-07T07:57:04Z

This PR removes the library multi inheritance madness. This change result cleaner code base, which eases the maintenance and hopefully eases accepting the new enhancement to the library.

The PR also removes the meta classes and the automatic keyword decoration in the keywordgroup object. Also the PR introduces a new context object(which is the library itself). The context object holds information from the different WebDriver instances and allows methods to register and fetch the different WebDriver instances.

Not ready for production Remember to take the https://github.com/robotframework/PythonLibCore

aaltat · 2017-03-07T08:10:35Z

This PR is not yet ready to merged, it has two unit test to be fixed and half of the acceptance test do fail. But I want to store the code also somewhere else than in my laptop and I want other also to take a look and give their input on the matter.

pekkaklarck · 2017-03-07T08:19:31Z

+
+    def _log(self, message, level='INFO', html=False):
+        level = level.upper()
+        if (level == 'INFO'):


Parens aren't needed! Python ain't Java! =)

Copy paste from the old library... Will fix it.

pekkaklarck · 2017-03-07T08:21:08Z

@@ -1,46 +1,58 @@
-from .keywordgroup import KeywordGroup
+from Selenium2Library.robotlibcore import keyword


I think it would be better to use from .robotlibrarycore import keyword here. Makes it easier to change the library name if/when we decide to do it.

pekkaklarck · 2017-03-07T08:25:35Z

+            print 'run failure here'
+            raise e
+
+    def current_browser(self):


This should be a property and possibly named simply browser. Makes usage from library components nicer when you can say context.browser.something() and not context.current_browser().something(). If the old architecture has this method, we should leave it for backwards compatibility and consider deprecating it.

A hundred times yes. Please leave _current_browser() for a release cycle as it will break a whole lot of outside code. I suggest emitting a warning every time it is invoked. Or perhaps only once.
There are many aspects to Selenium S2L does not expose so a lot of people depend on this private method. In the mean time you can have _current_browser() call browser or current_browser().

I also agree it would be better suited as an attribute rather than a method.

Thank you from reminding this one. We will create a _current_browser() for backwards compatibility. And most likely we must also create _cache.register() for the same reason. Now it is cache.register()

pekkaklarck · 2017-03-07T08:27:08Z


-from .keywordgroup import KeywordGroup
+from Selenium2Library.base import Base
+from Selenium2Library.robotlibcore import keyword


Already commented once, but I think from .module import something is better than from Selenium2Library.module import something. The former has less noise and require no changes if/when we change the library name.

pekkaklarck

Looks great in general!

pekkaklarck · 2017-03-07T08:27:43Z

+class AlertKeywords(Base):

    __ACCEPT_ALERT = 'accept'
    __DISMISS_ALERT = 'dismiss'


This can nowadays be normal constants named just like ACCEPT_ALERT.

pekkaklarck · 2017-03-07T08:28:56Z

    def delete_all_cookies(self):
        """Deletes all cookies."""
-        self._current_browser().delete_all_cookies()
+        self.ctx.current_browser().delete_all_cookies()


Good example where ctx.browser.delete_all_cookies() would be cleaner than the current ctx.current_browser().delete_all_cookies().

pekkaklarck · 2017-03-07T08:34:56Z

-        trg_elem = self._element_find(target, True, True)
-        ActionChains(self._current_browser()).drag_and_drop(src_elem, trg_elem).perform()
+        src_elem = self.element_find(source, True, True)
+        trg_elem = self.element_find(target, True, True)


General comments not related to the architecture change:

element_find() clearly needs default values so that True, True doesn't need to be repeated.

In general Boolean arguments should be given in named format like something=True to make it easier to understand the calling code.

This module is rather big. Should consider splitting it into smaller pieces. Element finding logic could perhaps be moved into context.

Because the above aren't really related to the architecture change, it's probably better not to do it as part of this PR. Could perhaps just add some TODOs.

pekkaklarck · 2017-03-07T08:37:55Z

+    def __init__(self, ctx):
+        Base.__init__(self)
+        self.ctx = ctx
+        self.element_find = ElementKeywords(ctx).element_find


This works, but moving the element finding logic into the context or into the base class would probably be cleaner.

pekkaklarck · 2017-03-07T08:41:54Z

One missing change is removing the decorator module dependency from setup.py. =)

AlexanderWhitestone · 2017-03-07T14:02:21Z



-class CookieKeywords(KeywordGroup):
+class CookieKeywords(object):


Just for my own understanding:
Why does this class inherit from object, but many other keyword classes inherit from Base?
I see that it isn't necessary since there is no logging for these keywords. I just want to understand the principal behind the choice.
Is it simplicity over uniformity?

Good point. I'd say all lib components should extend same base class. Consistency FTW!

Good point, consistency rules in this case

Thanks, glad to help.

aaltat · 2017-03-07T16:07:20Z

I think I will fix the unit test next. The failing unit test needs some thinking because the things that they are also testing are not anymore there. Perhaps I need refactor those parts out to a separate unit test for the browser cache.

ElementKeywords did contains methods which where used in many other keyword classes. Moved those methods to the Base class.

aaltat · 2017-03-16T22:13:40Z

Only 4 acceptance test fails. Not far to go.

aaltat · 2017-03-28T20:26:26Z

Run on failure fixed. Not totally sure is the this https://github.com/robotframework/Selenium2Library/pull/770/files#diff-d9fdca8cec320726d945c028908fd190R213 best approach, but works nicely. Three more failures to be fixed and then I should refactor the elementfinder.py

aaltat · 2017-03-28T20:36:29Z

Also I do not quite understand why this if is here: https://github.com/robotframework/Selenium2Library/blob/master/src/Selenium2Library/keywords/runonfailure.py#L64

aaltat · 2017-03-30T20:15:33Z

All acceptance test now pass. Now it is easier to refactor elementfinder.py

aaltat · 2017-04-01T18:41:10Z

This PR is ready for review, it would be good if someone would have time read the code and make sure that changed are good ones. And I know that this PR is quite huge one and not in average sized PR.

I also think that the elementfinder.py refactoring should be done in a separate PR, because this already is a huge PR.

pekkaklarck

Looks very good in general!

The only serious issue I noticed is how Base has instance attributes that I doubt should be per instance. See the comment in the class itself for details.

As a style issue I really don't like the idea to split function calls that cause > 78 char lines like

result = some_function_call(
    "All args here", "but some would fit prev line"
)

There are places where this enhances readability, but in the above case we just get one extra line and confusing single ). Just splitting the offending arguments into a separate line is, in my opinion, a lot better general guideline:

result = some_function_call("Those args that fit are here",
                            "remaining on second line")

Splitting all arguments to own line gets especially confusing in nested function calls and with if/for constructs.

pekkaklarck · 2017-04-18T08:43:41Z

-from .keywords import *
+import warnings
+
+from .utils import BrowserCache


Alphabetical ordering of imports is recommended by PEP-8. This particular import should be combined with the other from .utils import line a little lower.

Done and checked imports in all changed files.

pekkaklarck · 2017-04-18T08:46:11Z

+        try:
+            return DynamicCore.run_keyword(self, name, args, kwargs)
+        except Exception:
+            RunOnFailureKeywords(self)._run_on_failure()


Using _private method is ugly. Is this still work-in-progress? In that case adding TODO would be OK at this phase.

pekkaklarck · 2017-04-18T08:49:43Z

+        self.element_finder = ElementFinder()
+        self._speed_in_secs = 0.0
+        self._timeout_in_secs = 5.0
+        self._implicit_wait_in_secs = 5.0


Is the plan that different instances can have different values for these times? If not, then they shouldn't be instance attributes. This is important to check!

The _secs postfix sounds a little unnecessary too, but I guess it could help separating these from possible millisecond values.

No all instances must have the same value, that was the way it worked before the refactoring. Those timers where in https://github.com/robotframework/Selenium2Library/blob/master/src/Selenium2Library/keywords/browsermanagement.py#L45 but I moved them to Base. Other place that I was thinking to put them was the library __init__.py but can not anymore recall why I did actually put the timers here. Changed to not the instance specific but perhaps __init__ would be better place after all.

pekkaklarck · 2017-04-18T08:52:55Z

-from .keywordgroup import KeywordGroup
-
+from Selenium2Library.base import Base
+from Selenium2Library.robotlibcore import keyword


Should we use ..base instead of Selenium2Library.base? Robot uses the latter but the former would make the library name change easier. This isn't a big deal in general and definitely not in the context of this PR.

Lets decide this one when we decide should we rename the library.

pekkaklarck · 2017-04-18T08:57:39Z

+                'Closing browser with session id {}'.format(
+                    self.browsers.current.session_id
+                )
+            )


This is a bit verbose coding style, especially for something like debugging. Instead of using 5 lines I'd write it on 2, for example, like this:

self.debug('Closing browser with session id {}' .format(self.browsers.current.session_id))

As an unrelated note, I'd prefer proper sentences with . at the end. Again, not part of this PR.

pekkaklarck · 2017-04-18T09:02:20Z


-
-    def _make_ff(self , remote , desired_capabilites , profile_dir):
+    def _make_ff(self, remote, desired_capabilites, profile_dir):


+1 for removing unnecessary and PEP-8 incompatible whitespace. Separate commit in master would have been better, though. I do this all the time too and I wouldn't revert these changes.

pekkaklarck · 2017-04-18T09:08:14Z

-        self._info("Verifying element '%s' does not contain text '%s'."
+        self.info(
+            "Verifying element '%s' does not contain text '%s'."
                   % (locator, expected))


Inconsistent indentation. Why not to have info("Verifying ... like earlier?

pekkaklarck · 2017-04-18T09:12:24Z

-        actual_locator_count = len(self._element_find(locator, False, False))
+        actual_locator_count = len(self.element_find(
+            locator, first_only=False, required=False)
+        )


This is really hard to read. I'd use

actual_locator_count = len(self.element_find(locator, first_only=False, required=False))

or

elements = self.element_find(locator, first_only=False, required=False) actual_locator_count = len(elements)

instead. Shortening the local actual_locator_count variable to just count would probably be a good idea too. Everything might be nicely < 80 chars after that.

pekkaklarck · 2017-04-18T09:18:52Z

-    # Public, attributes
+        self.page_not_contains_element(
+            locator, message=message, loglevel=loglevel
+        )


This is the last time I mention this, but I don't think it is generally a good idea to move all arguments into separate row if having them in the same row as the method call makes the line > 80. It works nicely in some cases, especially if arguments themselves are long, but in general I consider it a lot better to split just the offending arguments to own row:

self.page_not_contains_element(locator, message=message, loglevel=loglevel)

pekkaklarck · 2017-04-18T09:25:35Z

+        if not element:
+            element = self.element_find(
+                locator, required=False, tag='file upload'
+            )


This is a good example of the thing I promised not to mention again. =)

Also changes run_on_failure to public method

aaltat · 2017-04-25T20:22:50Z

Í agree on the syntax comments but did not have time to fix those today. Hopefully tomorrow or next day.

I am not totally satisfied on the solution for the timers, in Base, perhaps they should be moved to library __init__?

pekkaklarck · 2017-04-25T21:30:07Z

Having config values in the library itself, i.e. in the context object, sounds good to me. If there are lot of them, we could create a separate class that has them all that the library then uses internally.

aaltat · 2017-04-26T05:48:01Z

There are not many parameters which would be needed globally. There are those three timers and is the run on failure already running, so 4 in total.

pekkaklarck · 2017-04-26T06:01:42Z

Do we still need a flag to tell is run-on-failure running?

aaltat · 2017-04-26T06:36:40Z

I think we need. Because run on failure can run any keyword and therefore it does this BuiltIn().run_keyword(self.ctx._run_on_failure_keyword). If the run on failure keyword is Selenium2Librarykeyword and it fails too, it causes recursive loop.

There might be other solutions to solve the recursive loop problem, but this is the most backwards compatible way.

aaltat · 2017-04-26T20:24:33Z

Fixed those too verbose coding styles, which I did spot. Also did little bit drive by refactoring and adjusted some spacing issues in the code which I have not touched.

pekkaklarck · 2017-04-27T05:21:29Z

Forgot that run-on-failure is implemented using BuiltIn.run_keyword. You are right that it often ends up calling S2L keywords recursively.

aaltat · 2017-04-28T05:25:14Z

It was pretty late when I made the last commit and therefore I might have missed some styling problems. I need to reread the changes one more time so that all is good to go. I will try to do it today or tomorrow. Is there something else that we would need to consider?

pekkaklarck · 2017-05-02T13:05:43Z

Haven't looked at this PR since my full review, but I think this is good to be merged in general. We can fine tune the code also after that.

Just need to decide is there so big need for new minor release with better Firefox support that this should wait. Creating a branch for the minor release afterwards is possible too, though.

aaltat · 2017-05-02T13:59:36Z

That is very good question. I am starting to think that doing Python 3 is more important because one can use the Create Webdriver keyword to get Firefox working. And because workaround exist, putting effort to release the Python 3 support feels more important.

pekkaklarck · 2017-05-02T19:51:56Z

I trust your judgement on this.

aaltat · 2017-05-03T15:54:28Z

There is always possibly to make the branch and release from there. Merged to master.

vairamsvsjdo · 2017-05-06T23:28:32Z

@aaltat Thanks for this drive to version 3 support.

Tatu Aalto and others added 2 commits March 5, 2017 23:57

POC for new architecture

6e84c52

Not ready for production Remember to take the https://github.com/robotframework/PythonLibCore

Fixing keyword classes to use new architecture

0edb107

pekkaklarck reviewed Mar 7, 2017

View reviewed changes

pekkaklarck requested changes Mar 7, 2017

View reviewed changes

AlexanderWhitestone reviewed Mar 7, 2017

View reviewed changes

aaltat added 5 commits March 8, 2017 23:39

Fixed unit tests for the new architecture

e109dd6

Fixed bug caused by the change.

c0f43a0

Fixed logging source

f1edd9f

Fixes for page should* and close browser keywords

44c5ab9

Refactoring common methods to the Base class

791d650

ElementKeywords did contains methods which where used in many other keyword classes. Moved those methods to the Base class.

Refactored Base to contain getter for browser and browsers

98ee06a

aaltat mentioned this pull request Mar 27, 2017

Removes unused import from browsermanagement.py #765

Closed

Fixes run on failure

257ba6b

Fixes waiting keywords

18eb9b3

Added TODO's to mark fixing in next PR

71cfe94

pekkaklarck mentioned this pull request Apr 12, 2017

Test with Python 3.6 instead of Python 3.3 #792

Closed

pekkaklarck requested changes Apr 18, 2017

View reviewed changes

This was referenced Apr 19, 2017

Create better architecture for Selenium2Library #771

Closed

unexpected Value Error . #798

Closed

aaltat added 2 commits April 25, 2017 22:46

Fixes imports to follow PEP8

1b9d717

Also changes run_on_failure to public method

Changed timer not to depend on the instance

0e0a08e

aaltat added 2 commits April 26, 2017 22:36

Moved timeouts to library __init__

5a3521e

Fixes too verbose coding style

3ec0cce

More code formatting to less verbose format

55c6137

pekkaklarck approved these changes May 2, 2017

View reviewed changes

aaltat merged commit 798fb31 into robotframework:master May 3, 2017

aaltat deleted the new_architecture branch May 9, 2017 19:37

AlexanderWhitestone mentioned this pull request May 10, 2017

Pulling improvements into Selenium2Library ASKIDA/Selenium2LibraryExtension#1

Open

		@@ -1,46 +1,58 @@
		from .keywordgroup import KeywordGroup
		from Selenium2Library.robotlibcore import keyword



		class CookieKeywords(KeywordGroup):
		class CookieKeywords(object):



		def _make_ff(self , remote , desired_capabilites , profile_dir):
		def _make_ff(self, remote, desired_capabilites, profile_dir):

Conversation

aaltat commented Mar 7, 2017

Uh oh!

aaltat commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaltat commented Mar 7, 2017

Uh oh!

aaltat commented Mar 16, 2017

Uh oh!

aaltat commented Mar 28, 2017

Uh oh!

aaltat commented Mar 28, 2017

Uh oh!

aaltat commented Mar 30, 2017

Uh oh!

aaltat commented Apr 1, 2017

Uh oh!

pekkaklarck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck left a comment •

edited

Loading

pekkaklarck commented Apr 25, 2017 •

edited

Loading