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

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 <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave
>>
>> On Mon, Jun 17, 2019 at 1:33 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>>
>>>
>>> On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi <
>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>>
>>>> On Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Dave/Hackers
>>>>>
>>>>> On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi <
>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Jun 14, 2019 at 1:59 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi <
>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)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
>>>>>>>> "<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.
>>>>>>>>
>>>>>>>
>>>>>>> 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 <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.
>>>>>
>>>>> 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*