Re: [pgadmin-hackers] [pgAdmin4][Patch][RM_2400]: Columns withdefaults set to NULL when removing contents after pasting in the edit grid

Thanks - I committed the code changes, as they seem to work very well.
The regression tests are failing for me though :-(. Can you take
another look please? Note that I'm running under Python 2.7 on Mal

======================================================================
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 106, in runTest
    self.page.close_query_tool()
  File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
line 68, in close_query_tool
    self.driver.switch_to.frame(self.driver.find_elements_by_tag_name("iframe")[0])
IndexError: list index out of range

On Sat, May 27, 2017 at 9:11 AM, Surinder Kumar

<surinder.kumar@enterprisedb.com> wrote:
> 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.kumar@enterprisedb.com> wrote:
>>
>> Hi Joao,
>>
>> On May 26, 2017 8:33 PM, "Joao Pedro De Almeida Pereira"
>> <jdealmeidapereira@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.kumar@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"
>> >> <jdealmeidapereira@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.kumar@enterprisedb.com> wrote:
>> >>>>
>> >>>> Hi
>> >>>>
>> >>>> Please find updated patch.
>> >>>>
>> >>>> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira
>> >>>> <jdealmeidapereira@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 <dpage@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.kumar@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.kumar@enterprisedb.com> wrote:
>> >>>>>> >>
>> >>>>>> >> Hi
>> >>>>>> >>
>> >>>>>> >> Please find updated patch and review.
>> >>>>>> >>
>> >>>>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> >>>>>> >> <jdealmeidapereira@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.kumar@enterprisedb.com> wrote:
>> >>>>>> >>>>
>> >>>>>> >>>> Hi Dave,
>> >>>>>> >>>>
>> >>>>>> >>>> Please review the updated patch.
>> >>>>>> >>>>
>> >>>>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <dpage@pgadmin.org>
>> >>>>>> >>>> wrote:
>> >>>>>> >>>>>
>> >>>>>> >>>>> Hi
>> >>>>>> >>>>>
>> >>>>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>> >>>>>> >>>>> <surinder.kumar@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
>> >>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >
>
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company