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 1hcnUd-000083-7O for pgadmin-hackers@arkaria.postgresql.org; Mon, 17 Jun 2019 08:59:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hcnUb-0008KH-QS for pgadmin-hackers@arkaria.postgresql.org; Mon, 17 Jun 2019 08:59:57 +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 1hcnUb-0008G4-6l for pgadmin-hackers@lists.postgresql.org; Mon, 17 Jun 2019 08:59:57 +0000 Received: from mail-io1-xd41.google.com ([2607:f8b0:4864:20::d41]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hcnUX-00015e-UJ for pgadmin-hackers@postgresql.org; Mon, 17 Jun 2019 08:59:55 +0000 Received: by mail-io1-xd41.google.com with SMTP id w25so19439545ioc.8 for ; Mon, 17 Jun 2019 01:59:53 -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=sVmZN9cCgRRcLVCS4u43Alu3MXOAxkN98wmx2XjUvSg=; b=mCsJ7xqg65ly+iWYBbae128Ao/ysbxTlEkFtaxAM4XeavVju/Qik0GJMeYVhpL0vLe 3sFeZz2uSx+7EcPurOn0/zGAc4hpkunUxZ4or1kl+w3/WuAZBMntbJrNxcZgE4YEhkXM heAP41QzgE4G8Go0rBD4RyqWnmbHtarh3jM56lidFqx/Lv6v9UQ2244nMfsruneFBilv WjgKRSKHKQ0HdVQjDBsk/cUNXxuRMw2QPetnKQkGRMtGcRicvZGVWMNuxco6uI+qNvTh XvnKBWHjkw95p7Yj6WdlUhtfVpXpZXndLPzbw2WoDhAuFN0naEMGvUcfl6qlLyUxBoVb rOIA== 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=sVmZN9cCgRRcLVCS4u43Alu3MXOAxkN98wmx2XjUvSg=; b=ixl3Li7KcamXb+cR4t9CmM2YtX60JLneC1862h4WR29xZSVgaFhYS1+BZ9GS9B1gO3 YHNFOt0w/vJbutYrwZ6ahcKhnQCOj5Yw3T0MfwR5hhMe0F7e9Fwm3QoSBK4ndvRjJRkl l61mn0QQH+HzPleKF11/2Wcd6SGUystlBzmlMuQ6cwDONxY5x0Q3x9gBkQUv+/NdP4hE 7O0FzMpgA0GyEgX8TkyiuUGXWjsPOqM2E8EH8eWF8pwq9zEiOECkdoMBQ5JVDWmHlBRq 71Ma9QgD2OFnNsLZVIH3H4wu96Zizttn8Z37CeY9f624wAlpdimw/i0aC1eNDnVdBh92 dVrg== X-Gm-Message-State: APjAAAX7hfffT3OKTv7RedDlDxkR4QMWi02Vkc5rKQsJcfRkmN4/0MPD W2PQTfXqsoDhudt6x37EV9se4Doq9tGjLOUWBPqNRw== X-Google-Smtp-Source: APXvYqxNzOym3W3Mb+iFTbjaXP1tk2daksQjt3TW9uF2nggMCSFad7BQMQcLjHDdIZbRNyHxYCJOgTqOi7N3ktksZdo= X-Received: by 2002:a5d:8794:: with SMTP id f20mr10252978ion.128.1560761993104; Mon, 17 Jun 2019 01:59:53 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Mon, 17 Jun 2019 09:59:41 +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="0000000000002d9dff058b813822" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000002d9dff058b813822 Content-Type: text/plain; charset="UTF-8" On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi 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 --0000000000002d9dff058b813822 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Jun 17, 2019 at 9:41 AM Aksha= y Joshi <akshay.joshi@e= nterprisedb.com> wrote:
Hi=C2=A0Dave

On Mon, Jun 17= , 2019 at 1:33 PM Dave Page <dpage@pgadmin.org> wrote:


On Mon, Jun 17, 2019 at 8:19 AM Ashe= sh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On= Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi <akshay.joshi@enterprisedb.com&g= t; wrote:
Hi=C2=A0Dave/Hackers

On Fri, Jun 14, 2019 at 6:1= 0 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:


On Fri, Jun 14, 2019 at 1:59 PM Dave Page <dpage@pgadmin.org> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Hi

On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi <akshay.joshi@enterpris= edb.com> wrote:
Hi Hackers

I have implemented th= e 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 th= e=C2=A0test cases or module wise test case.

How= it works: Attached patch contains the generic framework to read all th= e 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 fil= e in sequential order.

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

<= div>For expected SQL we will have following two options:
  • = Provide the expected sql in scenario itself as parameter "expected_= sql" : "<SQL>".
  • Create a output file w= ith any name in the same directory where the JSON file resides and specify = the parameter "expected_sql_file": "<name of the file&= gt;"
Attached patch contains both the abov= e mentioned examples.

Please review it.

Nice!

A few comm= ents:

- The scenario name should be "Reverse = Engineered SQL Test Cases"
- After the scenario name is outp= ut, can we output a \n so the next line isn't appended to the name?
=C2=A0 =C2=A0
=C2=A0 =C2=A0Will f= ix the above.=C2=A0
=
- How do we run only the r= e_sql tests? I tried the obvious ways (e.g.=C2=A0python runtests.py --pkg r= egression.re_sql.tests.test_resql.ReverseEngineeringSQLTestCase) but got er= rors. Please add an example to web/regression/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 cod= e. I have tried but facing issues when run only=C2=A0 "regression.re_s= ql.tests", will continue working on this.

=C2=A0 =C2=A0 =C2=A0Can we add a new parameter=C2=A0 = to --pkg "resql" to run all the reverse engineered test ca= ses for all the modules, it just like parameter "all" whic= h is used to run all the regression tests. Following will be the scenario i= f we add new parameter:
  • 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 specified module list=C2=A0
  • if= we run --pkg resql, run all the resql test cases only.=C2=A0 =C2=A0=C2=A0<= /li>
How about using the command li= ne 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 suit= e with '--no-resql', no test cases for the reverse engineering sql = should be running.
* By default, test suite should run the test c= ases for reverse engineering sql too.

NOTE: '-= -only-resql', and '--no-resql' must not be specified together.<= /div>

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

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 proble= m, 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 package/module in pgadmin directory. We have kept it in regression = folder. With current implementation if we provide "all" as a --pk= g parameter it will import all the modules where "test." s= tring is present in the module name. If we provide the specific package lik= e "browser.server_groups.servers.databases.casts.tests" th= en it will import all the files of that module.=C2=A0

<= div>=C2=A0 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= 9;re actually not.
=C2=A0
My questio= n here is why do we need to separate the resql test cases? It would be good to have if they run a= long with the API test case for all or specified module.
<= /blockquote>

Because the rest of the tests can take a lo= ng time to run, and it may be useful to just run these tests if that's = what the developer wants.
=C2=A0
=C2=A0But 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 achie= ve by any options like I suggest "--pkg resql" or suggested by Ashesh "--only-resql".=C2=A0=C2=A0

Right - and as I said, let's just conside= r them to be another package (i.e. do it the way you suggest).=C2=A0
<= /div>

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

EnterpriseDB UK: <= a href=3D"http://www.enterprisedb.com" target=3D"_blank">http://www.enterpr= isedb.com
The Enterprise PostgreSQL Company
--0000000000002d9dff058b813822--