Merge lp:~elopio/selenium-simple-test/print_elements into lp:selenium-simple-test

[Leo Arias]

Added _element_to_string to get a printable version of the element.

[lp:~elopio/selenium-simple-test/print_elements]

373. By Leo Arias on 2013-04-09

Improved the test with id.

[Vincent Ladeuil (vila)]

I don't think this is a good use of mock. It at least triggers my main reluctance about using it: the tests using mock in this proposal are not doubled with tests using the real code.

Can you try rewriting those tests ?

review: Needs Information

[lp:~elopio/selenium-simple-test/print_elements]

374. By Leo Arias on 2013-04-10

Reverted testproj.db.

375. By Leo Arias on 2013-04-10

added acceptance tests for element_to_string.

376. By Leo Arias on 2013-04-10

Improved the readability of the element_to_string unit tests.

[Leo Arias (elopio)]

<elopio> vila: please take a look at http://bazaar.launchpad.net/~elopio/selenium-simple-test/print_elements/revision/375
<vila> elopio: hehe, even shorter ! So you don't need the mock ones anymore right ? <evil grin>
<elopio> vila: I disagree with you there :)
<vila> elopio: I'm all ears
<elopio> vila: this tests are nice to have, but they don't test a unit of code, they are retesting things that are already tested in selenium, they depend on firefox to run, they are slower, and the set up for the test is done on an external file (index.html), thus are not as readable as the other ones.
<vila> elopio: ha, then don't write them this way
<elopio> vila: now I'm all ears ;)
<elopio> vila: the way to solve all those problems is with a unit test and mock. I agree there are some code paths not tested on the original
<elopio> for example, _get_text
<vila> elopio: your implementation could be more testable but it would mean to not rely as much on actions.py then to be able to test it with a simple hmtl snippet
<vila> elopio: didn't you implemented some helpers to get there without involving firefox ?
<elopio> vila: no, firefox is always needed. I tried with htmlunit, but it sucks.
<elopio> we could do something with phantomjs.
<elopio> that would make it faster than firefox, but still slower than the mocked unit test.
<vila> the mocked one is faster because it does less, but you don't get a clear idea of what 'less' means by just reading the tests, you need to know the implementation to write them which is... bad
<elopio> vila: I wrote them before implementing them.
<vila> elopio: knowing how the implementation will work as opposed to knowing only the API
<vila> elopio: you test actions._element_to_string but mock element.get_attribute
<vila> elopio: how do you know they are related ?
<elopio> knowing what the function does, not how it is implemented.
<elopio> it is a really low level function, so it's implementation is straight-forward.
<elopio> but the implementation could change, what the tests describe is what it does. When there's no id attribute, then get the text.
<elopio> lets say we have a way to test an html snipped without a huge delay
<elopio> then
<elopio> element = mock.Mock()
<elopio> element.get_attribute.return_value = None
<elopio> element.text = 'Test text'
<elopio> would be the same as
<elopio> element = '<p>Test text</p>'
<elopio> the latter is shorter and clearer, I agree.
<vila> elopio: no, the later is shor.. see ?
<vila> elopio: moreover, even after reading the mock tests thrice, I still don't get what they do, if I really want to understand, I need to look at the implementation
<vila> elopio: and I still don't understand why there is a if in test_element_with_id.mock.get_attribute but a single call...
<elopio> vila: maybe I can make them clearer, for example making a function called get_element_without_id, and that creates and return the mock.
<vila> elopio: you'll end up duplicating the implementation which is even worse ;)
<elopio> no, let me try.
<elopio> vila: here: http://bazaar.launchpad.net/~elopio/selenium-simple-test/print_elements/revision/376#src/sst/tests/test_actions.py
<elopio> once we have a beautifulsoup sele...

[Vincent Ladeuil (vila)]

review: Approve

=== modified file 'src/sst/actions.py'
--- src/sst/actions.py	2013-04-09 15:25:02 +0000
+++ src/sst/actions.py	2013-04-10 15:27:28 +0000
@@ -453,7 +453,7 @@
         _make_results_dir()
         browsermob_proxy.save_har(_make_useable_har_name())
 
