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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
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.


Thanks,
Surinder Kumar





--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

RM_2400.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> 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.

- Rows are pasted in reverse order.

- If I copy/paste a new row that has yet to be saved, none of the values are actually copied.

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

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

RM_2400_v1.patch (29K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Joao De Almeida Pereira
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?

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?
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?

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.

Thanks
Joao & Shruti


On Thu, May 18, 2017 at 1:22 AM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers


Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
Hi

Please find updated patch and review.

On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> wrote:
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers





--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

RM_2400_v2.patch (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
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 cellexpected 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 <[hidden email]> wrote:
Hi

Please find updated patch and review.

On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> wrote:
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers






--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Feature_test_cases_view_data_v1.patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <[hidden email]> 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
>>> <[hidden email]> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <[hidden email]> 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 ([hidden email])
>>>> 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


--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Reply | Threaded
Open this post in threaded view
|

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

Joao De Almeida Pereira
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

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?

Does _check_xss_in_view_data method checks for Cross Site Scripting?

Was there any reason to duplicate self.page._connects_to_server and self.page._close_query_tool?

The method _verify_insert_data looks more or less the same code as in the end of _copy_paste_row, should this be merged?

Thanks,
Joao & Shruti


On Thu, May 25, 2017 at 4:41 PM, Dave Page <[hidden email]> 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

Also, Can we replace the sleep with a "wait for object" or similar?

On Thu, May 25, 2017 at 6:42 AM, Surinder Kumar
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <[hidden email]> 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
>>> <[hidden email]> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <[hidden email]> 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 ([hidden email])
>>>> 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

Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
Hi

Please find updated patch.

On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> 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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <[hidden email]> 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
>>> <[hidden email]> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <[hidden email]> 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 ([hidden email])
>>>> 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




--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Feature_test_cases_view_data_v2.patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Joao De Almeida Pereira
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 <[hidden email]> wrote:
Hi

Please find updated patch.

On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> 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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <[hidden email]> 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
>>> <[hidden email]> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <[hidden email]> 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 ([hidden email])
>>>> 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



Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar

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" <[hidden email]> 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 <[hidden email]> wrote:
Hi

Please find updated patch.

On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> 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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <[hidden email]> 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
>>> <[hidden email]> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <[hidden email]> 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 ([hidden email])
>>>> 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




Reply | Threaded
Open this post in threaded view
|

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

Joao De Almeida Pereira
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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> wrote:
Hi

Please find updated patch.

On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> 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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <[hidden email]> 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
>>> <[hidden email]> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <[hidden email]> 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 ([hidden email])
>>>> 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





Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar

Hi Joao,

On May 26, 2017 8:33 PM, "Joao Pedro De Almeida Pereira" <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> wrote:
>>>>
>>>> Hi
>>>>
>>>> Please find updated patch.
>>>>
>>>> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>> > <[hidden email]> wrote:
>>>>>> >>
>>>>>> >> Hi
>>>>>> >>
>>>>>> >> Please find updated patch and review.
>>>>>> >>
>>>>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>>>>>> >> <[hidden email]> 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
>>>>>> >>> <[hidden email]> wrote:
>>>>>> >>>>
>>>>>> >>>> Hi Dave,
>>>>>> >>>>
>>>>>> >>>> Please review the updated patch.
>>>>>> >>>>
>>>>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>> >>>>>
>>>>>> >>>>> Hi
>>>>>> >>>>>
>>>>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>>> >>>>> <[hidden email]> 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 ([hidden email])
>>>>>> >>>> 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
>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
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 <[hidden email]> wrote:

Hi Joao,

On May 26, 2017 8:33 PM, "Joao Pedro De Almeida Pereira" <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> wrote:
>>>>
>>>> Hi
>>>>
>>>> Please find updated patch.
>>>>
>>>> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <[hidden email]> 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 <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>> > <[hidden email]> wrote:
>>>>>> >>
>>>>>> >> Hi
>>>>>> >>
>>>>>> >> Please find updated patch and review.
>>>>>> >>
>>>>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>>>>>> >> <[hidden email]> 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
>>>>>> >>> <[hidden email]> wrote:
>>>>>> >>>>
>>>>>> >>>> Hi Dave,
>>>>>> >>>>
>>>>>> >>>> Please review the updated patch.
>>>>>> >>>>
>>>>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
>>>>>> >>>>>
>>>>>> >>>>> Hi
>>>>>> >>>>>
>>>>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>>> >>>>> <[hidden email]> 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 ([hidden email])
>>>>>> >>>> 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
>>>>>
>>>>>
>>>>
>>>
>>
>




--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Feature_test_cases_view_data_v3.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi Joao,
>>
>> On May 26, 2017 8:33 PM, "Joao Pedro De Almeida Pereira"
>> <[hidden email]> 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
>> > <[hidden email]> 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"
>> >> <[hidden email]> 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
>> >>> <[hidden email]> wrote:
>> >>>>
>> >>>> Hi
>> >>>>
>> >>>> Please find updated patch.
>> >>>>
>> >>>> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira
>> >>>> <[hidden email]> 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 <[hidden email]>
>> >>>>> 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
>> >>>>>> <[hidden email]> 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
>> >>>>>> > <[hidden email]> wrote:
>> >>>>>> >>
>> >>>>>> >> Hi
>> >>>>>> >>
>> >>>>>> >> Please find updated patch and review.
>> >>>>>> >>
>> >>>>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> >>>>>> >> <[hidden email]> 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
>> >>>>>> >>> <[hidden email]> wrote:
>> >>>>>> >>>>
>> >>>>>> >>>> Hi Dave,
>> >>>>>> >>>>
>> >>>>>> >>>> Please review the updated patch.
>> >>>>>> >>>>
>> >>>>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]>
>> >>>>>> >>>> wrote:
>> >>>>>> >>>>>
>> >>>>>> >>>>> Hi
>> >>>>>> >>>>>
>> >>>>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>> >>>>>> >>>>> <[hidden email]> 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
>> >>>>>> >>>> ([hidden email])
>> >>>>>> >>>> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
Hi Dave

Please find update Feature test cases patch.

On Sun, May 28, 2017 at 12:40 AM, Dave Page <[hidden email]> wrote:
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
​Actually i used the generic close_query_tool method to close view data panel which popup with unsaved changes. but in my case nothing popups as there as no unsaved changes, so clicking on [x] button close the panel. So I write close_data_grid method in pgadmin_page.py file.

======================================================================
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
<[hidden email]> 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
> <[hidden email]> wrote:
>>
>> Hi Joao,
>>
>> On May 26, 2017 8:33 PM, "Joao Pedro De Almeida Pereira"
>> <[hidden email]> 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
>> > <[hidden email]> 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"
>> >> <[hidden email]> 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
>> >>> <[hidden email]> wrote:
>> >>>>
>> >>>> Hi
>> >>>>
>> >>>> Please find updated patch.
>> >>>>
>> >>>> On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira
>> >>>> <[hidden email]> 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 <[hidden email]>
>> >>>>> 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
>> >>>>>> <[hidden email]> 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
>> >>>>>> > <[hidden email]> wrote:
>> >>>>>> >>
>> >>>>>> >> Hi
>> >>>>>> >>
>> >>>>>> >> Please find updated patch and review.
>> >>>>>> >>
>> >>>>>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> >>>>>> >> <[hidden email]> 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
>> >>>>>> >>> <[hidden email]> wrote:
>> >>>>>> >>>>
>> >>>>>> >>>> Hi Dave,
>> >>>>>> >>>>
>> >>>>>> >>>> Please review the updated patch.
>> >>>>>> >>>>
>> >>>>>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]>
>> >>>>>> >>>> wrote:
>> >>>>>> >>>>>
>> >>>>>> >>>>> Hi
>> >>>>>> >>>>>
>> >>>>>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>> >>>>>> >>>>> <[hidden email]> 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
>> >>>>>> >>>> ([hidden email])
>> >>>>>> >>>> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Feature_test_cases_view_data_v4.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Sat, May 27, 2017 at 4:03 PM, Surinder Kumar
<[hidden email]> wrote:

