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 1euKMC-0000zs-Jt for pgadmin-hackers@arkaria.postgresql.org; Fri, 09 Mar 2018 15:54:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1euKMB-0007h4-4k for pgadmin-hackers@arkaria.postgresql.org; Fri, 09 Mar 2018 15:54:55 +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 1euKMA-0007gY-R6 for pgadmin-hackers@lists.postgresql.org; Fri, 09 Mar 2018 15:54:55 +0000 Received: from mail-io0-x242.google.com ([2607:f8b0:4001:c06::242]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1euKM0-0007xg-Mi for pgadmin-hackers@postgresql.org; Fri, 09 Mar 2018 15:54:53 +0000 Received: by mail-io0-x242.google.com with SMTP id f1so4038358iob.0 for ; Fri, 09 Mar 2018 07:54: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=9E8MkY2dBzMOIysnblRtvpwk4XTvIjuGZQjUIMuRiRs=; b=xcz1mOxOnrJmco/B0s3XKKpUnYEQ2gB5Gw5cOSLwLgAB939zktFF6/L7X55ic2eGsR V4XVvA6FQRoEtauDw8LO537QTxYW+L17u1cXmmpsryispUGyhXTd7ADO1q4l1ttjAdEh aVugDJktj1BqwGPH4Fa77DgExTCfdTG9x8e4MBQrmZGgjhQcMdm1yd+1ULWsivJeJ4Uc iAx27lEk8TfHrUBV/JfQaLI49clhxdCdDNoyvRMO9dOCl8J2u+Bo29Yu3OpKM1c594em hjLJjObFBpBsLMpO5kZQ6wBgebzpeEl34305EaFtLy+t/HUqRj7SA4Xp986civyIFjxs h2dg== 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=9E8MkY2dBzMOIysnblRtvpwk4XTvIjuGZQjUIMuRiRs=; b=W5BFM1F0mT6ji29VUFnYuLCDpEjjJgcy0gY/qt4CYRRuj4ZFMqQhBMogTnkQzAlt6c ZuwTsHIffhj1JsISu7P04HY1PzN6Kgp9fLr/6sgXGZzCnPn5laThWNg1V6c2/HTg9h7k 4p5E70C3nAuGrV3xj9hv1jzsWXv6pHktSpxi7rcAGhdDjV3ZlR9z5OK5xIMqzWjy+pMH 300esXvBP4T7WoOHzBD5YB8uHPi+RScZBCgS79r6K+KnSWS8gxP2mqCr5GYs6fwOpZcZ rYA+/k5N9w4HHIMF6jMu6msF7Ij1vFzsWJp9IJlSkCs3Rb80e4/0795bQQ0iVjMRiDHi sfpQ== X-Gm-Message-State: APf1xPCnynpj1J3Znj0oDZSfXXtieO09O2p7tpvYPlFadh2ZoXFcI0Tc u4WZ/5jGKzSiuR5xUS8pRCUmMIU1pjrN/nkz4x/Mbw== X-Google-Smtp-Source: AG47ELuStka5tA1CrsRhNl02W4v4aB6GvdYwdQPl8Dr2MuyQiusnL9675oOXSEgHDAjHvkuWKB6TJL+C3KEIaeyGTH0= X-Received: by 10.107.38.80 with SMTP id m77mr36768941iom.69.1520610881525; Fri, 09 Mar 2018 07:54:41 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Fri, 09 Mar 2018 15:54:31 +0000 Message-ID: Subject: Re: pgAdmin 4 commit: Support EXPLAIN on Greenplum. Fixes #3097 To: Dave Page Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a1140c2846f7b370566fccfd6" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a1140c2846f7b370566fccfd6 Content-Type: text/plain; charset="UTF-8" Hello, Definitely running single tests is something that would be great, specially if you are TDDing something waiting 30-40 seconds to get feedback is a little cumbersome when the test you are concerned with take less then a second. In the process: 1. Write a test 2. Make the test pass 3. Refactor in between each step you run the test more then 1 time, and depending on the refactoring you might need to run it several times. So imagine waiting 30 seconds per run to get results. To run a subset of tests is a pain because you need to be always changing the way you run the tests..... I believe we could archive a better granularity and choosing what test to run if we used a runner like pytest or nose to do it. What was the reason behind handrolling a test runner script? I am asking this because in a previous job I decided to handroll a unittest loader script and that was something that I regretted every time I had to touch it, and eventually was in the process of changing it to pytest. I looked into pytest to replace the current the current runtest, and the major problem I found was the testscenarios integration(See Note 1). It can be done but we would need to change all the test functions to receive the scenario variables through arguments on the function. Also didn't dug much into setting all the variables that we need there and all the environment. The other issue that I do not like very much about pytest is the fact that you loose the unittest assertion that is not so bad because there are some neat libraries like: https://github.com/grappa-py/grappa, https://github.com/ActivisionGameScience/assertpy, https://github.com/dgilland/verify. Personally I really like the syntax of Grapa, but the Veridfy one is pretty similar to Jasmine too. What are your thoughts? Note 1: As an example of what our functions would have to look like you can see: https://github.com/OriMenashe/pytest-scenario/blob/master/tests/test_parametrize.py As a example this class: class ServersWithServiceIDAddTestCase(BaseTestGenerator): """ This class will add the servers under default server group. """ scenarios = [ # Fetch the default url for server object ( 'Default Server Node url', dict( url='/browser/server/obj/' ) ) ] def setUp(self): pass def runTest(self): """ This function will add the server under default server group.""" url = "{0}{1}/".format(self.url, utils.SERVER_GROUP) # Add service name in the config self.server['service'] = "TestDB" response = self.tester.post( url, data=json.dumps(self.server), content_type='html/json' ) self.assertEquals(response.status_code, 200) response_data = json.loads(response.data.decode('utf-8')) self.server_id = response_data['node']['_id'] def tearDown(self): """This function delete the server from SQLite """ utils.delete_server_with_api(self.tester, self.server_id) Would have to look changed to: class ServersWithServiceIDAddTestCase(object): """ This class will add the servers under default server group. """ scenarios = [ # Fetch the default url for server object ( 'Default Server Node url', dict( url='/browser/server/obj/' ) ) ] def setUp(self): pass def runTest(self, url): """ This function will add the server under default server group.""" url = "{0}{1}/".format(url, utils.SERVER_GROUP) # Add service name in the config self.server['service'] = "TestDB" response = self.tester.post( url, data=json.dumps(self.server), content_type='html/json' ) self.assertEquals(response.status_code, 200) response_data = json.loads(response.data.decode('utf-8')) self.server_id = response_data['node']['_id'] def tearDown(self): """This function delete the server from SQLite """ utils.delete_server_with_api(self.tester, self.server_id) Thanks Joao On Fri, Mar 9, 2018 at 8:31 AM Dave Page wrote: > On Thu, Mar 8, 2018 at 2:22 PM, Joao De Almeida Pereira < > jdealmeidapereira@pivotal.io> wrote: > >> Hello Khushboo, >> Completely forgot about this python "feature"....... >> Attached is the fix. >> > > Thanks, applied. > > >> >> Just as a side question, does anyone else feel the pain of wanting to run >> a single test using a IDE or the command line and not being able to? >> > > Not really - the Python and JS tests are so quick I don't really care (and > with the Python ones, I can execute for a single module for even more > speed). > > What I would *really* like, is the ability to run individual feature > tests. That would be very valuable and save me a ton of time. > > > >> We an HandRolled the loader, and that as some implications. Did anyone >> try to use a different launcher like pytest or nose instead of the current >> runner? >> I understand that testscenarios is one of the problems we have if we want >> to move away from this way of running tests. >> Any suggestion? >> >> Thanks >> Joao >> >> On Wed, Mar 7, 2018 at 11:41 PM Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> Hi Joao, >>> >>> In the test_start_running_query.py, 2 static methods (is_begin_required_for_sql_query >>> and is_rollback_statement_required) >>> of StartRunningQuery class were used directly without @patch. Due to >>> this, in all the cases, the original value of them doesn't restore. >>> >>> To fix this, I have sent the patch in another thread, to restore its >>> original state, but I wonder if we can use these methods with @patch. >>> >>> Thanks, >>> Khushboo >>> >>> >>> On Fri, Feb 9, 2018 at 5:24 PM, Dave Page wrote: >>> >>>> Support EXPLAIN on Greenplum. Fixes #3097 >>>> >>>> - Extract SQLEditor.execute and SQLEditor._poll into their own files >>>> and add test around them >>>> - Extract SQLEditor backend functions that start executing query to >>>> their own files and add tests around it >>>> - Move the Explain SQL from the front-end and now pass the Explain >>>> plan parameters as a JSON object in the start query call. >>>> - Extract the compile_template_name into a function that can be used >>>> by the different places that try to select the version of the template and >>>> the server type >>>> >>>> Branch >>>> ------ >>>> master >>>> >>>> Details >>>> ------- >>>> >>>> https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=e16a95275336529a734bf0066889e39cc8ef0662 >>>> Author: Joao Pedro De Almeida Pereira >>>> >>>> Modified Files >>>> -------------- >>>> .../databases/schemas/tables/tests/test_utils.py | 0 >>>> web/pgadmin/static/js/sqleditor/execute_query.js | 287 ++++ >>>> .../js/sqleditor/is_new_transaction_required.js | 14 + >>>> .../static/js/sqleditor/query_tool_actions.js | 33 +- >>>> web/pgadmin/tools/sqleditor/__init__.py | 396 +---- >>>> web/pgadmin/tools/sqleditor/static/js/sqleditor.js | 227 +-- >>>> .../sqleditor/sql/10_plus/explain_plan.sql | 23 + >>>> .../sqleditor/sql/9.2_plus/explain_plan.sql | 20 + >>>> .../sqleditor/sql/default/explain_plan.sql | 17 + >>>> .../sqleditor/sql/gpdb_5.0_plus/explain_plan.sql | 5 + >>>> web/pgadmin/tools/sqleditor/tests/__init__.py | 8 + >>>> .../sqleditor/tests/test_explain_plan_templates.py | 152 ++ >>>> .../test_extract_sql_from_network_parameters.py | 59 + >>>> .../tools/sqleditor/tests/test_start_query_tool.py | 38 + >>>> web/pgadmin/tools/sqleditor/utils/__init__.py | 14 + >>>> .../sqleditor/utils/apply_explain_plan_wrapper.py | 24 + >>>> .../tools/sqleditor/utils/constant_definition.py | 32 + >>>> .../tools/sqleditor/utils/is_begin_required.py | 169 ++ >>>> .../tools/sqleditor/utils/start_running_query.py | 172 ++ >>>> .../tools/sqleditor/utils/tests/__init__.py | 8 + >>>> .../utils/tests/test_apply_explain_plan_wrapper.py | 121 ++ >>>> .../utils/tests/test_start_running_query.py | 445 +++++ >>>> .../utils/update_session_grid_transaction.py | 18 + >>>> web/pgadmin/utils/compile_template_name.py | 17 + >>>> .../utils/tests/test_compile_template_name.py | 34 + >>>> web/pgadmin/utils/versioned_template_loader.py | 2 +- >>>> web/regression/javascript/fake_endpoints.js | 6 +- >>>> .../javascript/sqleditor/execute_query_spec.js | 1702 >>>> ++++++++++++++++++++ >>>> .../sqleditor/is_new_transaction_required_spec.js | 65 + >>>> .../sqleditor/query_tool_actions_spec.js | 141 +- >>>> 30 files changed, 3670 insertions(+), 579 deletions(-) >>>> >>>> >>> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --001a1140c2846f7b370566fccfd6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello,
Definitely running single tests is something th= at would be great, specially if you are TDDing something waiting 30-40 seco= nds to get feedback is a little cumbersome when the test you are concerned = with take less then a second.

In the process:
1. Write a test
2. Make the test pass
3. Refactor=

in between each step you run the test more then 1= time, and depending on the refactoring you might need to run it several ti= mes. So imagine waiting 30 seconds per run to get results. To run a subset = of tests is a pain because you need to be always changing the way you run t= he tests.....

I believe we could archive a better = granularity and choosing what test to run if we used a runner like pytest o= r nose to do it. What was the reason behind handrolling a test runner scrip= t? I am asking this because in a previous job I decided to handroll a unitt= est loader script and that was something that I regretted every time I had = to touch it, and eventually was in the process of changing it to pytest.

I looked into pytest to replace the current the curr= ent runtest, and the major problem I found was the testscenarios integratio= n(See Note 1). It can be done but we would need to change all the test func= tions to receive the scenario variables through arguments on the function. = Also didn't dug much into setting all the variables that we need there = and all the environment.
The other issue that I do not like very = much about pytest is the fact that you loose the unittest assertion that is= not so bad because there are some neat libraries like:=C2=A0https://github.com/grappa-py/grappa,= =C2=A0https:/= /github.com/ActivisionGameScience/assertpy,=C2=A0https://github.com/dgilland/verify. Personally= I really like the syntax of Grapa, but the Veridfy one is pretty similar t= o Jasmine too.

What are your thoughts?
<= br>


Note 1: As an example of what o= ur functions would have to look like you can see:=C2=A0https://github.com/OriMenashe/pytest-scenario/blob/master/tests/test_param= etrize.py
As a example this class:
c=
lass ServersWithServiceIDAddTestCase(BaseTestGenerator):
    """ This =
class will add the servers under default server group. """
=
    scenarios=
 =3D [
        # Fetch the defaul=
t url for server object
  =
      (
            'De=
fault Server Node url', dict(
               =
 url=3D'/browser/server/obj/'
            )
        )
    ]

    def setUp(self):<=
