Re: Re: Server side cursor limitations for on demand loading of data in query tool [RM2137] [pgAdmin4]

From: Harshal Dhumal <harshal(dot)dhumal(at)enterprisedb(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: Re: Server side cursor limitations for on demand loading of data in query tool [RM2137] [pgAdmin4]
Date: 2017-06-08 13:10:28
Message-ID: CAFiP3vzomo+37TZ87tVr4AsmUNzzsFQkq8j8fEXNKdpVj9bOxw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Ignore this patch.
Rebase and migration of feature tests and jasmine tests required.

--
*Harshal Dhumal*
*Sr. Software Engineer*

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 8, 2017 at 3:56 PM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:

> Hi,
> Please find attached updated patch for feature RM2137.
>
> Changes in this patch:
> 1. Patch rebased.
>
> 2. Updated existing feature tests which requires changes due to this
> feature.
> affected feature test cases:
> i. PGDataypeFeatureTest
> ii. CheckForXssFeatureTest
>
> 3. Updated existing jasmine test cases which requires changes due to this
> feature.
> affected jasmine test cases:
> i. copy data
> ii. range_boundary_navigator
> iii. row_selector
> iv. set_stages_rows
>
> 4. New feature tests added
> i. on demand result set on scrolling.
> ii. on demand result set on grid select all.
> iii. on demand result set on column select all.
> iv. explain query
> v. explain query with verbose
> vi. explain query with costs
> vii. explain analyze query
> viii. explain analyze query with buffers
> ix. explain analyze query with timing
> x. auto commit disabled.
> xi. auto commit enabled.
> xii. auto rollback enabled.
> xiii. cancel query.
>
>
>
> --
> *Harshal Dhumal*
> *Sr. Software Engineer*
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Tue, May 16, 2017 at 8:14 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>>
>>
>> On Mon, May 15, 2017 at 7:40 PM, Harshal Dhumal <
>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> On Sat, May 13, 2017 at 12:35 AM, Joao Pedro De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> We were only able to apply the patch on 1f903ba2 (were seeing patch
>>>> does not apply due to sqleditor.js conflicts)
>>>> The javascript tests passed, but we were unable to copy rows or columns
>>>> or cells when running the application. Could you run feature tests?
>>>>
>>> There are three modes sqleditor can be launched
>>> 1. Query tool (Tools menus -> Query Tool)
>>> 2. Datagrid. (Right click on any table/view -> View Data -> View
>>> All/First 100/Last 100/Filtered rows)
>>> 3. Scripts (Right click on any table/view ->
>>> INSERT/CREATE/UPDATE/DELETE/SELECT)
>>>
>>> Paste functionality is only enabled in Datagrid and table has Primary
>>> key otherwise it's disabled. In your case row might have been copied but
>>> you were unable paste because you might be trying to paste the rows in
>>> Query tool. Please try again in Datagrid with table having Primary key.
>>>
>>>
>>>>
>>>> Now that more functionality is being added to sqleditor.js, this may be
>>>> a good time to extract the functionality to separate files. This will
>>>> increase readability, and encourage separation of concerns. It will also
>>>> make changes easier to test in isolation.
>>>>
>>> Ok. Let me check if I can separate out ant functionalities.
>>>
>>>
>>>> It's probably a good idea to test the changes made to the python as
>>>> well as javascript code. In this case, the new behavior of poll() in
>>>> sqleditor __init__ should be tested.
>>>>
>>>
>>> At this point we don't have any python unit tests that can test
>>> sqleditor backend (python code).
>>> @Dave should I include python unit test cases in this patch?
>>>
>>
>> We do have some feature tests that hit the query tool - Murtuza wrote
>> some anti-XSS validation tests for example, and Khushboo has been working
>> on some datatype rendering tests.
>>
>> As a general rule, I prefer we focus more on feature tests now than the
>> API tests - they cover the whole app end-to-end of course. The
>> disadvantages are:
>>
>> - The treeview isn't reliable enough for me to enable those tests on the
>> CI server yet.
>>
>> - They can take a long time to run, so we need to test multiple things at
>> once wherever possible. That means minimising browser reloads, or new
>> instances of tools like the Query Tool - or even the number of queries
>> executed through the query tool as part of a test.
>>
>> That said, yes, if there are specific things that are not covered by
>> Murtuza and Khushboo's work, we should test them. For example, loading all
>> rows when the user selects all, running/rendering EXPLAIN, auto-commit vs.
>> auto-rollback (and combinations thereof).
>>
>> The standard moving forwards should be to include feature tests for new
>> functionality and Jasmine tests for algorithmic JS code.
>>
>> I also agree with Joao on the modularisation of JS code. Testable and
>> reusable code should be in "library" files, and we should work to minimise
>> the amount of JS templates - for the most part, that means moving to the
>> client-side translation mechanism which Tira worked on, and I've done some
>> early migration work.
>>
>> Thanks.
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Dave Page 2017-06-08 13:15:41 Re: [pgAdmin4] [PATCH] History Tab rewrite in React
Previous Message Harshal Dhumal 2017-06-08 12:59:55 Re: Fix for RM2421 [pgAdmin4][patch]