Re: pgAdmin 4 commit: Support EXPLAIN on Greenplum. Fixes #3097

On Fri, Mar 9, 2018 at 11:04 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Fri, Mar 9, 2018 at 3:54 PM, Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hello,
>> Definitely running single tests is something that would be great,
>> specially if you are TDDing something waiting 30-40 seconds to get feedback
>> is a little cumbersome when the test you are concerned with take less then
>> a second.
>>
>> In the process:
>> 1. Write a test
>> 2. Make the test pass
>> 3. Refactor
>>
>
> Sure, makes sense for development. As I spend 99% of my time reviewing and
> testing these days, I was just relaying my pain points :-)
>
>
>>
>> in between each step you run the test more then 1 time, and depending on
>> the refactoring you might need to run it several times. So imagine waiting
>> 30 seconds per run to get results. To run a subset of tests is a pain
>> because you need to be always changing the way you run the tests.....
>>
>> I believe we could archive a better granularity and choosing what test to
>> run if we used a runner like pytest or nose to do it. What was the reason
>> behind handrolling a test runner script? I am asking this because in a
>> previous job I decided to handroll a unittest loader script and that was
>> something that I regretted every time I had to touch it, and eventually was
>> in the process of changing it to pytest.
>>
>
> Pure newbie-ism. I have no objections to changing to something else, if it
> reduces our tech debt.
>
>
>>
>> I looked into pytest to replace the current the current runtest, and the
>> major problem I found was the testscenarios integration(See Note 1). It can
>> be done but we would need to change all the test functions to receive the
>> scenario variables through arguments on the function. Also didn't dug much
>> into setting all the variables that we need there and all the environment.
>> The other issue that I do not like very much about pytest is the fact
>> that you loose the unittest assertion that is not so bad because there are
>> some neat libraries like: https://github.com/grappa-py/grappa,
>> https://github.com/ActivisionGameScience/assertpy,
>> https://github.com/dgilland/verify. Personally I really like the syntax
>> of Grapa, but the Veridfy one is pretty similar to Jasmine too.
>>
>> What are your thoughts?
>>
>
> Huh, I also really like the grappa syntax. It's nice and readable.
>
>
>>
>>
>>
>> Note 1: As an example of what our functions would have to look like you
>> can see:
>> https://github.com/OriMenashe/pytest-scenario/blob/master/tests/test_parametrize.py
>> As a example this class:
>>
>
> Without a diff, it's hard to be sure, but it looks like the only change
> was BaseTestGenerator to object on the first line?
>
See the function signature, that is the cumbersome issue