br>        pass

    def runTest(self):
        """ This function will=
 add the server under default server group."""
        url =3D =
"{0}{1}/".format(self.url, utils.SERVER_GROUP)
        # Add service name in the config
        self.server['service'=
;] =3D "TestDB"
<=
/span>        response =3D self.tester.post(
            =
url,
            dat=
a=3Djson.dumps(self.ser=
ver),
            co=
ntent_type=3D'html/json=
9;
        )
    =
    self.assertEquals(response=
.status_code, 200)
        response_data =3D json.loads(=
response.data.decode('utf-8'<=
/span>))
        self.serve=
r_id =3D response_data['node'=
]['_id']
    def tearDown(sel=
f):
        """This function delete the server from SQLite ""=
;"
  =
      utils.delete_server_with_api(self.tester, self.server_id)
Wou= ld have to look changed to:
class ServersWi=
thServiceIDAddTestCase(object=
):
    "&quo=
t;" This class will add the servers under default server group. "=
""

    scenarios =3D [
        # Fe=
tch the default url for server object
        (
            'Default Server Node url', dict(
 =
               url=3D'/browser/server/obj/'
            )
        )
    ]<=
br>
    def setUp(=
self):
        pass

    def runTes=
t(self, url):
        """ This function will add the se=
rver under default server group."""
        url =3D "{0}{1}/".format(url, utils.SERVER_GROUP)
        # Add service name in the config
<=
span style=3D"color:rgb(128,128,128)">        self.server[&#=
39;service'] =3D "Tes=
tDB"
        re=
sponse =3D self.tester.post(            url,
            data=3Djson.dumps(s=
elf.server),
            content_type=3D=
9;html/json'
        )
        self.assertE=
quals(response.status_code, 200)
        response_data =
=3D json.loads(response.data.decode(&=
#39;utf-8'))
        se=
lf.server_id =3D response_data['node']['_id'=
]

    def tearDown(self):
        """This function delete the server from SQL=
