Thanks - applied! On Tue, Jun 18, 2019 at 7:04 AM Akshay Joshi <akshay.jo...@enterprisedb.com> 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 <dp...@pgadmin.org> wrote: > >> >> >> On Mon, Jun 17, 2019 at 9:41 AM Akshay Joshi < >> akshay.jo...@enterprisedb.com> wrote: >> >>> Hi Dave >>> >>> On Mon, Jun 17, 2019 at 1:33 PM Dave Page <dp...@pgadmin.org> wrote: >>> >>>> >>>> >>>> On Mon, Jun 17, 2019 at 8:19 AM Ashesh Vashi < >>>> ashesh.va...@enterprisedb.com> wrote: >>>> >>>>> >>>>> On Mon, Jun 17, 2019 at 11:54 AM Akshay Joshi < >>>>> akshay.jo...@enterprisedb.com> wrote: >>>>> >>>>>> Hi Dave/Hackers >>>>>> >>>>>> On Fri, Jun 14, 2019 at 6:10 PM Akshay Joshi < >>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Jun 14, 2019 at 1:59 PM Dave Page <dp...@pgadmin.org> wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Thu, Jun 13, 2019 at 12:52 PM Akshay Joshi < >>>>>>>> akshay.jo...@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 >>>>>>>>> "<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
