Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query - Mailing list pgadmin-hackers
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.vashi@enterprisedb.com> wrote:
On Mon, Feb 26, 2018 at 10:02 PM, Dave Page <dpage@pgadmin.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.zabuawala@enterprisedb.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.zabuawala@enterprisedb.com> wrote:Hi Khushboo/Dave,With given commit, I'm again seeing the issue raised in https://redmine.postgresql.org/issues/1523 :(--Regards,On Mon, Feb 26, 2018 at 7:49 PM, Dave Page <dpage@pgadmin.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.vashi@enterprisedb.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
pgadmin-hackers by date: