Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query

Hello Khushboo,
Looks like we are almost doen, just missing one PEP-8 issue:
→ pycodestyle --config=.pycodestyle pgadmin/tools
pgadmin/tools/sqleditor/tests/test_poll_query_tool.py:46: [E126]
continuation line over-indented for hanging indent
1 E126 continuation line over-indented for hanging indent
1

The tests run successfully in our CI pipeline.

Thanks
Joao

On Sun, Mar 4, 2018 at 11:51 PM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> On Fri, Mar 2, 2018 at 6:55 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Could you rebase this please? It no longer applies.
>>
>> Please find the attached updated patch.
>
>> Thanks.
>>
>> On Thu, Mar 1, 2018 at 5:56 AM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Joao,
>>>
>>> Thanks for reviewing.
>>>
>>> On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hello Khushboo,
>>>> After reviewing the patch I have the gut feeling that we do not have
>>>> enough test coverage on this issue, specially due to the intricate while
>>>> loop and conditions around the polling.
>>>> I think that this deserve Unit tests around it, When I say Unit Test I
>>>> am not talking about executing queries against the database, but do some
>>>> stubbing of the database so that we can control the flow that we want.
>>>>
>>> You are right. It needs more unit testing. I have checked below
>>> scenarios:
>>> 1. Returns 2 notices with data output
>>> 2. Returns 1000 notices with data output
>>> 3. No notices with data output
>>>
>>> By running above, I have checked, each time returned notices are
>>> accurate, no old notices are getting appended, it does not affect with the
>>> amount of messages (few, none or more). Also, with the updated patch, I
>>> have made sure that all these queries run with the single transaction id
>>> (same connection).
>>>
>>> So, please let me know if you think I can add more things to this.
>>>
>>>>
>>>>
>>> It is a temptation to try to always do a Feature Test to test what we
>>>> want because it is "easier" to write and ultimately it is what users see,
>>>> but while 1 Feature Test runs we can run 200 Unit Tests that give us much
>>>> more confidence that the code is doing what we expect it to do.
>>>>
>>>> Right, so added regression tests instead of feature tests.
>>>
>>> This being said, I run the tests on the CI Pipeline and all tests pass.
>>>> Running pycodestyle fails due to some line sizes on the
>>>> psycopg2/__init__py. I believe that it is not what you changed, but since
>>>> you were changing the file it can be fixed it is just:
>>>>
>>>> pgadmin/utils/driver/psycopg2/__init__.py:1276: [E501] line too long
>>>> (81 > 79 characters)
>>>> pgadmin/utils/driver/psycopg2/__init__.py:1277: [E501] line too long
>>>> (91 > 79 characters)
>>>> pgadmin/utils/driver/psycopg2/__init__.py:1282: [E501] line too long
>>>> (81 > 79 characters)
>>>> pgadmin/utils/driver/psycopg2/__init__.py:1283: [E501] line too long
>>>> (91 > 79 characters)
>>>> 4 E501 line too long (81 > 79 characters)
>>>>
>>>> Fixed. Thanks for pointing out.
>>>
>>>>
>>>> Thanks
>>>> Joao
>>>>
>>>>
>>>> On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> On Mon, Feb 26, 2018 at 10:02 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Argh, I ran some tests, but didn't spot any lost messages in the
>>>>>> tests I ran. I'll revert the patch.
>>>>>>
>>>>>> Khushboo;
>>>>>>
>>>>>> Please look at the following:
>>>>>>
>>>>>> - Fix the patch so it doesn't drop messages.
>>>>>>
>>>>> Fixed.
>>>>> By default, the notice attribute of the connection object of psycopg 2
>>>>> only stores 50 notices. Once it reaches to 50 it starts from 1 again.
>>>>> To fix this I have changed the notice attribute from list to deque to
>>>>> append more messages. Currently I have kept the maximum limit at a time of
>>>>> the notice attribute is 100000 (in a single poll).
>>>>>
>>>>>> - Add regression tests to make sure it doesn't break in the future.
>>>>>> This may require creating one or more functions the spew out a whole lot of
>>>>>> notices, and then running a couple of queries and checking the output.
>>>>>>
>>>>> Added. With this regression test, the current code is failing which
>>>>> has been taken care in this patch.
>>>>>
>>>>>> - Check the messages panel on the history tab. I just noticed it
>>>>>> seems to only be showing an even smaller subset of the messages.
>>>>>>
>>>>> Tested and no issues found.
>>>>>
>>>>>>
>>>>>>
>>>>> Thanks.
>>>>>>
>>>>>> On Mon, Feb 26, 2018 at 4:23 PM, Murtuza Zabuawala <
>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Sent bit early,
>>>>>>>
>>>>>>> You can run 'VACUUM FULL VERBOSE' in query tool and verify the
>>>>>>> populated messages (pgAdmin3 vs. pgAdmin4).
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Feb 26, 2018 at 9:48 PM, Murtuza Zabuawala <
>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Khushboo/Dave,
>>>>>>>>
>>>>>>>> With given commit, I'm again seeing the issue raised in
>>>>>>>> https://redmine.postgresql.org/issues/1523 :(
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Regards,
>>>>>>>> Murtuza Zabuawala
>>>>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Feb 26, 2018 at 7:49 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Ensure we pick up the messages from the current query and not a
>>>>>>>>> previous one. Fixes #3094
>>>>>>>>>
>>>>>>>>> Branch
>>>>>>>>> ------
>>>>>>>>> master
>>>>>>>>>
>>>>>>>>> Details
>>>>>>>>> -------
>>>>>>>>>
>>>>>>>>> https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=08b3ccc01a4d57e8ea3657f8882a53dcd1b99386
>>>>>>>>> Author: Khushboo Vashi <khushboo(dot)vashi(at)enterprisedb(dot)com>
>>>>>>>>>
>>>>>>>>> Modified Files
>>>>>>>>> --------------
>>>>>>>>> web/pgadmin/utils/driver/abstract.py | 1 +
>>>>>>>>> web/pgadmin/utils/driver/psycopg2/__init__.py | 64
>>>>>>>>> +++++++++------------------
>>>>>>>>> 2 files changed, 21 insertions(+), 44 deletions(-)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> 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
>>
>