> Hi Dave
>
> Please find update Feature test cases patch.
>
> On Sun, May 28, 2017 at 12:40 AM, Dave Page <[hidden email]> wrote:
>>
>> 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
>
> Actually i used the generic close_query_tool method to close view data panel
> which popup with unsaved changes. but in my case nothing popups as there as
> no unsaved changes, so clicking on [x] button close the panel. So I write
> close_data_grid method in pgadmin_page.py file.

It passed with PG 9.4, but then failed (twice) under 9.6:

======================================================================
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 105, in runTest
    self._copy_paste_row()
  File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
line 250, in _copy_paste_row
    self._verify_row_data(False)
  File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
line 275, in _verify_row_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 142, in _compare_cell_value
    CheckForViewDataTest.TIMEOUT_STRING
  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 div element to appear


----------------------------------------------------------------------


--
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Here's the screenshot.

On Sat, May 27, 2017 at 4:26 PM, Dave Page <[hidden email]> wrote:

> Hi
>
> On Sat, May 27, 2017 at 4:03 PM, Surinder Kumar
> <[hidden email]> wrote:
>> Hi Dave
>>
>> Please find update Feature test cases patch.
>>
>> On Sun, May 28, 2017 at 12:40 AM, Dave Page <[hidden email]> wrote:
>>>
>>> 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
>>
>> Actually i used the generic close_query_tool method to close view data panel
>> which popup with unsaved changes. but in my case nothing popups as there as
>> no unsaved changes, so clicking on [x] button close the panel. So I write
>> close_data_grid method in pgadmin_page.py file.
>
> It passed with PG 9.4, but then failed (twice) under 9.6:
>
> ======================================================================
> 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 105, in runTest
>     self._copy_paste_row()
>   File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
> line 250, in _copy_paste_row
>     self._verify_row_data(False)
>   File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
> line 275, in _verify_row_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 142, in _compare_cell_value
>     CheckForViewDataTest.TIMEOUT_STRING
>   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 div element to appear
>
>
> ----------------------------------------------------------------------
>
>
> --
> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

