Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1erQ1y-0007Mn-LW for pgadmin-hackers@arkaria.postgresql.org; Thu, 01 Mar 2018 15:22:02 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1erQ1x-0008PR-4T for pgadmin-hackers@arkaria.postgresql.org; Thu, 01 Mar 2018 15:22:01 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1erQ1w-0008PH-Tk for pgadmin-hackers@lists.postgresql.org; Thu, 01 Mar 2018 15:22:01 +0000 Received: from mail-it0-x244.google.com ([2607:f8b0:4001:c0b::244]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1erQ1g-0002s0-S0 for pgadmin-hackers@postgresql.org; Thu, 01 Mar 2018 15:21:59 +0000 Received: by mail-it0-x244.google.com with SMTP id v194so8249346itb.0 for ; Thu, 01 Mar 2018 07:21:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SpjMiVFmPwvhxAX+6Es0Y7WKxzqlDAYXjMhrEYT8Xu4=; b=ad9H3QSLYdM1rqZQ1XJ0YpgzIkQAOUP8Z7fJGcruh2Q1LqYvyEz9FMmUZApvpugHj3 rxCX1AnWsZCKqKLgPEeJ5dVdZX9kz6Cl8efM7sQ40odW415BBgNXX2csgSVdKDicOHXI JAhFOPdV7QjIkvGRYUGTCaH4Xz7SynyrJOR9jJ7rOagLNmPgFocxLCjsc284hv0XKsDg JJ6AOy3AI2ClxXtprJudsvTKV4+ZL3cM94fWFTtEivKp95Du1Ay8P3ZXoYYSgo8vO69v pxbIEpsvVuLS3/l0DP+6XL7a8gpbcRs10jzSymWUXztmTGoZ5i5lJ0f9qUd1sg4LpuP2 UG4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SpjMiVFmPwvhxAX+6Es0Y7WKxzqlDAYXjMhrEYT8Xu4=; b=pCZz4vUexN+xWja34KOz3jvCJSf3bk2B41ML66cHyAuVSe0MY3W63BMN4NCoFI1lS9 M44WNmMW3ZUz/r+ppKV5mdXo0v3fjosciFuil757ny0XbDP/8/KDRMwsSGFfNPRb6XJr DVQIJzZ90mcv7f0zYB+B456oLzM4pFQIfq7LUIpdJThPt+gsldJlRFYD/1wqI971NOGk ytMoYdn8R5VQv3E7tIlCujsbVO/XBMFtC9tgT70XSwgaE4FtMtV6+nBr5Qe4AXtFnheU bpZ7yUx6xIemp6ZnXzc9VJ1d9MF9z5klqhVtb9pYPrAG8vEmCoAe9ZqIGb312/Tq6zcP RKrQ== X-Gm-Message-State: APf1xPC4maJjlm5hl94Z4+AuiNvKHo0fV3AV7sDzhYV4Jdha8EfxoTv6 bwZBCWIYRGG9JjO69qNxxZto0kpKGZdJ8bTaFooDHg== X-Google-Smtp-Source: AG47ELsUoPgdVMbeBvmlrWrlWgcyq+Pd95g6xeltmZqY22nnS4oHEslzCgMW2q6+DLtmIIgPbqSuGzmkJbkVLOWUDRk= X-Received: by 10.36.78.14 with SMTP id r14mr2987718ita.146.1519917701647; Thu, 01 Mar 2018 07:21:41 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Thu, 01 Mar 2018 15:21:31 +0000 Message-ID: Subject: Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query To: Khushboo Vashi Cc: Dave Page , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a113a9986b1e6f605665b6a3e" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a113a9986b1e6f605665b6a3e Content-Type: text/plain; charset="UTF-8" Hello Khushboo, The patch runs successfully in our CI with all tests passing. I see the test that you created, and I do not understand why we need to create tests that do HTTP requests in order to check something that is executed against the database. What I was talking about in my previous email was having tests that tested the function by itself. (Copied from: https://jfiaffe.files.wordpress.com/2014/09/tests-pyramid.png) This is the Testing Pyramid, there are a bunch of different drawings of it and ways to explain it, but in broad stokes what is means is that we should have the majority of tests around a Unit (that are some disagreements in the community of what a Unit is) and a very small amount of Manual testing. What Unit usually means is piece of code, it can be a function, it can be a class or it can even be a module, but is something self contained. In pgAdmin's case the majority of our tests go around the Integration Layer because we are using HTTP requests in order to executing queries in the database, so basically we are doing tests end to end in the backend, and the cost time. I do not want to held this patch back because of this, and I say this because I have minimal confidence with the tests that you created, that they would catch the majority of the problems, and hope that the majority of the code is exercised by it. Nevertheless I would like to challenge all the Hackers to think about testing in a different way. The tests in our code are used to give us confidence that the work we did is working as expected, this also makes it much easier to refactor out bad patterns or very complicated ones into something simple. A signal that our code is more complicated then it should is when we need to test some behavior and we end up with a Stubbing Hell or we need to test it End 2 End because it is to hard to isolate the part we want to test. In the other hand we should not test all functions and every class, because we might be coupling our tests to much to the implementation and that will have the contrary effect, and we will not be able to refactor and simply our code. Like everything in life there need to be a balance. Thanks Joao On Thu, Mar 1, 2018 at 12:56 AM Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > Hi Joao, > > Thanks for reviewing. > > On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeida Pereira < > jdealmeidapereira@pivotal.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.vashi@enterprisedb.com> wrote: >> >>> On Mon, Feb 26, 2018 at 10:02 PM, Dave Page 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, >>>>>> Murtuza Zabuawala >>>>>> EnterpriseDB: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>>> >>>>>> On Mon, Feb 26, 2018 at 7:49 PM, Dave Page 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 >>>>>>> >>>>>>> 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 >>>> >>> --001a113a9986b1e6f605665b6a3e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Khushboo,
The patch runs successfully in our CI = with all tests passing.

I see the test that you cr= eated, and I do not understand why we need to create tests that do HTTP req= uests in order to check something that is executed against the database. Wh= at I was talking about in my previous email was having tests that tested th= e function by itself.

=C2=A0

This is the Testing Pyramid, there are a bunch = of different drawings of it and ways to explain it, but in broad stokes wha= t is means is that we should have the majority of tests around a Unit (that= are some disagreements in the community of what a Unit is) and a very smal= l amount of Manual testing. What Unit usually means is piece of code, it ca= n be a function, it can be a class or it can even be a module, but is somet= hing self contained. In pgAdmin's case the majority of our tests go aro= und the Integration Layer because we are using HTTP requests in order to ex= ecuting queries in the database, so basically we are doing tests end to end= in the backend, and the cost time.

I do not want = to held this patch back because of this, and I say this because I have mini= mal confidence with the tests that you created, that they would catch the m= ajority of the problems, and hope that the majority of the code is exercise= d by it.

Nevertheless I would like to challenge al= l the Hackers to think about testing in a different way. The tests in our c= ode are used to give us confidence that the work we did is working as expec= ted, this also makes it much easier to refactor out bad patterns or very co= mplicated ones into something simple. A signal that our code is more compli= cated then it should is when we need to test some behavior and we end up wi= th a Stubbing Hell or we need to test it End 2 End because it is to hard to= isolate the part we want to test. In the other hand we should not test all= functions and every class, because we might be coupling our tests to much = to the implementation and that will have the contrary effect, and we will n= ot be able to refactor and simply our code.
Like everything in li= fe there need to be a balance.

Thanks
Jo= ao

On Thu, Mar 1= , 2018 at 12:56 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Joao,

Than= ks for reviewing.

On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almei= da Pereira <jdealmeidapereira@pivotal.io> wrote:<= br>
Hello Khushboo,
Afte= r reviewing the patch I have the gut feeling that we do not have enough tes= t coverage on this issue, specially due to the intricate while loop and con= ditions around the polling.
I think that this deserve Unit tests = around it, When I say Unit Test I am not talking about executing queries ag= ainst the database, but do some stubbing of the database so that we can con= trol 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=C2=A0

By running above, I have checked, ea= ch time returned notices are accurate, no old notices are getting appended,= it does not affect with the amount of messages (few, none or more).=C2=A0 = Also, with the updated patch, I have made sure that all these queries run w= ith the single transaction id (same connection).

S= o, please let me know if you think I can add more things to this.
= =C2=A0=C2=A0
It is a temptation to try to always do a Feature = Test to test what we want because it is "easier" to write and ult= imately 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.=C2= =A0

This being said, I run the tests on t= he CI Pipeline and all tests pass. Running pycodestyle fails due to some li= ne sizes on the psycopg2/__init__py. I believe that it is not what you chan= ged, but since you were changing the file it can be fixed it is just:
=

pgadmin/utils/driver/psycopg2/__init__.py:1276: [E= 501] line too long (81 > 79 characters)
pgadmin/utils/driver/p= sycopg2/__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=C2=A0 =C2= =A0 =C2=A0 =C2=A0E501 line too long (81 > 79 characters)

Fixed. Thanks f= or pointing out.=C2=A0
<= div class=3D"gmail_extra">

Thanks
Joao


=
On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi <khushboo.vash= i@enterprisedb.com> wrote:
<= div dir=3D"ltr">
On Mo= n, Feb 26, 2018 at 10:02 PM, Dave Page <dpage@pgadmin.org> w= rote:
Argh, I ran some tests, but didn't spot any lost messages in the tests= I ran. I'll revert the patch.

Khushboo;
<= br>
Please look at the following:

- Fix = the patch so it doesn't drop messages.
Fixed.
By default, the notice attribute of the connect= ion object of psycopg 2 only stores 50 notices. Once it reaches to 50 it st= arts from 1 again.
To fix this I have changed the notice attribut= e from list to deque to append more messages. Currently I have kept the max= imum limit at a time of the notice attribute is=C2=A0100000 (in a single po= ll).=C2=A0
- 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 car= e 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 t= he messages.
Tested = and no issues found.
=C2=A0
=
Thanks.

On Mon, Feb 26, 2018 at 4:23 PM, Murtuza Zabuawala= <murtuza.zabuawala@enterprisedb.com> wrote= :
Sent bit ear= ly,=C2=A0

You can run 'V= ACUUM FULL VERBOSE' in query tool and verify the populated messa= ges (pgAdmin3 vs. pgAdmin4).=C2=A0


On Mon, Feb 26, 2= 018 at 9:48 PM, Murtuza Zabuawala <murtuza.zabuawala@ente= rprisedb.com> wrote:
Hi Khushboo/Dave,

With given commit, I'm again seeing= the issue raised in https://redmine.postgresql.org/issues/1523 :(


<= /div>

=

<= div dir=3D"ltr">
--
Regards,
<= font size=3D"2">Murtuz= a Zabuawala
EnterpriseDB:=C2=A0http://www.enterprisedb.com
The= Enterprise PostgreSQL Company

<= /div>

On Mon, Feb 26, 2018 at 7:49 PM, Dave Page <= span dir=3D"ltr"><dpage@pgadmin.org> wrote:
Ensure we pick up the messages from the current query an= d not a previous one. Fixes #3094

Branch
------
master

Details
-------
https://git.postgresql.org/gitweb?p=3Dpgadmin4.git;a=3Dcommitdi= ff;h=3D08b3ccc01a4d57e8ea3657f8882a53dcd1b99386
Author: Khushboo Vashi <khushboo.vashi@enterprisedb.com>

Modified Files
--------------
web/pgadmin/utils/driver/abstract.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 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
<= br>EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
=
--001a113a9986b1e6f605665b6a3e--