Re: Feature test regression failures

On Wed, Mar 22, 2017 at 7:23 PM, Atira Odhner <aodhner(at)pivotal(dot)io> wrote:

> First - let me try to explain the problem with the failure in the
>> feature test.
>> We do not load all the javascript libraries, when starting the pgAdmin 4
>> (i.e. the loading the browser/index.html).
>> But - load them only when first tree-item of certain type is added.
>> e.g. We load the javascript module of 'table', 'index', etc only after
>> first tree item of database is added in the tree.
>
>
> I've seen this bug opening the tree by hand, so it is not merely a matter
> of opening tree items as quickly as a machine. There are two different code
> paths for loading the tree state --- the way the initial load happens upon
> expand and the call that is made when the user clicks 'Refresh...' in the
> context menu. Maybe a good approach to starting to fix this bug would be to
> have only one code path for loading tree state.
>
What do you mean by that?

> I don't think this has to do with loading the javascript modules.
>
It is - we've personally experienced the same in early phase of the
development, when the spinner control was not implemented.
We have seen that - when we expand the tree node, it does reloads the
server-groups under the server-group node, just because of the same reason.

>
>
>> But - as I said earlier, it represents the data for the tree-item (not
>> the item itself).
>> Hence - it should be 'itemData', and not 'item'.
>>
>
> 'itemData' and 'item' are both fine with me.
>
>
>> We've already used 'd' to represent the data, 'i' for item, 't' for tree
>> at all other places in pgAdmin 4.
>> So - it is consistent across the application.
>
>
> Consistency is great, but in this case, aiming for consistency is
> hindering the legibility of the application code. At some point, we should
> go through the whole application and change all the 'i' to 'item', 'd' to
> data', etc. Until then, I think it is worthwhile to sacrifice consistency
> in exchange for adding meaning and clarity to the code that is being
> updated.
>
To be honest, not in that case.
As the current choice of the alphabet tells enough about the meaning of the
code.

We're deviating from the actual point at the moment, so - let's concentrate
on that first.
I would change it to itemData.

-- Thanks, Ashesh

