issue #463 by Gaurang033 · Pull Request #724 · robotframework/SeleniumLibrary

Gaurang033 · 2016-12-30T11:58:31Z

…ssue #463

table_cell_should_not_contain
table_column_should_not_contain
table_footer_should_not_contain
table_header_should_not_contain
table_row_should_not_contain
table_should_not_contain

…ssue robotframework#463 table_cell_should_not_contain table_column_should_not_contain table_footer_should_not_contain table_header_should_not_contain table_row_should_not_contain table_should_not_contain

aaltat

In practice code in new keywords looks quite OK. There are some updates needed, but they do not need big refactoring. I did not read acceptance test, but I will read those in near future.

aaltat · 2017-01-03T22:06:11Z

            self.log_source(loglevel)
            raise AssertionError(message)

+    def table_cell_should_not_contain(self, table_locator, row, column, expected, loglevel='INFO'):


For all the new code, try to follow pep8 maximum line length: https://www.python.org/dev/peps/pep-0008/#maximum-line-length. Fix in all places where it is exceeded in code which you have written. You do not need fix the existing code.

Perhaps expected is not good argument name, because we expect that string is not there. Could not_contain or value be better argument names.

I have kept all the arguments name same as other functions in the file to keep the consistency, if like I can change that.

Please change only argument name only for new keywords. Changing the old ones would be backwards incompatible change and I am reluctant to do so.

aaltat · 2017-01-03T22:15:45Z

+        except AssertionError, err:
+            self._info(err)
+            self.log_source(loglevel)
+            raise AssertionError(message)


The exception text is not correct. It should say that:
Cell in table '%s' in row #%s and column #%s was not found

But the text is correct for the exception in line 105.

aaltat · 2017-01-03T22:20:18Z

+                   % (table_locator, row, column, expected))
+        try:
+            content = self.get_table_cell(table_locator, row, column, loglevel='NONE')
+        except AssertionError, err:


Write:
except AssertionError as err:
that is supported by Python 2 and Python 3

aaltat · 2017-01-03T22:21:04Z

+        `loglevel` argument.
+        """
+        element = self._table_element_finder.find_by_col(self._current_browser(), table_locator, col, expected)
+        if element is not None:


It is more Python way to write:
if not element:

aaltat · 2017-01-03T22:21:59Z

+            self.log_source(loglevel)
+            raise AssertionError("Column #%s in table identified by '%s' "
+                                 "should not have contained text '%s'."
+                   % (col, table_locator, expected))


The indentation is not following pep8.

aaltat · 2017-01-03T22:22:16Z

+            self.log_source(loglevel)
+            raise AssertionError(message)
+        self._info("Cell contains %s." % content)
+


Empty line is not needed.

aaltat · 2017-01-03T22:22:48Z

+        `loglevel` argument.
+        """
+        element = self._table_element_finder.find_by_footer(self._current_browser(), table_locator, expected)
+        if element is not None:


It is more Python way to write:
if not element:

aaltat · 2017-01-03T22:23:27Z

+        `loglevel` argument.
+        """
+        element = self._table_element_finder.find_by_header(self._current_browser(), table_locator, expected)
+        if element is not None:


It is more Python way to write:
if not element:

aaltat · 2017-01-03T22:23:38Z

+        See `Page Should Contain Element` for explanation about `loglevel` argument.
+        """
+        element = self._table_element_finder.find_by_row(self._current_browser(), table_locator, row, expected)
+        if element is not None:


It is more Python way to write:
if not element:

aaltat · 2017-01-03T22:23:48Z

+        `loglevel` argument.
+        """
+        element = self._table_element_finder.find_by_content(self._current_browser(), table_locator, expected)
+        if element is not None:


It is more Python way to write:
if not element:

aaltat · 2017-01-03T22:32:24Z

Also you can add your name to the https://github.com/robotframework/Selenium2Library/blob/master/BUILD.rst file if you want credit.

Gaurang033 · 2017-01-05T11:20:45Z

@aaltat I am not sure what you mean by the following comment.

Also you can add your name to the https://github.com/robotframework/Selenium2Library/blob/master/BUILD.rst file if you want credit.

aaltat · 2017-01-05T11:24:58Z

You can add, if you want, your name to the file and short description what you have done.

aaltat · 2017-01-05T11:27:33Z

Ah but the file was wrong, here the correct file https://github.com/robotframework/Selenium2Library/blob/master/CHANGES.rst

Gaurang033 · 2017-01-05T11:29:48Z

@aaltat That's what I was wondering. Should I mentioned the changes under 1.8.1 or create the new section 1.8.2 and mentioned under that?

aaltat · 2017-01-05T11:34:20Z

1.8.1 is the right one, it should actually say next release.

aaltat · 2017-01-11T10:47:49Z

Is that TODO comment valid one?

aaltat · 2017-01-11T11:17:50Z

Could you remove the comment then? This week is busy but I hope that I time in the weekend to read the code

aaltat

Could you remove the ToDo comment

aaltat · 2017-01-11T07:07:14Z

+                   % (table_locator, row, column, value))
+        cell_not_exist_message = ("Cell in table '%s' in row #%s and column #%s was not found"
+                                  % (table_locator, row, column))
+        # TODO new message as per comment


This line indicates that PR is not ready.

Could you remove the comment, because it looks invalid

aaltat · 2017-02-01T21:06:08Z

There are some comments for the test.

aaltat · 2017-02-01T20:15:49Z


+    def table_cell_should_not_contain(self, table_locator, row, column, value, loglevel='INFO'):
+        """
+        Verifies that a certain cell in a table does not contain `expected`.


