public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Ashesh Vashi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4][Patch]: Feature #4202 Implement new framework to test Reverse Engineering SQL
Date: Tue, 18 Jun 2019 14:28:56 +0100
Message-ID: <CA+OCxoyJvumqjfmS_xtHgo_ye7Snvk4niK+_K68h9__HG0QPJA@mail.gmail.com> (raw)
In-Reply-To: <CANxoLDesW5Dyvr==JF+adGJ_jtZunTX2w=iyABrS9o=1StkvpQ@mail.gmail.com>
References: <CANxoLDe_FGeAnGy9Vo22VBKRfd8-=yb4AbZcE4AbyR-tUnYgBg@mail.gmail.com>
<CA+OCxozE6mwqrBrGnQaaEjcJoyR7t8RYJGNj+RkfP0HsrYohCw@mail.gmail.com>
<CANxoLDdbQV-PP16GQUnCz0Ki8xt_P-T3DDPZEq=R6YQNxAiUQQ@mail.gmail.com>
<CANxoLDcMqJFHUBcRkWRbtKm1h5BJeUSdOgbSvmK0c+drZW19Og@mail.gmail.com>
<CAG7mmozk+V9UDEBS2_K8-iw=YxaeROWLs-eYW1oL_n1VX+oNUA@mail.gmail.com>
<CA+OCxozvxRP=urL4zVYf+rNBtoXJNj+Uh3SJEtv8gna_orF2Cw@mail.gmail.com>
<CANxoLDeLk8WoaQThGPKGziSUVmcpNzqAHFOoBtFcZmPb7Z1mXA@mail.gmail.com>
<CA+OCxoy+=j-mQ8sT6PFpEGXEtpmOyrr6=wVWeh7zSK_V0=t+WA@mail.gmail.com>
<CANxoLDesW5Dyvr==JF+adGJ_jtZunTX2w=iyABrS9o=1StkvpQ@mail.gmail.com>
Thanks - applied!
On Tue, Jun 18, 2019 at 7:04 AM Akshay Joshi <[email protected]>
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 <[email protected]> wrote:
>
>>
>>
>> On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi <
>> [email protected]> wrote:
>>
>>> Hi Dave
>>>
>>> On Mon, Jun 17, 2019 at 1:33 PM Dave Page <[email protected]> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi <
>>>> [email protected]> wrote:
>>>>
>>>>>
>>>>> On Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hi Dave/Hackers
>>>>>>
>>>>>> On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Jun 14, 2019 at 1:59 PM Dave Page <[email protected]> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi <
>>>>>>>> [email protected]> 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*
>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
view thread (10+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin4][Patch]: Feature #4202 Implement new framework to test Reverse Engineering SQL
In-Reply-To: <CA+OCxoyJvumqjfmS_xtHgo_ye7Snvk4niK+_K68h9__HG0QPJA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox