[pgAdmin4][Patch]: RM 4360 Debugger buttons should be disabled until it is completely started
Hi Hackers, Attached is the patch to fix RM #4360 Debugger buttons should be disabled until it is completely started. Patch contains following fix apart from the actual RM: - Fixed issue when restart the execution of debugger, it is a regression of #4329 - Fixed issue while change in preference dialog for debugger but that will not reflect real time. Please review it. -- *Thanks & Regards* *Akshay Joshi* *Sr. Software Architect* *EnterpriseDB Software India Private Limited* *Mobile: +91 976-788-8246* RM_4360.patch Description: Binary data
Re: [GSoC][Patch] Automatic Mode Detection V1
Hi [please keep the maililng list CC'd] On Mon, Jun 17, 2019 at 3:05 PM Yosry Muhammad wrote: > >> Do you want me to ask our design guy for an icon? >> > > That would be great to keep things clear and separated for the users. > I've asked Chethana (CC'd) to create one. > Please find attached a patch to fix the problem that happened with you. > The problem is that I edited the primary_keys.sql file in > web/tools/sqleditor/templates/sqleditor/sql/default/ only, while there was > another one in /templates/sqleditor/sql/11_plus/. I wonder what happens > with versions before 11? are the scripts in the default/ folder used if > they are not found in that version folder? > > The patch also removes a few unnecessary lines of code that I found, not > related to the problem. > Ahh, yes - that works :-). I haven't done a detailed code review yet as you're going to be whacking things around for a bit, but I didn't see any obvious styling issues except for: (pgadmin4) dpage@hal:~/git/pgadmin4$ make check-pep8 pycodestyle --config=.pycodestyle docs/ pycodestyle --config=.pycodestyle pkg/ pycodestyle --config=.pycodestyle web/ web/pgadmin/tools/sqleditor/__init__.py:440: [E125] continuation line with same indent as next logical line web/pgadmin/tools/sqleditor/command.py:929: [E501] line too long (80 > 79 characters) web/pgadmin/tools/sqleditor/command.py:977: [W391] blank line at end of file web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:53: [E501] line too long (92 > 79 characters) web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:74: [E501] line too long (80 > 79 characters) web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:81: [E501] line too long (97 > 79 characters) web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:83: [E501] line too long (84 > 79 characters) 1 E125 continuation line with same indent as next logical line 5 E501 line too long (80 > 79 characters) 1 W391 blank line at end of file 7 make: *** [check-pep8] Error 1 All patches need to pass that (and all other) existing tests before they can be committed. Aside from that: - When revising patches, please send an updated one for the whole thing, rather than incremental ones. Incrementals are more work to apply and don't give us any benefit in return. - We need to add a "do you want to continue" warning before actions like Execute or EXPLAIN are run, if there are unsaved changes in the grid. - I think we should make the text in any cells that has been edited bold until saved, so the user can see where changes have been made (as they can with deleted rows). - If I make two data edits and then delete a row, I get 3 entries in the History panel, all showing the same delete. I would actually argue that data edit queries that pgAdmin generates should not go into the History at all, but maybe they should be added albeit with a flag to say they're internal queries and an option to hide them. Thoughts? - We need to think about how data editing fits in with transaction control. Right now, it seems to happen entirely outside of it - for example, I tend to work with auto commit turned off, so my connection sits idle-in-transaction following an initial select, and remains un-affected by edits. Please think about this and suggest options for us to discuss. > - What documentations or unit tests should I write? any guidelines here would be appreciated. >>> >> We're aiming to add unit tests to give as much coverage as possible, >> focussing on Jasmine, Python/API and then feature tests in that order (fast >> -> slow execution, which is important). So we probably want >> >> - one feature test to do basic end-to-end validation >> - Python/API tests to exercise is_query_resultset_updatable, >> save_changed_data and anything else that seems relevant. >> - Jasmine tests to ensure buttons are enabled/disabled as they should be, >> and that primary key and updatability data is tracked properly (this may >> not be feasible, but I'd still like it to be investigated and justified if >> not). >> >> We're also a day or two away from committing a new test suite for >> exercising CRUD operations and the resulting reverse engineered SQL; if we >> can utilise that to test primary_keys.sql, that'd be good. >> >> > I am sorry but I don't understand what should be done exactly in those > tests. Could you tell me where I can look at examples for feature tests, > Python/API tests and Jasmine tests (preferably for features related to the > query tool)? > They're all over the codebase to be honest. Some examples though: Varions Jasmine tests: web/regression/javascript (e.g. history, slickgrid, sqleditor) Various API tests: web/pgadmin/tools/sqleditor/tests Feature tests: web/pgadmin/feature_tests (e.g. query_tool_*) > > >> Once the in-place editing works, we'll need to rip out all the code >> related to the View/Edit data mode of the query tool. For example, there >> will be
pgAdmin 4 commit: Add a framework for testing reversed engineered SQL a
Add a framework for testing reversed engineered SQL and CRUD API endpoints. Fixes #4202 Branch -- master Details --- https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=9e65c971a35620afd1354e9ff7e049b2dcb705b2 Author: Akshay Joshi Modified Files -- Makefile | 3 + docs/en_US/release_notes_4_9.rst | 1 + .../casts/tests/default/alter_implicit_cast.sql| 9 + .../casts/tests/default/create_implicit_cast.sql | 7 + .../databases/casts/tests/default/test.json| 68 +++ .../databases/schemas/collations/tests/__init__.py | 3 +- .../databases/schemas/synonyms/tests/__init__.py | 3 +- .../databases/schemas/views/tests/__init__.py | 3 +- web/pgadmin/utils/route.py | 42 +++- web/regression/README | 3 + web/regression/re_sql/__init__.py | 0 web/regression/re_sql/tests/__init__.py| 0 web/regression/re_sql/tests/test_resql.py | 225 + web/regression/runtests.py | 7 + 14 files changed, 359 insertions(+), 15 deletions(-)
Re: [pgAdmin4][Patch]: Feature #4202 Implement new framework to test Reverse Engineering SQL
Thanks - applied! On Tue, Jun 18, 2019 at 7:04 AM Akshay Joshi 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 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 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 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 > "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" >: ""*. >- Create a output file with any name in the same directory >where the JSON file resides and specify the parameter > "*expected_sql_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 , 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 o
pgAdmin 4 commit: Ensure the debugger control buttons are only enabled
Ensure the debugger control buttons are only enabled once initialisation is complete. Fixes #4360 Branch -- master Details --- https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=b36d5d153bb77141b5f40634c2ea2a03601acaab Author: Akshay Joshi Modified Files -- docs/en_US/release_notes_4_9.rst | 3 +- web/pgadmin/tools/debugger/__init__.py | 4 +- web/pgadmin/tools/debugger/static/js/direct.js | 102 +++-- .../tools/debugger/templates/debugger/direct.html | 19 ++-- 4 files changed, 52 insertions(+), 76 deletions(-)
Re: [pgAdmin4][Patch]: RM 4360 Debugger buttons should be disabled until it is completely started
Thanks, patch applied. On Tue, Jun 18, 2019 at 9:52 AM Akshay Joshi wrote: > Hi Hackers, > > Attached is the patch to fix RM #4360 Debugger buttons should be disabled > until it is completely started. Patch contains following fix apart from the > actual RM: > >- Fixed issue when restart the execution of debugger, it is a >regression of #4329 >- Fixed issue while change in preference dialog for debugger but that >will not reflect real time. > > > Please review it. > > -- > *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
Re: [GSoC][Patch] Automatic Mode Detection V1
Hi, I have been working all day to try to make this patch applicable. On Tue, Jun 18, 2019 at 3:05 PM Dave Page wrote: > Hi > > [please keep the maililng list CC'd] > > On Mon, Jun 17, 2019 at 3:05 PM Yosry Muhammad wrote: > >> >>> Do you want me to ask our design guy for an icon? >>> >> >> That would be great to keep things clear and separated for the users. >> > > I've asked Chethana (CC'd) to create one. > Waiting for the icon, will set it up once it is ready. > > >> Please find attached a patch to fix the problem that happened with you. >> The problem is that I edited the primary_keys.sql file in >> web/tools/sqleditor/templates/sqleditor/sql/default/ only, while there was >> another one in /templates/sqleditor/sql/11_plus/. I wonder what happens >> with versions before 11? are the scripts in the default/ folder used if >> they are not found in that version folder? >> >> The patch also removes a few unnecessary lines of code that I found, not >> related to the problem. >> > > Ahh, yes - that works :-). I haven't done a detailed code review yet as > you're going to be whacking things around for a bit, but I didn't see any > obvious styling issues except for: > > (pgadmin4) dpage@hal:~/git/pgadmin4$ make check-pep8 > pycodestyle --config=.pycodestyle docs/ > pycodestyle --config=.pycodestyle pkg/ > pycodestyle --config=.pycodestyle web/ > web/pgadmin/tools/sqleditor/__init__.py:440: [E125] continuation line with > same indent as next logical line > web/pgadmin/tools/sqleditor/command.py:929: [E501] line too long (80 > 79 > characters) > web/pgadmin/tools/sqleditor/command.py:977: [W391] blank line at end of > file > web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:53: > [E501] line too long (92 > 79 characters) > web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:74: > [E501] line too long (80 > 79 characters) > web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:81: > [E501] line too long (97 > 79 characters) > web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:83: > [E501] line too long (84 > 79 characters) > 1 E125 continuation line with same indent as next logical line > 5 E501 line too long (80 > 79 characters) > 1 W391 blank line at end of file > 7 > make: *** [check-pep8] Error 1 > > All patches need to pass that (and all other) existing tests before they > can be committed. Aside from that: > > I ran pep8 checks and JS tests on this patch, however I could not run python tests due to a problem with chromedriver (working on it), please let me know if any tests fail. - When revising patches, please send an updated one for the whole thing, > rather than incremental ones. Incrementals are more work to apply and don't > give us any benefit in return. > > The attached patch is a single patch including all old and new increments. - We need to add a "do you want to continue" warning before actions like > Execute or EXPLAIN are run, if there are unsaved changes in the grid. > > - I think we should make the text in any cells that has been edited bold > until saved, so the user can see where changes have been made (as they can > with deleted rows). > Both done, new rows are highlighted too. > > - If I make two data edits and then delete a row, I get 3 entries in the > History panel, all showing the same delete. I would actually argue that > data edit queries that pgAdmin generates should not go into the History at > all, but maybe they should be added albeit with a flag to say they're > internal queries and an option to hide them. Thoughts? > That was a bug with the existing 'save changes' action of 'View Data', on which mine is based upon. I fixed the bug, now the queries are shown correctly. However, the queries are shown in the form in which they are sent from the backend to the database driver (without parameters - also an already existing bug in View Data Mode), for example: INSERT INTO public.kweek ( > media_url, username, text, created_at) VALUES ( > %(media_url)s::character varying, %(username)s::character varying, > %(text)s::text, %(created_at)s::timestamp without time zone) > returning id; > I propose two solutions: 1- Hide pgadmin's generated sql from history (in both modes). 2- Show the actual sql query that was executed after the parameters are plugged in (more understandable and potentially helpful). > - We need to think about how data editing fits in with transaction > control. Right now, it seems to happen entirely outside of it - for > example, I tend to work with auto commit turned off, so my connection sits > idle-in-transaction following an initial select, and remains un-affected by > edits. Please think about this and suggest options for us to discuss. > I integrated the data editing in the transaction control as you noted. Now the behavior is as follows: 1- In View Data mode, same existing behavior. 2- In Query Tool mode: - If auto-commit is on: the modifications are made and commited on