ite """
        utils.delete_server_with_api(self.tester, self.server_id)


Thanks
Joao
<= /div>

On Fri, Mar 9, 2= 018 at 8:31 AM Dave Page <dpage@pga= dmin.org> wrote:
On Thu, Mar 8, = 2018 at 2:22 PM, Joao De Almeida Pereira <jdealmeidapereira@piv= otal.io> wrote:
Hello Khushboo,
Completely forgot about this python "feature= ".......
Attached is the fix.
<= br>
Thanks, applied.
=C2=A0=

J= ust as a side question, does anyone else feel the pain of wanting to run a = single test using a IDE or the command line and not being able to?

Not really - the Python a= nd JS tests are so quick I don't really care (and with the Python ones,= I can execute for a single module for even more speed).

What I would *really* like, is the ability to run individual feature= tests. That would be very valuable and save me a ton of time.
<= /div>

=C2=A0
We an HandRolled the loader, and that as some implications= . Did anyone try to use a different launcher like pytest or nose instead of= the current runner?=C2=A0
I understand that testscenarios is one= of the problems we have if we want to move away from this way of running t= ests.
Any suggestion?

Thanks
Joao

On Wed, Mar 7, 2018 at 11:41 PM Khushboo = Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Joao,

In the=C2=A0test_start_running_query.py, 2 static methods (is_begin_required_for_sql_query and is_roll= back_statement_required)
of StartRunningQuery class were used directly without @patch. = Due to this, in all the cases, the original value of them d= oesn't restore.

= To fix this, I have sent the patch in another thread, to re= store its original state, but I wonder if we can use these methods with @pa= tch.

Thanks,
Khushboo


On Fri, = Feb 9, 2018 at 5:24 PM, Dave Page <dpage@pgadmin.org> wrote:=
Support EXPLAIN on Greenplum. Fixes #309= 7

=C2=A0- Extract SQLEditor.execute and SQLEditor._poll into their own files = and add test around them
=C2=A0- Extract SQLEditor backend functions that start executing query to t= heir own files and add tests around it
=C2=A0- Move the Explain SQL from the front-end and now pass the Explain pl= an parameters as a JSON object in the start query call.
=C2=A0- Extract the compile_template_name into a function that can be used = by the different places that try to select the version of the template and = the server type

Branch
------
master

Details
-------
https://git.postgresql.org/gitweb?p=3Dpgadmin4.git;a=3Dcommitdi= ff;h=3De16a95275336529a734bf0066889e39cc8ef0662
Author: Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io>

Modified Files
--------------
.../databases/schemas/tables/tests/test_utils.py=C2=A0 =C2=A0|=C2=A0 =C2=A0= 0
web/pgadmin/static/js/sqleditor/execute_query.js=C2=A0 =C2=A0|=C2=A0 287 ++= ++
.../js/sqleditor/is_new_transaction_required.js=C2=A0 =C2=A0 |=C2=A0 =C2=A0= 14 +
.../static/js/sqleditor/query_tool_actions.js=C2=A0 =C2=A0 =C2=A0 |=C2=A0 = =C2=A033 +-
web/pgadmin/tools/sqleditor/__init__.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 |=C2=A0 396 +----
web/pgadmin/tools/sqleditor/static/js/sqleditor.js |=C2=A0 227 +--
.../sqleditor/sql/10_plus/explain_plan.sql=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 =C2=A023 +
.../sqleditor/sql/9.2_plus/explain_plan.sql=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 =C2=A020 +
.../sqleditor/sql/default/explain_plan.sql=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 =C2=A017 +
.../sqleditor/sql/gpdb_5.0_plus/explain_plan.sql=C2=A0 =C2=A0|=C2=A0 =C2=A0= 5 +
web/pgadmin/tools/sqleditor/tests/__init__.py=C2=A0 =C2=A0 =C2=A0 |=C2=A0 = =C2=A0 8 +
.../sqleditor/tests/test_explain_plan_templates.py |=C2=A0 152 ++
.../test_extract_sql_from_network_parameters.py=C2=A0 =C2=A0 |=C2=A0 =C2=A0= 59 +
.../tools/sqleditor/tests/test_start_query_tool.py |=C2=A0 =C2=A038 +
web/pgadmin/tools/sqleditor/utils/__init__.py=C2=A0 =C2=A0 =C2=A0 |=C2=A0 = =C2=A014 +
.../sqleditor/utils/apply_explain_plan_wrapper.py=C2=A0 |=C2=A0 =C2=A024 +<= br> .../tools/sqleditor/utils/constant_definition.py=C2=A0 =C2=A0|=C2=A0 =C2=A0= 32 +
.../tools/sqleditor/utils/is_begin_required.py=C2=A0 =C2=A0 =C2=A0|=C2=A0 1= 69 ++
.../tools/sqleditor/utils/start_running_query.py=C2=A0 =C2=A0|=C2=A0 172 ++=
.../tools/sqleditor/utils/tests/__init__.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 =C2=A0 8 +
.../utils/tests/test_apply_explain_plan_wrapper.py |=C2=A0 121 ++
.../utils/tests/test_start_running_query.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 445 +++++
.../utils/update_session_grid_transaction.py=C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2= =A0 =C2=A018 +
web/pgadmin/utils/compile_template_name.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 =C2=A017 +
.../utils/tests/test_compile_template_name.py=C2=A0 =C2=A0 =C2=A0 |=C2=A0 = =C2=A034 +
web/pgadmin/utils/versioned_template_loader.py=C2=A0 =C2=A0 =C2=A0|=C2=A0 = =C2=A0 2 +-
web/regression/javascript/fake_endpoints.js=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 =C2=A0 6 +-
.../javascript/sqleditor/execute_query_spec.js=C2=A0 =C2=A0 =C2=A0| 1702 ++= ++++++++++++++++++
.../sqleditor/is_new_transaction_required_spec.js=C2=A0 |=C2=A0 =C2=A065 +<= br> .../sqleditor/query_tool_actions_spec.js=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0|=C2=A0 141 +-
30 files changed, 3670 insertions(+), 579 deletions(-)


=


--
Dave Page
Blog: = http://pgsnake.bl= ogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company
--001a1140c2846f7b370566fccfd6--