-
+        
 def assert_checkbox(id_or_elem):
     """
     Assert that the element is a checkbox.
@@ -473,23 +473,54 @@
     or isn't a checkbox."""
     checkbox = assert_checkbox(id_or_elem)
     real = checkbox.is_selected()
-    msg = 'Checkbox: %r - Has Value: %r' % (_get_text(checkbox), real)
+    msg = 'Checkbox: %r - Has Value: %r' % (_element_to_string(checkbox), real)
     if real != value:
         _raise(msg)
 
 
+def _element_to_string(element):
+    element_id = element.get_attribute('id')
+    if element_id:
+        return element_id
+    else:
+        element_text = _get_text(element)
+        if element_text:
+            return element_text
+        else:
+            return element.get_attribute('outerHTML')
+
+
+def _get_text(elem):
+    text = None
+    try:
+        text = elem.text
+    except InvalidElementStateException:
+        pass
+    if text:
+        # Note that some elements (like textfields) return empty string
+        # for text and we still need to call value
+        return text
+    try:
+        text = elem.get_attribute('value')
+    except InvalidElementStateException:
+        pass
+    return text
+
+        
+
 def toggle_checkbox(id_or_elem):
     """
     Toggle the checkbox value. Takes an element id or object. Raises a failure
     exception if the element specified doesn't exist or isn't a checkbox."""
     checkbox = assert_checkbox(id_or_elem)
-    logger.debug('Toggling checkbox: %r' % _get_text(checkbox))
+    element_string = _element_to_string(checkbox)
+    logger.debug('Toggling checkbox: %r' % element_string)
     before = checkbox.is_selected()
     checkbox.click()
     after = checkbox.is_selected()
-    msg = 'Checkbox: %r - was not toggled, value remains: %r' \
-        % (_get_text(checkbox), before)
     if before == after:
+        msg = 'Checkbox: %r - was not toggled, value remains: %r' \
+            % (element_string, before)
         _raise(msg)
 
 
@@ -499,7 +530,8 @@
     exception if the element specified doesn't exist or isn't a checkbox."""
     checkbox = assert_checkbox(id_or_elem)
     logger.debug(
-        'Setting checkbox %r to %r' % (_get_text(checkbox), new_value))
+        'Setting checkbox %r to %r' % (_element_to_string(checkbox),
+                                       new_value))
     # There is no method to 'unset' a checkbox in the browser object
     current_value = checkbox.is_selected()
     if new_value != current_value:
@@ -526,7 +558,7 @@
     """
     key_element = _get_elem(id_or_elem)
     msg = 'Simulating keypress on %r with %r key' \
-        % (_get_text(key_element), key_to_press)
+        % (_element_to_string(key_element), key_to_press)
     logger.debug(msg)
     key_code = _make_keycode(key_to_press)
     key_element.send_keys(key_code)
@@ -558,7 +590,7 @@
     off by passing `clear=False`."""
     textfield = assert_textfield(id_or_elem)
     msg = 'Writing to textfield %r with text %r' \
-        % (_get_text(textfield), new_text)
+        % (_element_to_string(textfield), new_text)
     logger.debug(msg)
 
     # clear field like this, don't use clear()
@@ -576,7 +608,7 @@
     current_text = textfield.get_attribute('value')
     if current_text != new_text:
         msg = 'Textfield: %r - did not write. Text was: %r' \
-            % (_get_text(textfield), current_text)
+            % (_element_to_string(textfield), current_text)
         _raise(msg)
 
 
@@ -589,7 +621,7 @@
     link = _get_elem(id_or_elem)
     if link.tag_name != 'a':
         msg = 'The text %r is not part of a Link or a Link ID' \
-            % _get_text(link)
+            % _element_to_string(link)
         _raise(msg)
     return link
 
@@ -623,7 +655,7 @@
         _logger.debug('Capturing http traffic...')
         browsermob_proxy.new_har()
 
-    logger.debug('Clicking link %r' % _get_text(link))
+    logger.debug('Clicking link %r' % _element_to_string(link))
     link.click()
 
     if wait:
@@ -667,7 +699,7 @@
         logger.debug('Capturing http traffic...')
         browsermob_proxy.new_har()
 
-    logger.debug('Clicking element %r' % _get_text(elem))
+    logger.debug('Clicking element %r' % _element_to_string(elem))
     elem.click()
 
     if wait:
@@ -904,7 +936,8 @@
     """Set the select drop-list to a text or value specified."""
     elem = assert_dropdown(id_or_elem)
     logger.debug(
-        'Setting %r option list to %r' % (_get_text(elem), text or value))
+        'Setting %r option list to %r' % (_element_to_string(elem),
+                                          text or value))
     if text and not value:
         for element in elem.find_elements_by_tag_name('option'):
             if element.text == text:
@@ -958,7 +991,7 @@
     a radio button"""
     elem = assert_radio(id_or_elem)
     selected = elem.is_selected()
