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.ku...@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.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 >> >>>>> >> >>>>> >> >>>> >> >>> >> >> >> > > > -- 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