Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdEAq-0004RJ-F5 for pgadmin-hackers@arkaria.postgresql.org; Tue, 18 Jun 2019 13:29:20 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hdEAp-00034E-Ax for pgadmin-hackers@arkaria.postgresql.org; Tue, 18 Jun 2019 13:29:19 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdEAo-00032p-Sv for pgadmin-hackers@lists.postgresql.org; Tue, 18 Jun 2019 13:29:19 +0000 Received: from mail-wm1-x343.google.com ([2a00:1450:4864:20::343]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdEAh-0003Tx-HI for pgadmin-hackers@postgresql.org; Tue, 18 Jun 2019 13:29:17 +0000 Received: by mail-wm1-x343.google.com with SMTP id h19so2322657wme.0 for ; Tue, 18 Jun 2019 06:29:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Mn2ZmsZNcVI+5tFQIog3gFaU85JB3P4kOHgpUA3ZjP0=; b=JcjB13/rg1mbfZw62wazY3Kh6DdMJYRFUD8J5B/J0zhhyQM+2pIgNOb3s2LQgL13NF rKo45/st3GkzH8zlLA15zi7Znf07vauskfi4mUhC8Ts7HIBvDwAai4iBJGgSthKTJrIm 5FDg2y6QMUoGbOWoE705YmkPUyVfCwjn4gseNKLFmR+MyIYC4FSm1ue/1mnbL01XHv0q TmXf5iJXeJ6h9rxVNjy4P8vvujhdhkIcTe/tQzthgQ9DlKKaRF4TWWX4FTX8W1348896 r84it8UPyz46wD/w0v3MYPQVpDcX2zNzkLeI4uD5gMZRTVG7fnm7rUcSufW61+zCD+gY f5lQ== 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=Mn2ZmsZNcVI+5tFQIog3gFaU85JB3P4kOHgpUA3ZjP0=; b=IpNRWo/amJUV6Ozi3tgriEqAGmoXRwqqBaidJgKxQLOzZsm9pIm4eRMqCLA4c4C+st DwlWavJlOiqqhnQeNfWjp91KNThqRdJ9dKZL5WIEHefeAuuVQe4C+haLNf/J2KQKk4Q9 ZDK3yZ1tUkZr0POM4uNooyofjtnyD01q9roRlMDl+OLiN0Ryp2VJJgNNxTPdb/Eg8Z4i n0FxcFP8wYslebdVbQynR/lVy1FdA5r6JXUJRtV9LhSmRWDCkZex4eKSjYvebf7T/mWX ZUcM4w+rAbrHBuO97AtTQtVbzd0yYTKDQljxEG2JZEuSDYLCodnl5mRd+USupaf3nfKB BEeQ== X-Gm-Message-State: APjAAAXYfQf2ZdX+APGh4J+muAHhvEGbwCypg3O6UG2YfhkVNi8crI3d zuRWVIXEdCrWDdEKDVU8XiPMfpecSTJ++PSwm1/IzA== X-Google-Smtp-Source: APXvYqyeT3iwgyuBd1sbnZIhSOATSeI/o7VIx0WH9mQ9yTbGAhOz9skIGSozEi7URDnzeZ4MkUuqpJRW/8OQSjGxsFQ= X-Received: by 2002:a1c:eb0a:: with SMTP id j10mr3979488wmh.1.1560864549957; Tue, 18 Jun 2019 06:29:09 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Tue, 18 Jun 2019 14:28:56 +0100 Message-ID: Subject: Re: [pgAdmin4][Patch]: Feature #4202 Implement new framework to test Reverse Engineering SQL To: Akshay Joshi Cc: Ashesh Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000000b08c7058b9919f6" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000000b08c7058b9919f6 Content-Type: text/plain; charset="UTF-8" Thanks - applied! On Tue, Jun 18, 2019 at 7:04 AM Akshay Joshi wrote: > Hi Dave/Hackers > > Attached is the modified patch to fix the given review comments. Please > review it. > > On Mon, Jun 17, 2019 at 2:29 PM Dave Page wrote: > >> >> >> On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi < >> akshay.joshi@enterprisedb.com> wrote: >> >>> Hi Dave >>> >>> On Mon, Jun 17, 2019 at 1:33 PM Dave Page wrote: >>> >>>> >>>> >>>> On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi < >>>> ashesh.vashi@enterprisedb.com> wrote: >>>> >>>>> >>>>> On Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi < >>>>> akshay.joshi@enterprisedb.com> wrote: >>>>> >>>>>> Hi Dave/Hackers >>>>>> >>>>>> On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi < >>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Jun 14, 2019 at 1:59 PM Dave Page wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi < >>>>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Hackers >>>>>>>>> >>>>>>>>> I have implemented the new test framework to test the Reverse >>>>>>>>> Engineering SQL. I have integrated it as a part of API/Regression test >>>>>>>>> suite. It will work when we run all the test cases or module wise test case. >>>>>>>>> >>>>>>>>> *How it works*: Attached patch contains the generic framework to >>>>>>>>> read all the JSON files from the *tests->version based (example >>>>>>>>> 9.6_plus, 10_plus or default) folder. *Run all the test scenarios >>>>>>>>> present in the JSON file in sequential order. >>>>>>>>> >>>>>>>>> Format of the JSON file is mentioned in >>>>>>>>> "web/pgadmin/browser/server_groups/servers/databases/casts/tests/default/test.json" >>>>>>>>> >>>>>>>>> For expected SQL we will have following two options: >>>>>>>>> >>>>>>>>> - Provide the expected sql in scenario itself as parameter *"expected_sql" >>>>>>>>> : ""*. >>>>>>>>> - Create a output file with any name in the same directory >>>>>>>>> where the JSON file resides and specify the parameter "*expected_sql_file": >>>>>>>>> ""* >>>>>>>>> >>>>>>>>> Attached patch contains both the above mentioned examples. >>>>>>>>> >>>>>>>>> Please review it. >>>>>>>>> >>>>>>>> >>>>>>>> Nice! >>>>>>>> >>>>>>>> A few comments: >>>>>>>> >>>>>>>> - The scenario name should be "Reverse Engineered SQL Test Cases" >>>>>>>> - After the scenario name is output, can we output a \n so the next >>>>>>>> line isn't appended to the name? >>>>>>>> >>>>>>> >>>>>>> Will fix the above. >>>>>>> >>>>>>>> - How do we run only the re_sql tests? I tried the obvious ways >>>>>>>> (e.g. python runtests.py --pkg >>>>>>>> regression.re_sql.tests.test_resql.ReverseEngineeringSQLTestCase) but got >>>>>>>> errors. Please add an example to web/regression/README. >>>>>>>> >>>>>>> >>>>>>> It is not a pgadmin module and we have kept it in regression >>>>>>> folder, so will have to change the existing code. I have tried but facing >>>>>>> issues when run only "regression.re_sql.tests", will continue working on >>>>>>> this. >>>>>>> >>>>>> >>>>>> Can we add a new parameter to --pkg "*resql*" to run all the >>>>>> reverse engineered test cases for all the modules, it just like parameter " >>>>>> *all*" which is used to run all the regression tests. Following will >>>>>> be the scenario if we add new parameter: >>>>>> >>>>>> - If we run --pkg all, run all the API and resql test cases. >>>>>> - If we run --pkg , run the API and resql test cases >>>>>> for the specified module list >>>>>> - if we run --pkg resql, run all the resql test cases only. >>>>>> >>>>>> How about using the command line options '--only-resql', and >>>>> '--no-resql' for the same? >>>>> * If we run the test suite with '--only-resql', it should run only the >>>>> test cases for the reverse engineering sql for all or selected packages >>>>> specified by '--pkg'. >>>>> * If we run the test suite with '--no-resql', no test cases for the >>>>> reverse engineering sql should be running. >>>>> * By default, test suite should run the test cases for reverse >>>>> engineering sql too. >>>>> >>>>> NOTE: '--only-resql', and '--no-resql' must not be specified together. >>>>> >>>>> Let's leave the command line option '--pkg' for selecting the packages >>>>> only. >>>>> >>>> >>>> Why add more options? I don't see why we can't think of these tests as >>>> just another package. If that's really a problem, we could just rename it >>>> to --tests or something. >>>> >>> >>> As I mentioned in my previous email, this is not a regular >>> package/module in pgadmin directory. We have kept it in regression >>> folder. With current implementation if we provide "all" as a --pkg >>> parameter it will import all the modules where "*test.*" string is >>> present in the module name. If we provide the specific package like " >>> *browser.server_groups.servers.databases.casts.tests*" then it will >>> import all the files of that module. >>> >>> So here problem is if we specify "python runtests.py --pkg >>> *regression.re_sql.tests*" we don't have list of all the module to >>> iterate over the *tests* folder and get the JSON file. >>> >> >> Yes, I know all of that. That's why I said "*think* of these tests as >> just another package". I know they're actually not. >> >> >>> My question here is why do we need to separate the resql test cases? It >>> would be good to have if they run along with the API test case for all or >>> specified module. >>> >> >> Because the rest of the tests can take a long time to run, and it may be >> useful to just run these tests if that's what the developer wants. >> >> >>> >>> But if we will have to support it than we should have one option to >>> identify that we need to run only *re_sql* for all the modules. That we >>> can achieve by any options like I suggest "--pkg resql" or suggested by >>> Ashesh "--only-resql". >>> >> >> Right - and as I said, let's just consider them to be another package >> (i.e. do it the way you suggest). >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > -- > *Thanks & Regards* > *Akshay Joshi* > > *Sr. Software Architect* > *EnterpriseDB Software India Private Limited* > *Mobile: +91 976-788-8246* > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --0000000000000b08c7058b9919f6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks - applied!

On Tue, Jun 18, 2019 at 7:04 AM Akshay Jo= shi <akshay.joshi@enter= prisedb.com> wrote:
Hi=C2=A0Dave/Hackers

Attached is the modified patch to fix the given= review comments. Please review it.

On Mon, Jun 17, 2019 at 2:29 PM Dave Pag= e <dpage@pgadmin.= org> wrote:


On Mon, Jun 17, 2019 at 9:41 AM Ak= shay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi=C2= =A0Dave

On Mon, Jun 17, 2019 at 1:33 PM Dave Page <dpage@pgadmin.org> wrote:
<= div dir=3D"ltr">

On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi <ashesh.vashi@e= nterprisedb.com> wrote:
On Mon, Jun 17, 2019 at 11:54 AM Akshay Jos= hi <a= kshay.joshi@enterprisedb.com> wrote:
Hi=C2=A0Dave/= Hackers

On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi <akshay.joshi@enterprisedb.c= om> wrote:


On Fri, Jun 14, 2019 at 1:59 PM Dav= e Page <dpage@pga= dmin.org> wrote:
Hi

On Thu, Jun 13, 2019 at 12:52 P= M Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Hackers
I have implemented the new test framework to test the Reverse = Engineering SQL. I have integrated it as a part of API/Regression test suit= e. It will work when we run all the=C2=A0test cases or module wise test cas= e.

How it works: Attached patch contains th= e generic framework to read all the JSON files from the tests->versio= n based (example 9.6_plus, 10_plus or default) folder. Run all the test= scenarios present in the JSON file in sequential order.

Format of the JSON file is mentioned in "<path_of_source>= web/pgadmin/browser/server_groups/servers/databases/casts/tests/default/tes= t.json"

For expected SQL we will have followi= ng two options:
  • Provide the expected sql in scenario itse= lf as parameter "expected_sql" : "<SQL>".<= br>
  • Create a output file with any name in the same directory where = the JSON file resides and specify the parameter "expected_sql_file&= quot;: "<name of the file>"
Att= ached patch contains both the above mentioned examples.

Please review it.

Nice!

A few comments:

- The scen= ario name should be "Reverse Engineered SQL Test Cases"
- After the scenario name is output, can we output a \n so the next line i= sn't appended to the name?
=C2=A0 = =C2=A0
=C2=A0 =C2=A0Will fix the above.=C2=A0
- How do we run only the re_sql tests? I tried the obvious ways (= e.g.=C2=A0python runtests.py --pkg regression.re_sql.tests.test_resql.Rever= seEngineeringSQLTestCase) but got errors. Please add an example to web/regr= ession/README.

=C2=A0 =C2= =A0It is not a pgadmin module and we have kept it in regression folder, so = will have to change the existing code. I have tried but facing issues when = run only=C2=A0 "regression.re_sql.tests", will continue working o= n this.

=C2=A0 =C2=A0 =C2= =A0Can we add a new parameter=C2=A0 to --pkg "resql" to ru= n all the reverse engineered test cases for all the modules, it just like p= arameter "all" which is used to run all the regression tes= ts. Following will be the scenario if we add new parameter:
    <= li>If we run --pkg all, run all the API and resql test cases.
  • If we= run --pkg <module list>, run the API and resql test cases for the sp= ecified module list=C2=A0
  • if we run --pkg resql, run all the resql = test cases only.=C2=A0 =C2=A0=C2=A0
How about using the command line options '--only-resql', and = '--no-resql' for the same?
* If we run the test suite wit= h '--only-resql', it should run only the test cases for the reverse= engineering sql for all or selected packages specified by '--pkg'.=
* If we run the test suite with '--no-resql', no test ca= ses for the reverse engineering sql should be running.
* By defau= lt, test suite should run the test cases for reverse engineering sql too.

NOTE: '--only-resql', and '--no-resql&#= 39; must not be specified together.

Let's leav= e the command line option '--pkg' for selecting the packages only.= =C2=A0

Why add more optio= ns? I don't see why we can't think of these tests as just another p= ackage. If that's really a problem, we could just rename it to --tests = or something.=C2=A0

=C2= =A0 =C2=A0As I mentioned in my previous email, this is not a regular packag= e/module in pgadmin directory. We have kep= t it in regression folder. With current implementation if we provide "= all" as a --pkg parameter it will import all the modules where "<= b>test." string is present in the module name. If we provide the s= pecific package like "browser.server_groups.servers.databases.casts= .tests" then it will import all the files of that module.=C2=A0

=C2=A0 So here problem is if we specify "python= runtests.py --pkg regression.re_sql.tests&q= uot; we don't have list of all the module to iterate over the tests<= /b> folder and get the JSON file.

Yes, I know all of that. That= 9;s why I said "*think* of these tests as just another package". = I know they're actually not.
=C2=A0
<= div>My question here is why do we need to separate the resql test cases? It would be good to have if they run along w= ith the API test case for all or specified module.

Because the rest of the tests can take a long tim= e to run, and it may be useful to just run these tests if that's what t= he developer wants.
=C2=A0

=C2=A0But if we will have to support it than we should have one optio= n to identify that we need to run only re_sql<= /span> for all the modules. That we can achieve by any options like I s= uggest "--pkg resql" or suggest= ed by Ashesh "--only-resql".=C2=A0=C2=A0
<= div>
Right - and as I said, let's just consider them to b= e another package (i.e. do it the way you suggest).=C2=A0
<= br>
--
Dave Page
Blog: http://pgsnake.blogspot.comTwitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise= PostgreSQL Company


--
=
Thanks & Reg= ards
Akshay Joshi
Sr. Software Architect
EnterpriseDB Software India Private Limited=
Mobile: +91 976-788-8246


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @p= gsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL = Company
--0000000000000b08c7058b9919f6--