-    msg = 'Radio %r should be set to: %s.' % (_get_text(elem), value)
+    msg = 'Radio %r should be set to: %s.' % (_element_to_string(elem), value)
     if value != selected:
         _raise(msg)
 
@@ -966,27 +999,10 @@
 def set_radio_value(id_or_elem):
     """Select the specified radio button."""
     elem = assert_radio(id_or_elem)
-    logger.debug('Selecting radio button item %r' % _get_text(elem))
+    logger.debug('Selecting radio button item %r' % _element_to_string(elem))
     elem.click()
 
 
-def _get_text(elem):
-    text = None
-    try:
-        text = elem.text
-    except InvalidElementStateException:
-        pass
-    if text:
-        # Note that some elements (like textfields) return empty string
-        # for text and we still need to call value
-        return text
-    try:
-        text = elem.get_attribute('value')
-    except InvalidElementStateException:
-        pass
-    return text
-
-
 def assert_text(id_or_elem, text):
     """
     Assert the specified element text is as specified.
@@ -996,11 +1012,7 @@
     elem = _get_elem(id_or_elem)
     real = _get_text(elem)
     if real is None:
-        if isinstance(id_or_elem, str):
-            element_string = id_or_elem
-        else:
-            element_string = elem.get_attribute('outerHTML')
-        msg = 'Element %r has no text attribute' % element_string
+        msg = 'Element %r has no text attribute' % _element_to_string(elem)
         _raise(msg)
     if real != text:
         msg = 'Element text should be %r. It is %r.' % (text, real)
@@ -1015,7 +1027,7 @@
     elem = _get_elem(id_or_elem)
     real = _get_text(elem)
     if real is None:
-        msg = 'Element %r has no text attribute' % _get_text(elem)
+        msg = 'Element %r has no text attribute' % _element_to_string(elem)
         _raise(msg)
     msg = 'Element text is %r. Does not contain %r' % (real, text)
     if regex:
@@ -1173,7 +1185,7 @@
         logger.debug('Capturing http traffic...')
         browsermob_proxy.new_har()
 
-    logger.debug('Clicking button %r' % _get_text(button))
+    logger.debug('Clicking button %r' % _element_to_string(button))
     button.click()
 
     if wait:
@@ -1420,7 +1432,8 @@
     the attribute using a regular expression search.
     """
     elem = _get_elem(id_or_elem)
-    logger.debug('Checking attribute %r of %r' % (attribute, _get_text(elem)))
+    logger.debug(
+        'Checking attribute %r of %r' % (attribute,  _element_to_string(elem)))
     actual = elem.get_attribute(attribute)
     if not regex:
         success = value == actual
@@ -1442,7 +1455,7 @@
     elem = _get_elem(id_or_elem)
     logger.debug(
         'Checking css property %r: %r of %r' % (
-            property, value, _get_text(elem)))
+            property, value, _element_to_string(elem)))
     actual = elem.value_of_css_property(property)
     # some browsers return string with space padded commas, some don't.
     actual = actual.replace(', ', ',')

=== modified file 'src/sst/selftests/assert_text.py'
--- src/sst/selftests/assert_text.py	2013-04-09 15:25:02 +0000
+++ src/sst/selftests/assert_text.py	2013-04-10 15:27:28 +0000
@@ -14,22 +14,11 @@
 # Test wrong text.
 fails(assert_text, 'some_id', 'Wrong text')
 
