Subject: [pgAdmin4][Patch]: Feature #4202 Implement new framework to test Reverse Engineering SQL

From: Akshay Joshi <[email-masked]>
Date: Thu, 13 Jun 2019 17:22:11 +0530

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
"<path_of_source>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"
   : "<SQL>"*.
   - Create a output file with any name in the same directory where the
   JSON file resides and specify the parameter "*expected_sql_file": "<name
   of the file>"*

Attached patch contains both the above mentioned examples.

Please review it.

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*
*EnterpriseDB Software India Private Limited*
*Mobile: +91 976-788-8246*

---

From: Dave Page <[email-masked]>
Date: Fri, 14 Jun 2019 09:29:31 +0100

Hi

On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi <[email-masked]>
wrote:

> 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.
>
>    JSON file resides and specify the parameter "*expected_sql_file":
>    "<name of the file>"*
>

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?
- 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.
- Once we have a way to run these tests only, please add a "make
check-resql" target to the Makefile.
- Can the expected output be formatted in the JSON such that it doesn't use
\n, but uses regular line breaks? That would make it easier to copy/paste.

Thanks.

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

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

---

From: Akshay Joshi <[email-masked]>
Date: Fri, 14 Jun 2019 18:10:06 +0530

On Fri, Jun 14, 2019 at 1:59 PM Dave Page <[email-masked]> wrote:

> Hi
>
> On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi <
> [email-masked]> wrote:
>
>> Hi Hackers
>>
>> *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.
>>

   Will fix the above.

> 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 the expected output be formatted in the JSON such that it doesn't
> use \n, but uses regular line breaks? That would make it easier to
> copy/paste.
>

    I have tried that during implementation, but JSON does not allow
line-breaks.

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*

---

From: Akshay Joshi <[email-masked]>
Date: Mon, 17 Jun 2019 11:53:54 +0530

Hi Dave/Hackers

On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi <[email-masked]>
wrote:

>>> 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.
>>>

     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 <module list>, run the API and resql test cases for
   the specified module list
   - if we run --pkg resql, run all the resql test cases only.

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*

---

From: Ashesh Vashi <[email-masked]>
Date: Mon, 17 Jun 2019 12:48:58 +0530

On Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi <[email-masked]>
wrote:

> Hi Dave/Hackers
>
> On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi <
>>>>    - Create a output file with any name in the same directory where
>>>>    the JSON file resides and specify the parameter "*expected_sql_file":
>>>>    "<name of the file>"*
>>>>
>>    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:
>
> 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.

-- Thanks, Ashesh

---

From: Dave Page <[email-masked]>
Date: Mon, 17 Jun 2019 09:02:56 +0100

On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi <[email-masked]>
wrote:

>
> On Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi <
>>>>> 10_plus or default) folder. *Run all the test scenarios present in
>>>>> the JSON file in sequential order.
>>>>>
>>>> - 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.
>>>
> * 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.

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.

---

From: Akshay Joshi <[email-masked]>
Date: Mon, 17 Jun 2019 14:11:15 +0530

Hi Dave

On Mon, Jun 17, 2019 at 1:33 PM Dave Page <[email-masked]> wrote:

>
>
> On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi <
> [email-masked]> wrote:
>
>>
>>>>>> *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.
>>>>>>
>>>    - If we run --pkg <module list>, run the API and resql test cases
>>>    for the specified module list
>>> How about using the command line options '--only-resql', and
>> '--no-resql' for the same?
>> * By default, test suite should run the test cases for reverse
>> engineering sql too.
>>
> 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. 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.

 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".

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*

---

From: Dave Page <[email-masked]>
Date: Mon, 17 Jun 2019 09:59:41 +0100

On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi <[email-masked]>
wrote:

>>>>>    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.
>>>>>
>>>>
>>>> *all*" which is used to run all the regression tests. Following will
>>>> be the scenario if we add new parameter:
>>>>
> 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.
>
> *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.

> 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).

---

From: Akshay Joshi <[email-masked]>
Date: Tue, 18 Jun 2019 11:34:33 +0530

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 <[email-masked]> wrote:

>
>
> On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi <
>>>>>>>>    - Create a output file with any name in the same directory
>>>>>>>>    where the JSON file resides and specify the parameter "*expected_sql_file":
>>>>>>>>    "<name of the file>"*
>>>>>>>>
> Right - and as I said, let's just consider them to be another package
> (i.e. do it the way you suggest).
>
> --
> Dave Page

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*

---

From: Dave Page <[email-masked]>
Date: Tue, 18 Jun 2019 14:28:56 +0100

Thanks - applied!

On Tue, Jun 18, 2019 at 7:04 AM Akshay Joshi <[email-masked]>
wrote:

>> 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.
>>
>>