Re: Feature test regression failures

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.

-- Thanks, Ashesh

> Thanks!
> Joao and Sarah
>
> On Thu, Mar 16, 2017 at 10:56 PM, Atira Odhner <aodhner(at)pivotal(dot)io> wrote:
>
>> We're relying on the third party library jquery.acitree for tree
>>> operations.
>>
>>
>> Yes, what I was suggesting is to probably move away from that. AciTree is
>> one of the libraries my team identified as questionable (not in major
>> repositories, not actively maintained) and there are lots of alternatives.
>>
>> That said, I took a peek at the code and noticed that it was using a
>> global variable ('d') inside the loop where it calculated the tree
>> hierarchy. I suspect that is not the only issue, but here is a patch for it.
>>
>> Tira
>>
>> On Thu, Mar 16, 2017 at 1:39 PM, Ashesh Vashi <
>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>>
>>>
>>> On Mar 16, 2017 22:32, "Atira Odhner" <aodhner(at)pivotal(dot)io> wrote:
>>>
>>> I have seen this issue as well.
>>>
>>> Ashesh, this issue is related to the loading of the tree node data, not
>>> loading of code, correct?
>>>
>>> Theoritically - Each node may contain code to represent the node url.
>>> For all current nodes follow the function (present in all inherited class
>>> for nodes) to generated URI.
>>>
>>> So - indirectly it relies on the individual node code.
>>>
>>> Each time the user expands a node triggers an ajax request to fetch the
>>> child nodes. There are probably some performance tradeoffs to loading that
>>> tree up front.
>>>
>>> Agree.
>>> That was the reason, we load them only when first parent node (in some
>>> cases grand parent node) is loaded.
>>>
>>> But, there are ways to solve this issue without doing that. We could use
>>> callbacks/promises to wait for things to be loaded before rendering the
>>> node in an enabled/expandable state. It might be helpful to use a one-way
>>> data flow redux pattern to manage the state and rendering of the tree nodes.
>>>
>>> We're relying on the third party library jquery.acitree for tree
>>> operations.
>>> Most operations can be done asynchronous. But - url generation for
>>> individual node is done using callbacks, which can not work in asynchronous
>>> mode.
>>>
>>> I am thinking of using the 'require' function within that function, when
>>> that node type is not present in the application at that point of time.
>>> Though - I am still not sure, wheather 'require' works that way, or not.
>>>
>>> -- Thanks, Ashesh
>>>
>>> Tira
>>>
>>> On Thu, Mar 16, 2017, 6:49 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <
>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>> On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>> Hi Ashesh,
>>>>
>>>> A common theme is emerging from some of the feature test regression
>>>> failures on the Jenkins server. Please see:
>>>>
>>>> https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/
>>>> web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToSer
>>>> verFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png
>>>>
>>>> I've very occasionally seen similar behaviour to this in the past - in
>>>> fact it's part of the reason why we grey out the UI until pgAdmin is
>>>> fully loaded.
>>>>
>>>> Any idea what might be causing it?
>>>>
>>>> This can happen, when the module is not yet loaded for the respective
>>>> node, and it is being expanded.
>>>> Just thinking - shall we load all the javascript in the beginning?
>>>>
>>>>
>>>> That could be a lot of code, especially as the app grows. It may not be
>>>> an issue in the runtime (though, some recent reports would imply
>>>> otherwise), but it almost certainly would be on slower connections to
>>>> installations running in server mode.
>>>>
>>>> There must be a way to ensure the code is loaded before we allow it to
>>>> be used?
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>
>>>
>>
>>
>> --
>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>
>>
>