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*
RM_4202.patch
Description: Binary data