-# Test no text with id.
+# Test no text.
 try:
     assert_text('element_without_text', '')
 except AssertionError as error:
     assert_equal(
-        "Element 'element_without_text' has no text attribute", error.message)
-else:
-    raise AssertionError('assert_text did not fail.')
-
-# Test no text with element.
-try:
-    assert_text(get_element(id='element_without_text'), '')
-except AssertionError as error:
-    expected_message = ("Element u\'<p id=\"element_without_text\"></p>\' "
-                        "has no text attribute")
-    assert_equal(
-        expected_message, error.message)
+        "Element u'element_without_text' has no text attribute", error.message)
 else:
     raise AssertionError('assert_text did not fail.')

=== added file 'src/sst/selftests/element_to_string.py'
--- src/sst/selftests/element_to_string.py	1970-01-01 00:00:00 +0000
+++ src/sst/selftests/element_to_string.py	2013-04-10 15:27:28 +0000
@@ -0,0 +1,19 @@
+import sst.actions
+
+
+sst.actions.go_to('/')
+sst.actions.assert_title('The Page Title')
+
+element_with_id = sst.actions.get_element(id='some_id')
+element_string = sst.actions._element_to_string(element_with_id)
+sst.actions.assert_equal('some_id', element_string)
+
+element_without_id_with_text = sst.actions.get_element(css_class='no_id')
+element_string = sst.actions._element_to_string(element_without_id_with_text)
+sst.actions.assert_equal('Element without id, with text.', element_string)
+
+element_without_id_without_text = sst.actions.get_element(
+    css_class='no_id_no_text')
+element_string = sst.actions._element_to_string(
+    element_without_id_without_text)
+sst.actions.assert_equal('<p class="no_id_no_text"></p>', element_string)

=== modified file 'src/sst/tests/test_actions.py'
--- src/sst/tests/test_actions.py	2013-02-11 19:03:51 +0000
+++ src/sst/tests/test_actions.py	2013-04-10 15:27:28 +0000
@@ -20,10 +20,9 @@
 from cStringIO import StringIO
 import logging
 
-
+import mock
 import testtools
 
-
 from sst import actions
 
 
@@ -64,3 +63,46 @@
     def test_wait_for_retries(self):
         self.assertRaisesOnlyOnce(None, actions.wait_for,
                                   self.raise_stale_element)
+
+
+class TestElementToString(testtools.TestCase):
+
+    def test_element_with_id(self):
+        element = self._get_element_with_id(element_id='Test id')
+        self.assertEqual('Test id', actions._element_to_string(element))
+
+    def _get_element_with_id(self, element_id):
+        element = mock.Mock()
+        def mock_get_attribute(attribute):
+            if attribute == 'id':
+                return element_id
+        element.get_attribute.side_effect = mock_get_attribute
+        return element
+
+    def test_element_without_id_with_text(self):
+        element = self._get_element_without_id_with_text(text='Test text')
+        self.assertEqual('Test text', actions._element_to_string(element))
+
+    def _get_element_without_id_with_text(self, text):
+        element = mock.Mock()
+        element.get_attribute.return_value = None
+        element.text = text
+        return element
+
+    def test_element_without_id_without_text(self):
+        element = self._get_element_without_id_without_text(tag='p')
+        self.assertEqual(
+            '<p></p>', actions._element_to_string(element))
+
+    def _get_element_without_id_without_text(self, tag):
+        element = mock.Mock()
+        def mock_get_attribute(attribute):
+            values = {
+                'id': None,
+                'value': None,
+                'outerHTML': '<{0}></{0}>'.format(tag)
+            }
+            return values[attribute]
+        element.get_attribute.side_effect = mock_get_attribute
+        element.text = None
+        return element

=== modified file 'src/testproject/templates/index.html'
--- src/testproject/templates/index.html	2013-04-09 02:23:36 +0000
+++ src/testproject/templates/index.html	2013-04-10 15:27:28 +0000
@@ -15,6 +15,8 @@
 
   <p class="unique_class" id="some_id">Some text here</p>
   <p id="element_without_text"></p>
+  <p class="no_id">Element without id, with text.</p>
+  <p class="no_id_no_text"></p>
   <p class="some_class">More text</p>
   <p class="some_class">And yet more</p>