Re: [pgadmin-hackers] [pgAdmin4][Patch][RM_2400]: Columns withdefaults set to NULL when removing contents after pasting in the edit grid
От | Dave Page |
---|---|
Тема | Re: [pgadmin-hackers] [pgAdmin4][Patch][RM_2400]: Columns withdefaults set to NULL when removing contents after pasting in the edit grid |
Дата | |
Msg-id | CA+OCxoxx-1tpugsAwgZO6oJu=1isk7Awt8ZUpjuNiE5QJjgLHA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [pgadmin-hackers] [pgAdmin4][Patch][RM_2400]: Columns withdefaults set to NULL when removing contents after pasting in the edit grid (Surinder Kumar <surinder.kumar@enterprisedb.com>) |
Ответы |
Re: [pgadmin-hackers] [pgAdmin4][Patch][RM_2400]: Columns withdefaults set to NULL when removing contents after pasting in the edit grid
|
Список | pgadmin-hackers |
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 Also, Can we replace the sleep with a "wait for object" or similar? 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
В списке pgadmin-hackers по дате отправления:
Предыдущее
От: pgAdmin 4 JenkinsДата:
Сообщение: [pgadmin-hackers] Build failed in Jenkins: pgadmin4-master-python35 #119
Следующее
От: pgAdmin 4 JenkinsДата:
Сообщение: [pgadmin-hackers] Build failed in Jenkins: pgadmin4-master-python26 #240