Join lines 77 and 78 as a single line.

aaltat · 2017-02-01T20:18:37Z

            self.log_source(loglevel)
            raise AssertionError(message)

+    def table_cell_should_not_contain(self, table_locator, row, column, value, loglevel='INFO'):


Following PEP8[1] is required, fix the line length. Fix only in places where you did provide new code.

[1] https://www.python.org/dev/peps/pep-0008/

aaltat · 2017-02-01T20:41:56Z

+
+*** Test Cases ***
+Should Give Error Message When Content Found In Table
+    [Documentation]    Should Give Error Message When Content Found In Table


Do repeats what test name says and therefore it is not needed. Remove the documentation.

aaltat · 2017-02-01T20:43:36Z

+*** Keywords ***
+Verify Table Does Contain With CSS And XPath Locators
+    [Arguments]    ${locator}    ${value}
+    [Documentation]    Verify Table Contains With CSS And XPath Locators


Remove documentation because it repeats the keyword name.

aaltat · 2017-02-01T20:45:33Z

+
+
+*** Keywords ***
+Verify Table Does Contain With CSS And XPath Locators


Keyword does not have anything to do with the CSS or xpath locators. Better name for keyword could be: Table Should Not Contain keyword raises correct error when value is found from table.

aaltat · 2017-02-01T21:03:03Z

+Test Table Row Should Give Error When It Contains
+    [Documentation]    Test Table Row Should Give Error When It Contains
+    ${table}=    set variable    tableWithSingleHeader
+    ${value} =    set variable    tableWithSingleHeader_A1


Value is an exact match, right? Expand test partial text too. Also add test when value is in table header and footer.

aaltat · 2017-02-01T21:03:15Z

+    [Documentation]    Test Table column Should Not Contain
+    ${table}=    set variable    simpleTable
+    ${column}=    set variable    1
+    ${value} =    set variable    simpleTable_B2


Value is an exact match, right? Expand test partial text too. Also add test when value is in table header and footer.

aaltat · 2017-02-01T21:03:39Z

+    [Documentation]    Test Table column Should Give Error When It Contains
+    ${table}=    set variable    tableWithSingleHeader
+    ${column}=    set variable    1
+    ${value} =    set variable    tableWithSingleHeader_A1


Value is an exact match, right? Expand test partial text too. Also add test when value is in table header and footer.

The value is in the header, should I just rename the test case and add new where value is in footer? or modify this and add condition to verify when value is in footer.

Use a way that feels the best for your. Both options are fine.

aaltat · 2017-02-01T21:04:41Z

+    [Documentation]    Test Table Header Should Not Contain
+    ${table}=    set variable    tableWithSingleHeader
+    ${value} =    set variable    tableWithSingleHeader_A3
+    table header should not contain  ${table}    ${value}


Add test when value is table body and table footer but value is not in table header.

the value is in the body. Should I rename this test case to reflect that and add new test case to verify when value is in footer but not in header? or should I rename this test case to reflect both condition where value is in body and footer but not in header and add verification. I can't do that with single value as there is not table where same value is in footer and body but not in header. if you like I can create a new table.

Create test when value is not in header, body or footer, like with tidii or something.

Test when value is in body (the current test)

Create test when value is in footer.

These test can be combined to one test if it feels natural to write test in such way.

aaltat · 2017-02-01T21:05:19Z

+    [Documentation]    Test Table Footer Should Not Contain
+    ${table}=    set variable    withHeadAndFoot
+    ${expected} =    set variable    withHeadAndFoot_A1
+    table footer should not contain    ${table}    ${expected}


Add test where value is in table header and body.

The value withHeadAndFoot_A1 is already in the body, should i just add one more verification where value is not in header and rename the test? or should I rename this test to verify that value is in body but not in footer and then add another test to verify where value is in footer but not in header?

aaltat · 2019-04-22T19:43:52Z

Closing due inactivity.

Added following keywords and their acceptance test cases to fix the i…

2d35cd2

…ssue robotframework#463 table_cell_should_not_contain table_column_should_not_contain table_footer_should_not_contain table_header_should_not_contain table_row_should_not_contain table_should_not_contain

aaltat requested changes Jan 3, 2017

View reviewed changes

incorporated the code review comments

d9880bb

updated with new table keywords

8d4dfa3

aaltat requested changes Jan 29, 2017

View reviewed changes

removed the unnecessary TODO statement.

81fb55a

aaltat requested changes Feb 1, 2017

View reviewed changes

aaltat closed this Apr 22, 2019

emanlove mentioned this pull request Nov 5, 2022

Need "Table Should Not Contain" keyword #463

Closed



		* Keywords *
		Verify Table Does Contain With CSS And XPath Locators

Conversation

Gaurang033 commented Dec 30, 2016

Uh oh!

aaltat 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!

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!

aaltat commented Jan 3, 2017

Uh oh!

Gaurang033 commented Jan 5, 2017

Uh oh!

aaltat commented Jan 5, 2017

Uh oh!

aaltat commented Jan 5, 2017

Uh oh!

Gaurang033 commented Jan 5, 2017

Uh oh!

aaltat commented Jan 5, 2017

Uh oh!

aaltat commented Jan 11, 2017

Uh oh!

aaltat commented Jan 11, 2017

Uh oh!

aaltat 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!

aaltat commented Feb 1, 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!

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