Hi Dave Please find updated Feature test case and review.
This patch is dependent on RM_2400v2.patch
On Fri, May 26, 2017 at 8:45 PM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:
> Hi Joao,
>
> On May 26, 2017 8:33 PM, "Joao Pedro De Almeida Pereira" <
> jdealmeidapere...@pivotal.io> wrote:
> >
> > Hi Surinder,
> > You are right, nevertheless that was not the only error we had on the
> flaky tests.
> Okay, please send stack trace where test case fails..
>
> Thanks
> Surinder
>
> >
> > Thanks
> > Joao & Shruti
> >
> > On Fri, May 26, 2017 at 10:18 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
> >>
> >> Hi Joao,
> >>
> >> Please apply patch RM_2400v2.patch first then apply Feature test cases
> patch.
> >>
> >>
> >> On May 26, 2017 7:42 PM, "Joao Pedro De Almeida Pereira" <
> jdealmeidapere...@pivotal.io> wrote:
> >>>
> >>> Hello Surinder,
> >>>
> >>> Thanks for updating the patch. After looking into the updated version,
> we found the following issues.
> >>>
> >>>
> >>> The tests looked flaky. We ran the tests three times and each time we
> had timeout issues.
> >>> It failed twice in the location mentioned below. It also failed
> because the row1 cell2 values was [null] instead of the expected [default].
> >>>
> >>> Traceback (most recent call last): File "/Users/pivotal/workspace/
> pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py", line 105,
> in runTest self._copy_paste_row() File "/Users/pivotal/workspace/
> pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py", line 248,
> in _copy_paste_row self._compare_cell_value(row1_cell2_xpath,
> "[default]") File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/feature_
> tests/view_data_dml_queries.py", line 147, in _compare_cell_value
> CheckForViewDataTest.TIMEOUT_STRING File "/Users/pivotal/.pyenv/
> versions/pgadmin/lib/python2.7/site-packages/selenium/webdriver/support/wait.py",
> line 80, in until raise TimeoutException(message, screen, stacktrace)
> TimeoutException: Message: Timed out waiting for div element to appear
> >>>
> >>>
> >>>
> >>> We also noticed that the function _wait is no longer used. Maybe we
> can remove it.
>
Removed.
> >>>
> >>> Thanks
> >>> Joao & Shruti
> >>>
> >>> On Fri, May 26, 2017 at 9:07 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Please find updated patch.
> >>>>
> >>>> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
> >>>>>
> >>>>> Hello Surinder,
> >>>>>
> >>>>> We are having issues running the tests, this is the error that we
> are getting:
> >>>>>
> >>>>> File "/Users/pivotal/workspace/pgadmin4/web/regression/
> python_test_utils/test_utils.py", line 196, in create_table_with_query
> pg_cursor.execute(query) ProgrammingError: role "postgres" does not exist
> Traceback (most recent call last): File "/Users/pivotal/workspace/
> pgadmin4/web/regression/python_test_utils/test_utils.py", line 196, in
> create_table_with_query pg_cursor.execute(query) ProgrammingError: relation
> "defaults_id_seq" does not exist
> >>>>>
> >>>>>
> >>>>> Fixed.
> >>>>>
> >>>>> There is already a function that waits for an element to be
> displayed on the screen. The function is: self.page.find_by_xpath
> >>>>>
> >>>>> In line 179 and 180, both functions do the same thing, why do we
> need to wait first and then wait again. Were you experiencing flakiness?
> >>>>
> >>>> I have to use another instance of webDriverWait because I was getting
> TimeoutException. I guess timeout of 10 seconds wasn't enough to search the
> element in DOM.
> >>>>>
> >>>>>
> >>>>> Does _check_xss_in_view_data method checks for Cross Site Scripting?
> >>>>
> >>>> I forgot to change the method name.
> >>>>>
> >>>>>
> >>>>> Was there any reason to duplicate self.page._connects_to_server and
> self.page._close_query_tool?
> >>>>
> >>>> Fixed.
> >>>>
> >>>> I got following exception when I used "self.page._close_query_tool":
> >>>>>
> >>>>> Traceback (most recent call last):
> >>>>> File "/Users/surinder/Documents/Projects/pgadmin4/web/pgadmin/
> feature_tests/view_data_dml_queries.py", line 125, in runTest
> >>>>> self.page.close_query_tool()
> >>>>> File "/Users/surinder/Documents/Projects/pgadmin4/web/
> regression/feature_utils/pgadmin_page.py", line 66, in close_query_tool
> >>>>> self.driver.switch_to.frame(self.driver.find_elements_by_
> tag_name("iframe")[0])
> >>>>> File "/Users/surinder/Documents/Workspaces/PEM_35/lib/python3.
> 5/site-packages/selenium/webdriver/remote/switch_to.py", line 87, in frame
> >>>>> self._driver.execute(Command.SWITCH_TO_FRAME, {'id':
> frame_reference})
> >>>>> File "/Users/surinder/Documents/Workspaces/PEM_35/lib/python3.
> 5/site-packages/selenium/webdriver/remote/webdriver.py", line 238, in
> execute
> >>>>> self.error_handler.check_response(response)
> >>>>> File "/Users/surinder/Documents/Workspaces/PEM_35/lib/python3.
> 5/site-packages/selenium/webdriver/remote/errorhandler.py", line 193, in
> check_response
> >>>>> raise exception_class(message, screen, stacktrace)
> >>>>> selenium.common.exceptions.WebDriverException: Message: unknown
> error: Runtime.evaluate threw exception: Error: element is not attached to
> the page document
> >>>>> at Cache.retrieveItem (<anonymous>:173:17)
> >>>>> at unwrap (<anonymous>:293:20)
> >>>>> at unwrap (<anonymous>:297:19)
> >>>>> at callFunction (<anonymous>:343:29)
> >>>>> at apply.ELEMENT (<anonymous>:357:23)
> >>>>> at <anonymous>:358:3
> >>>>> (Session info: chrome=58.0.3029.110)
> >>>>> (Driver info: chromedriver=2.29.461585 (
> 0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.10.2
> x86_64)
> >>>>
> >>>>
> >>>> I replaced this method with the one I was using in the test file and
> It is working for every test case.
> >>>>>
> >>>>>
> >>>>> The method _verify_insert_data looks more or less the same code as
> in the end of _copy_paste_row, should this be merged?
> >>>>
> >>>> Yes, I have merged.
> >>>>
> >>>> Thanks
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Joao & Shruti
> >>>>>
> >>>>>
> >>>>> On Thu, May 25, 2017 at 4:41 PM, Dave Page <dp...@pgadmin.org>
> wrote:
> >>>>>>
> >>>>>> Hi
> >>>>>>
> >>>>>> The tests failed on both PG 9.4 and 9.6 for me :-(
> >>>>>>
> >>>>>> ============================================================
> ==========
> >>>>>> ERROR: runTest (pgadmin.feature_tests.view_data_dml_queries.
> CheckForViewDataTest)
> >>>>>> Validate Insert, Update operations in View data with given test data
> >>>>>> ------------------------------------------------------------
> ----------
> >>>>>> Traceback (most recent call last):
> >>>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/
> view_data_dml_queries.py",
> >>>>>> line 120, in runTest
> >>>>>> self._verify_insert_data()
> >>>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/
> view_data_dml_queries.py",
> >>>>>> line 316, in _verify_insert_data
> >>>>>> self._compare_cell_value(cell_xpath, config_data[str(idx)][1])
> >>>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/
> view_data_dml_queries.py",
> >>>>>> line 165, in _compare_cell_value
> >>>>>> "Timed out waiting for element to appear"
> >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/support/wait.py",
> >>>>>> line 80, in until
> >>>>>> raise TimeoutException(message, screen, stacktrace)
> >>>>>> TimeoutException: Message: Timed out waiting for element to appear
> >>>>
> >>>> I increases the timeout of webDriverWait from "0.3" to "0.8". It
> should fix this issue.
> >>>>>>
> >>>>>>
> >>>>>> Also, Can we replace the sleep with a "wait for object" or similar?
> >>>>
> >>>> I have removed sleep.
> >>>>>>
> >>>>>>
> >>>>>> On Thu, May 25, 2017 at 6:42 AM, Surinder Kumar
> >>>>>> <surinder.ku...@enterprisedb.com> wrote:
> >>>>>> > Hi
> >>>>>> >
> >>>>>> > Please find attached patch for Feature test cases.
> >>>>>> >
> >>>>>> > The approach is to create a single table 'defaults' with columns
> of various
> >>>>>> > types(number, text, json and boolean) and write test cases for
> these cell
> >>>>>> > types with different input values.
> >>>>>> >
> >>>>>> > Following are the test cases covered:
> >>>>>> >
> >>>>>> > 1) Add a new row, save and compare the resulted value with
> expected values
> >>>>>> >
> >>>>>> > 2) Copy/Paste row, save and compare cell data
> >>>>>> >
> >>>>>> > a) Clear cell value and escape, the cell must set to [default]
> >>>>>> >
> >>>>>> > 3) Update cell:
> >>>>>> >
> >>>>>> > a) Insert two single quotes(''), expected value is blank string
> >>>>>> >
> >>>>>> > b) Clear a cell, expected value is [null]
> >>>>>> >
> >>>>>> > c) Insert a string \’\’, expected value is ''
> >>>>>> >
> >>>>>> > d) Insert a string \\’\\’, expected value is \’\’
> >>>>>> >
> >>>>>> > e) Insert a string \\\\’\\\\’, expected value is \\’\\’
> >>>>>> >
> >>>>>> > f) If a checkbox cell is double clicked, return value must be
> 'true'
> >>>>>> >
> >>>>>> >
> >>>>>> > Thanks
> >>>>>> > Surinder Kumar
> >>>>>> >
> >>>>>> >
> >>>>>> >
> >>>>>> >
> >>>>>> >
> >>>>>> >
> >>>>>> > On Fri, May 19, 2017 at 6:08 PM, Surinder Kumar
> >>>>>> > <surinder.ku...@enterprisedb.com> wrote:
> >>>>>> >>
> >>>>>> >> Hi
> >>>>>> >>
> >>>>>> >> Please find updated patch and review.
> >>>>>> >>
> >>>>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
> >>>>>> >> <jdealmeidapere...@pivotal.io> wrote:
> >>>>>> >>>
> >>>>>> >>> Hello Hackers,
> >>>>>> >>>
> >>>>>> >>> We reviewed the PR, and there are a lot of new lines of code in
> this
> >>>>>> >>> patch, are we sure that we can have a good coverage of the
> functionality
> >>>>>> >>> just by doing feature tests?
> >>>>>> >>> Adding more code to a 3.5k+ lines file doesn't look like a good
> option,
> >>>>>> >>> do you think it is possible to extract some of the
> functionality to their
> >>>>>> >>> own files and have test around those functionalities?
> >>>>>> >>
> >>>>>> >> To improve the code readability, reduce code complexity and to
> make code
> >>>>>> >> testable, the code must be splitted component wise.
> >>>>>> >> Here is my suggestion:
> >>>>>> >>
> >>>>>> >> 1. The code for classes like “SQLEditorView” and
> “SqlEditorController” can
> >>>>>> >> be moved into two files like "editor_view.js and
> “editor_controller.js" and
> >>>>>> >> called from within "sqleditor.js".
> >>>>>> >>
> >>>>>> >> 2. All utilities functions can be moved into separate utils file
> and can
> >>>>>> >> write test cases.
> >>>>>> >>
> >>>>>> >> 3. Slickgrid listener functions such as:
> >>>>>> >> onBeforeEditCell, onKeyDown,
> >>>>>> >> onCellChange, onAddNewRow
> >>>>>> >>
> >>>>>> >> can be moved into
> >>>>>> >> a file and write test cases around.
> >>>>>> >>
> >>>>>> >> This needs discussion. Any suggestion?
> >>>>>> >>>
> >>>>>> >>>
> >>>>>> >>> Do we really need to have an epicRandomString function in our
> code? Would
> >>>>>> >>> it be better to use a library that would provide us that
> functionality out
> >>>>>> >>> of the box?
> >>>>>> >>
> >>>>>> >> We are using "epicRandomString function" to uniquely identify
> each record,
> >>>>>> >> I talked to Harshal who is eliminating the use of this function
> and instead
> >>>>>> >> maintaining counter for the rows added/updated/deleted.
> >>>>>> >>>
> >>>>>> >>> The functions this.applyValue in slick.pgadmin.editors.js that
> were
> >>>>>> >>> change in this patch are exactly the same, can we extract that
> code into a
> >>>>>> >>> single function instead of repeating the code?
> >>>>>> >>
> >>>>>> >> I have moved common logic into a new function.
> >>>>>> >>>
> >>>>>> >>>
> >>>>>> >>> Using feature tests is a good option to ensure that the
> integration of
> >>>>>> >>> all the components of the application is working as expected,
> but in order
> >>>>>> >>> to tests behaviors that are edge cases the Unit Tests are much
> cheaper to
> >>>>>> >>> run and create.
> >>>>>> >>
> >>>>>> >> I will write test cases for functions once they are moved into
> their
> >>>>>> >> separate files as discussed above.
> >>>>>> >>>
> >>>>>> >>>
> >>>>>> >>> Thanks
> >>>>>> >>> Joao & Shruti
> >>>>>> >>>
> >>>>>> >>>
> >>>>>> >>> On Thu, May 18, 2017 at 1:22 AM, Surinder Kumar
> >>>>>> >>> <surinder.ku...@enterprisedb.com> wrote:
> >>>>>> >>>>
> >>>>>> >>>> Hi Dave,
> >>>>>> >>>>
> >>>>>> >>>> Please review the updated patch.
> >>>>>> >>>>
> >>>>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <dp...@pgadmin.org>
> wrote:
> >>>>>> >>>>>
> >>>>>> >>>>> Hi
> >>>>>> >>>>>
> >>>>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
> >>>>>> >>>>> <surinder.ku...@enterprisedb.com> wrote:
> >>>>>> >>>>>>
> >>>>>> >>>>>> Hi Dave,
> >>>>>> >>>>>>
> >>>>>> >>>>>> Implementation:
> >>>>>> >>>>>>
> >>>>>> >>>>>> 1) Took a flag 'is_row_copied' for each copied row:
> >>>>>> >>>>>>
> >>>>>> >>>>>> - to distinguish it from add/update row.
> >>>>>> >>>>>> - to write code specific to copied row only as it requires
> different
> >>>>>> >>>>>> logic than add row.
> >>>>>> >>>>>>
> >>>>>> >>>>>> 2) After pasting an existing row:
> >>>>>> >>>>>>
> >>>>>> >>>>>> - If a user clear the cell value, it must set cell to
> [default] value
> >>>>>> >>>>>> if default value is explicitly given for column while
> creating table
> >>>>>> >>>>>> otherwise [null].
> >>>>>> >>>>>>
> >>>>>> >>>>>> - Again, if a user entered a value in same cell, then
> removes that
> >>>>>> >>>>>> value, set it to [null] (the same behaviour is for add row).
> >>>>>> >>>>>>
> >>>>>> >>>>>> 3) Took a 2-dimensional array "grid.copied_rows" to keep
> track the
> >>>>>> >>>>>> changes of each row's cell so that changes made to each cell
> are independent
> >>>>>> >>>>>> and removed once data is saved.
> >>>>>> >>>>>>
> >>>>>> >>>>>> 4) On pasting a row, the cell must be highlighted with light
> green
> >>>>>> >>>>>> colour, so triggers an addNewRow event. Now copied row will
> add to grid
> >>>>>> >>>>>> instead of re-rendering all rows again.
> >>>>>> >>>>>>
> >>>>>> >>>>>> 5) Moved the common logic into functions.
> >>>>>> >>>>>>
> >>>>>> >>>>>> This patch pass the feature test cases and jasmine test case.
> >>>>>> >>>>>> Also I will add the test cases for copy row and send an
> updated patch.
> >>>>>> >>>>>>
> >>>>>> >>>>>> Please find attached patch and review.
> >>>>>> >>>>>
> >>>>>> >>>>>
> >>>>>> >>>>> Looks good. I saw the following issues:
> >>>>>> >>>>>
> >>>>>> >>>>> - The "new" row is not available once I've created the first
> new row,
> >>>>>> >>>>> or pasted some.
> >>>>>> >>>>
> >>>>>> >>>> Fixed.
> >>>>>> >>>>>
> >>>>>> >>>>>
> >>>>>> >>>>> - Rows are pasted in reverse order.
> >>>>>> >>>>
> >>>>>> >>>> Fixed.
> >>>>>> >>>>>
> >>>>>> >>>>>
> >>>>>> >>>>> - If I copy/paste a new row that has yet to be saved, none of
> the
> >>>>>> >>>>> values are actually copied.
> >>>>>> >>>>
> >>>>>> >>>> Fixed.
> >>>>>> >>>>>
> >>>>>> >>>>>
> >>>>>> >>>>> - Attempting to save a row that contains all null/default
> values
> >>>>>> >>>>> results in an invalid query:
> >>>>>> >>>>>
> >>>>>> >>>>> INSERT INTO public.defaults (
> >>>>>> >>>>> ) VALUES (
> >>>>>> >>>>> );
> >>>>>> >>>>>
> >>>>>> >>>>> I think the only answer here is to explicitly insert NULL
> into any
> >>>>>> >>>>> columns *without* a default value.
> >>>>>> >>>>
> >>>>>> >>>> I could not reproduce, However #3 might have fixed it too.
> >>>>>> >>>>
> >>>>>> >>>> Apart from this, I noticed epicRandomString(...) function
> doesn't
> >>>>>> >>>> generate unique number always, due to this save and delete
> rows was
> >>>>>> >>>> affected. Not all rows saved or deleted. The new function
> always returns a
> >>>>>> >>>> unique random number.
> >>>>>> >>>> Fixed.
> >>>>>> >>>>>
> >>>>>> >>>>>
> >>>>>> >>>>> Thanks.
> >>>>>> >>>>>
> >>>>>> >>>>> --
> >>>>>> >>>>> Dave Page
> >>>>>> >>>>> Blog: http://pgsnake.blogspot.com
> >>>>>> >>>>> Twitter: @pgsnake
> >>>>>> >>>>>
> >>>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com
> >>>>>> >>>>> The Enterprise PostgreSQL Company
> >>>>>> >>>>
> >>>>>> >>>>
> >>>>>> >>>>
> >>>>>> >>>>
> >>>>>> >>>> --
> >>>>>> >>>> Sent via pgadmin-hackers mailing list (
> pgadmin-hackers@postgresql.org)
> >>>>>> >>>> To make changes to your subscription:
> >>>>>> >>>> http://www.postgresql.org/mailpref/pgadmin-hackers
> >>>>>> >>>>
> >>>>>> >>>
> >>>>>> >>
> >>>>>> >
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Dave Page
> >>>>>> Blog: http://pgsnake.blogspot.com
> >>>>>> Twitter: @pgsnake
> >>>>>>
> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com
> >>>>>> The Enterprise PostgreSQL Company
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>
>
Feature_test_cases_view_data_v3.patch
Description: Binary data
-- Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers
