Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Hi Joao,

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