Hi Surinder, You are right, nevertheless that was not the only error we had on the flaky tests.
Thanks Joao & Shruti On Fri, May 26, 2017 at 10:18 AM, Surinder Kumar < [email protected]> 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" < > [email protected]> 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 < > [email protected]> wrote: > >> Hi >> >> Please find updated patch. >> >> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira < >> [email protected]> 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/fea >>> ture_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/s >>> ite-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/s >>> ite-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/s >>> ite-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 <[email protected]> wrote: >>> >>>> Hi >>>> >>>> The tests failed on both PG 9.4 and 9.6 for me :-( >>>> >>>> ====================================================================== >>>> ERROR: runTest (pgadmin.feature_tests.view_da >>>> ta_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_da >>>> ta_dml_queries.py", >>>> line 120, in runTest >>>> self._verify_insert_data() >>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_da >>>> ta_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_da >>>> ta_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-packa >>>> ges/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 >>>> <[email protected]> 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 >>>> > <[email protected]> wrote: >>>> >> >>>> >> Hi >>>> >> >>>> >> Please find updated patch and review. >>>> >> >>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira >>>> >> <[email protected]> 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 >>>> >>> <[email protected]> wrote: >>>> >>>> >>>> >>>> Hi Dave, >>>> >>>> >>>> >>>> Please review the updated patch. >>>> >>>> >>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[email protected]> >>>> wrote: >>>> >>>>> >>>> >>>>> Hi >>>> >>>>> >>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar >>>> >>>>> <[email protected]> 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 ( >>>> [email protected]) >>>> >>>> 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 >>>> >>> >>> >> > >