>
> Tira & Joao
>
> On Wed, Mar 22, 2017 at 2:20 AM, Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> --
>>
>> Thanks & Regards,
>>
>> Ashesh Vashi
>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>> <http://www.enterprisedb.com>
>>
>>
>> *http://www.linkedin.com/in/asheshvashi*
>> <http://www.linkedin.com/in/asheshvashi>
>>
>> On Tue, Mar 21, 2017 at 9:20 PM, Atira Odhner <aodhner(at)pivotal(dot)io> wrote:
>>
>>> Here is a new patchset that instead hides the spinner when the acitree
>>> has been initialized. On average, the spinner seems to disappear about 2
>>> seconds sooner, and I haven't seen flakiness with these changes yet.
>>>
>>> Tira & Joao
>>>
>>> On Mon, Mar 20, 2017 at 4:17 PM, Atira Odhner <aodhner(at)pivotal(dot)io>
>>> wrote:
>>>
>>>> Note that this patch makes the problem of the tree not having loaded
>>>> worse, because it only waits for js modules to load rather than arbitrarily
>>>> waiting 900ms.
>>>>
>>> The patch was never intended to remove the spinner earlier.
>> Idea was: the spinner should be hidden only after all dependent
>> javascript modules are loaded.
>>
>> I agree about using arbitrary wait was never a good option for sure.
>> Though - I still see the flaw in my approach.
>> If the dependent script has error, it wont get loaded, and can make the
>> things worse by not hiding the spinner at all.
>>
>> I liked the idea behind your first patch about using the tree events to
>> hide the spinner.
>> Though - I see scope of improvement in it.
>>
>> On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodhner(at)pivotal(dot)io> wrote:
>>>>
>>>>> Hi Ashesh,
>>>>>
>>>>> *Regarding your second patch:*
>>>>>
>>>>> It looks like your second patch addresses module loading. This is an
>>>>> improvement over the previous hard timeout, but won’t do anything for the
>>>>> tree issues. The module loading code can also be simplified; we’ve attached
>>>>> a patchset that is tidier, tests the behavior, and speeds up the polling.
>>>>>
>>>>> Ashesh, can you explain why you are setting the text on the spinner
>>>>> after hiding it, or why you are hiding it rather than removing it?
>>>>>
>>>> First - let me try to explain the problem with the failure in the
>> feature test.
>> We do not load all the javascript libraries, when starting the pgAdmin 4
>> (i.e. the loading the browser/index.html).
>> But - load them only when first tree-item of certain type is added.
>> e.g. We load the javascript module of 'table', 'index', etc only after
>> first tree item of database is added in the tree.
>>
>> Now - when we run the feature tests, it expands all the nodes one by one
>> very quickly.
>> And, that makes the thing worse, as the javascript module for 'table' is
>> still not loaded in the browser, definitely - not immediately after the
>> first 'database' item is added, it always takes some time to load the
>> module, and takes few fraction of seconds/milliseconds.
>>
>> Now - the intention was show the spinner, when the dependent javascript
>> modules gets loaded.
>> Hence - we did not remove the spinner, but - instead only hide it.
>>
>> And, changed the text to 'Loading the dependent resources...', so that -
>> whenever we once again show the spinner, it will show that text.
>> I am considering using the tree events to show the spinner again using
>> the similar approach used in your first patch.
>>
>> Then - change the feature test to wait for the spinner to go away before
>> expanding the table node.
>>
>>> *Regarding your first patch:*
>>>>>
>>>>> Descriptive variable names are clearer than single-letter variable
>>>>> names. Could you clean up the first patch to use clearer variables, perhaps
>>>>> add some tests and break it up into methods so that I can more easily
>>>>> understand what your change does?
>>>>>
>>>> Agree.
>> But - as I said earlier, it represents the data for the tree-item (not
>> the item itself).
>> Hence - it should be 'itemData', and not 'item'.
>>
>> We've already used 'd' to represent the data, 'i' for item, 't' for tree
>> at all other places in pgAdmin 4.
>> So - it is consistent across the application.
>>
>> I would add comments to understand the functionality a lot better.
>>
>> Will share the patch for the same.
>>
>>
>> -- Thanks, Ashesh
>>
>>> Thank you,
>>>>>
>>>>> Tira & George
>>>>>
>>>>> On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
>>>>>> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>> > On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>>>>>> wrote:
>>>>>> >>
>>>>>> >> Hello,
>>>>>> >>
>>>>>> >> We agree that we should keep an eye on this and the failing
>>>>>> feature tests.
>>>>>> >> Our current story touches part of this code, but we won't go into
>>>>>> changing
>>>>>> >> the library for now.
>>>>>> >>
>>>>>> >> The patch Tira sent fixes a global variable problem that we found
>>>>>> while
>>>>>> >> looking into the code that generates the Tree, that
>>>>>> >> had the potential to load the tree in an infinite loop.
>>>>>> >> Can you apply the patch like this, or would you rather us send it
>>>>>> in a
>>>>>> >> separate patch email?
>>>>>> >
>>>>>> > Name of the variable should have been itemData, d (as earlier), as
>>>>>> it
>>>>>> > represents the data for the expanding node item.
>>>>>> > And, that is not good enough for resolving the issue.
>>>>>> >
>>>>>> > I've two approaches to resolve the idea.
>>>>>> > 1. Load the nodes (even when the module representing that node is
>>>>>> not yet
>>>>>> > loaded)
>>>>>> > Pros:
>>>>>> > - Nodes will be loaded even when the module for the node type is
>>>>>> not yet
>>>>>> > loaded.
>>>>>> > Cons:
>>>>>> > - The nodes with modified url (not the default mechanism) may get
>>>>>> failed, if
>>>>>> > the module is not yet loaded.
>>>>>> > (NOTE: We've not no nodes with that changes at the moment. so -
>>>>>> we can
>>>>>> > safely ignore it.)
>>>>>> > - Operations specific to the nodes will not be honoured until
>>>>>> modules is not
>>>>>> > loaded.
>>>>>> >
>>>>>> > 2. Wait for the modules to get loaded before allowing any
>>>>>> operations on
>>>>>> > them.
>>>>>> > Pros:
>>>>>> > - All operations will be done on a node only after the respective
>>>>>> module is
>>>>>> > loaded.
>>>>>> > Cons:
>>>>>> > - It will block any operations on pgAdmin 4, when the dependent
>>>>>> modules are
>>>>>> > being loaded. It can annoy the user.
>>>>>> >
>>>>>> > Please find the patches for both the approaches.
>>>>>> >
>>>>>> > Dave - please take a look at it.
>>>>>>
>>>>>> I'll leave you and Tira to figure this one out if you don't mind. My
>>>>>> plate is kinda full at the moment.
>>>>>>
>>>>>> I will note though that neither blocking or potential failures are
>>>>>> desirable.
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>