>
>
>> class ServersWithServiceIDAddTestCase(BaseTestGenerator):
>> """ This class will add the servers under default server group. """
>>
>> scenarios = [
>> # Fetch the default url for server object
>> (
>> 'Default Server Node url', dict(
>> url='/browser/server/obj/'
>> )
>> )
>> ]
>>
>> def setUp(self):
>> pass
>>
>> def runTest(self):
>> """ This function will add the server under default server group."""
>> url = "{0}{1}/".format(self.url, utils.SERVER_GROUP)
>> # Add service name in the config
>> self.server['service'] = "TestDB"
>> response = self.tester.post(
>> url,
>> data=json.dumps(self.server),
>> content_type='html/json'
>> )
>> self.assertEquals(response.status_code, 200)
>> response_data = json.loads(response.data.decode('utf-8'))
>> self.server_id = response_data['node']['_id']
>>
>> def tearDown(self):
>> """This function delete the server from SQLite """
>> utils.delete_server_with_api(self.tester, self.server_id)
>>
>> Would have to look changed to:
>>
>> class ServersWithServiceIDAddTestCase(object):
>> """ This class will add the servers under default server group. """
>>
>> scenarios = [
>> # Fetch the default url for server object
>> (
>> 'Default Server Node url', dict(
>> url='/browser/server/obj/'
>> )
>> )
>> ]
>>
>> def setUp(self):
>> pass
>>
>> def runTest(self, url):
>> """ This function will add the server under default server group."""
>> url = "{0}{1}/".format(url, utils.SERVER_GROUP)
>> # Add service name in the config
>> self.server['service'] = "TestDB"
>> response = self.tester.post(
>> url,
>> data=json.dumps(self.server),
>> content_type='html/json'
>> )
>> self.assertEquals(response.status_code, 200)
>> response_data = json.loads(response.data.decode('utf-8'))
>> self.server_id = response_data['node']['_id']
>>
>> def tearDown(self):
>> """This function delete the server from SQLite """
>> utils.delete_server_with_api(self.tester, self.server_id)
>>
>>
>>
>> Thanks
>> Joao
>>
>> On Fri, Mar 9, 2018 at 8:31 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> On Thu, Mar 8, 2018 at 2:22 PM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hello Khushboo,
>>>> Completely forgot about this python "feature".......
>>>> Attached is the fix.
>>>>
>>>
>>> Thanks, applied.
>>>
>>>
>>>>
>>>> Just as a side question, does anyone else feel the pain of wanting to
>>>> run a single test using a IDE or the command line and not being able to?
>>>>
>>>
>>> Not really - the Python and JS tests are so quick I don't really care
>>> (and with the Python ones, I can execute for a single module for even more
>>> speed).
>>>
>>> What I would *really* like, is the ability to run individual feature
>>> tests. That would be very valuable and save me a ton of time.
>>>
>>>
>>>
>>>> We an HandRolled the loader, and that as some implications. Did anyone
>>>> try to use a different launcher like pytest or nose instead of the current
>>>> runner?
>>>> I understand that testscenarios is one of the problems we have if we
>>>> want to move away from this way of running tests.
>>>> Any suggestion?
>>>>
>>>> Thanks
>>>> Joao
>>>>
>>>> On Wed, Mar 7, 2018 at 11:41 PM Khushboo Vashi <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Joao,
>>>>>
>>>>> In the test_start_running_query.py, 2 static methods (is_begin_required_for_sql_query
>>>>> and is_rollback_statement_required)
>>>>> of StartRunningQuery class were used directly without @patch. Due to
>>>>> this, in all the cases, the original value of them doesn't restore.
>>>>>
>>>>> To fix this, I have sent the patch in another thread, to restore its
>>>>> original state, but I wonder if we can use these methods with @patch.
>>>>>
>>>>> Thanks,
>>>>> Khushboo
>>>>>
>>>>>
>>>>> On Fri, Feb 9, 2018 at 5:24 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Support EXPLAIN on Greenplum. Fixes #3097
>>>>>>
>>>>>> - Extract SQLEditor.execute and SQLEditor._poll into their own files
>>>>>> and add test around them
>>>>>> - Extract SQLEditor backend functions that start executing query to
>>>>>> their own files and add tests around it
>>>>>> - Move the Explain SQL from the front-end and now pass the Explain
>>>>>> plan parameters as a JSON object in the start query call.
>>>>>> - Extract the compile_template_name into a function that can be used
>>>>>> by the different places that try to select the version of the template and
>>>>>> the server type
>>>>>>
>>>>>> Branch
>>>>>> ------
>>>>>> master
>>>>>>
>>>>>> Details
>>>>>> -------
>>>>>>
>>>>>> https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=e16a95275336529a734bf0066889e39cc8ef0662
>>>>>> Author: Joao Pedro De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>
>>>>>>
>>>>>> Modified Files
>>>>>> --------------
>>>>>> .../databases/schemas/tables/tests/test_utils.py | 0
>>>>>> web/pgadmin/static/js/sqleditor/execute_query.js | 287 ++++
>>>>>> .../js/sqleditor/is_new_transaction_required.js | 14 +
>>>>>> .../static/js/sqleditor/query_tool_actions.js | 33 +-
>>>>>> web/pgadmin/tools/sqleditor/__init__.py | 396 +----
>>>>>> web/pgadmin/tools/sqleditor/static/js/sqleditor.js | 227 +--
>>>>>> .../sqleditor/sql/10_plus/explain_plan.sql | 23 +
>>>>>> .../sqleditor/sql/9.2_plus/explain_plan.sql | 20 +
>>>>>> .../sqleditor/sql/default/explain_plan.sql | 17 +
>>>>>> .../sqleditor/sql/gpdb_5.0_plus/explain_plan.sql | 5 +
>>>>>> web/pgadmin/tools/sqleditor/tests/__init__.py | 8 +
>>>>>> .../sqleditor/tests/test_explain_plan_templates.py | 152 ++
>>>>>> .../test_extract_sql_from_network_parameters.py | 59 +
>>>>>> .../tools/sqleditor/tests/test_start_query_tool.py | 38 +
>>>>>> web/pgadmin/tools/sqleditor/utils/__init__.py | 14 +
>>>>>> .../sqleditor/utils/apply_explain_plan_wrapper.py | 24 +
>>>>>> .../tools/sqleditor/utils/constant_definition.py | 32 +
>>>>>> .../tools/sqleditor/utils/is_begin_required.py | 169 ++
>>>>>> .../tools/sqleditor/utils/start_running_query.py | 172 ++
>>>>>> .../tools/sqleditor/utils/tests/__init__.py | 8 +
>>>>>> .../utils/tests/test_apply_explain_plan_wrapper.py | 121 ++
>>>>>> .../utils/tests/test_start_running_query.py | 445 +++++
>>>>>> .../utils/update_session_grid_transaction.py | 18 +
>>>>>> web/pgadmin/utils/compile_template_name.py | 17 +
>>>>>> .../utils/tests/test_compile_template_name.py | 34 +
>>>>>> web/pgadmin/utils/versioned_template_loader.py | 2 +-
>>>>>> web/regression/javascript/fake_endpoints.js | 6 +-
>>>>>> .../javascript/sqleditor/execute_query_spec.js | 1702
>>>>>> ++++++++++++++++++++
>>>>>> .../sqleditor/is_new_transaction_required_spec.js | 65 +
>>>>>> .../sqleditor/query_tool_actions_spec.js | 141 +-
>>>>>> 30 files changed, 3670 insertions(+), 579 deletions(-)
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>