CheckForViewDataTest-2017.05.27_16.22.57-Python-2.7.10.png (559K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Surinder Kumar
Hi Dave,

Please find updated patch with following changes:

1) Locate grid row by div's style attribute 'top'(i.e. 'top:0px' for first row), instead of by div class-name because the order of rendered rows is not always same.

2) Increase the wait timeout of WebDriverWait to 5 seconds in '_compare_cell_value(...)' method.

3) Add a new utils method 'find_by_css_selector' in pgadmin_page.py to locate css element with wait_for method.

4) Add a new utils method 'wait_for_element_to_stale' in pgadmin_page.py. It is useful when "StaleElementReferenceException" exception is raised and element is not attached to the page while finding element by xpath.

5) Instead of finding each cell value by xpath and compare with actual value, now a row is located using xpath and all of cell values are extracted into an array and then compared with actual values. It eliminates the use of wait_timeout.

Also, I added a print statement for debugging where a TimeoutException was occurred last time.


On Sun, May 28, 2017 at 1:58 AM, Dave Page <[hidden email]> wrote:
Here's the screenshot.

On Sat, May 27, 2017 at 4:26 PM, Dave Page <[hidden email]> wrote:
> Hi
>
> On Sat, May 27, 2017 at 4:03 PM, Surinder Kumar
> <[hidden email]> wrote:
>> Hi Dave
>>
>> Please find update Feature test cases patch.
>>
>> On Sun, May 28, 2017 at 12:40 AM, Dave Page <[hidden email]> wrote:
>>>
>>> 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
>>
>> Actually i used the generic close_query_tool method to close view data panel
>> which popup with unsaved changes. but in my case nothing popups as there as
>> no unsaved changes, so clicking on [x] button close the panel. So I write
>> close_data_grid method in pgadmin_page.py file.
>
> It passed with PG 9.4, but then failed (twice) under 9.6:
>
> ======================================================================
> 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 105, in runTest
>     self._copy_paste_row()
>   File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
> line 250, in _copy_paste_row
>     self._verify_row_data(False)
>   File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
> line 275, in _verify_row_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 142, in _compare_cell_value
>     CheckForViewDataTest.TIMEOUT_STRING
>   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 div element to appear
>
>
> ----------------------------------------------------------------------
>
>
> --
> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Feature_test_cases_view_data_v5.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Thanks, patch applied.

On Fri, Jun 2, 2017 at 8:55 PM, Surinder Kumar
<[hidden email]> wrote:

> Hi Dave,
>
> Please find updated patch with following changes:
>
> 1) Locate grid row by div's style attribute 'top'(i.e. 'top:0px' for first
> row), instead of by div class-name because the order of rendered rows is not
> always same.
>
> 2) Increase the wait timeout of WebDriverWait to 5 seconds in
> '_compare_cell_value(...)' method.
>
> 3) Add a new utils method 'find_by_css_selector' in pgadmin_page.py to
> locate css element with wait_for method.
>
> 4) Add a new utils method 'wait_for_element_to_stale' in pgadmin_page.py. It
> is useful when "StaleElementReferenceException" exception is raised and
> element is not attached to the page while finding element by xpath.
>
> 5) Instead of finding each cell value by xpath and compare with actual
> value, now a row is located using xpath and all of cell values are extracted
> into an array and then compared with actual values. It eliminates the use of
> wait_timeout.
>
> Also, I added a print statement for debugging where a TimeoutException was
> occurred last time.
>
>
> On Sun, May 28, 2017 at 1:58 AM, Dave Page <[hidden email]> wrote:
>>
>> Here's the screenshot.
>>
>> On Sat, May 27, 2017 at 4:26 PM, Dave Page <[hidden email]> wrote:
>> > Hi
>> >
>> > On Sat, May 27, 2017 at 4:03 PM, Surinder Kumar
>> > <[hidden email]> wrote:
>> >> Hi Dave
>> >>
>> >> Please find update Feature test cases patch.
>> >>
>> >> On Sun, May 28, 2017 at 12:40 AM, Dave Page <[hidden email]> wrote:
>> >>>
>> >>> 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
>> >>
>> >> Actually i used the generic close_query_tool method to close view data
>> >> panel
>> >> which popup with unsaved changes. but in my case nothing popups as
>> >> there as
>> >> no unsaved changes, so clicking on [x] button close the panel. So I
>> >> write
>> >> close_data_grid method in pgadmin_page.py file.
>> >
>> > It passed with PG 9.4, but then failed (twice) under 9.6:
>> >
>> > ======================================================================
>> > 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 105, in runTest
>> >     self._copy_paste_row()
>> >   File
>> > "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
>> > line 250, in _copy_paste_row
>> >     self._verify_row_data(False)
>> >   File
>> > "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
>> > line 275, in _verify_row_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 142, in _compare_cell_value
>> >     CheckForViewDataTest.TIMEOUT_STRING
>> >   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 div element to appear
>> >
>> >
>> > ----------------------------------------------------------------------
>> >
>> >
>> > --
>> > 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
>
>



--
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Previous Thread Next Thread