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

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.

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.

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)

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