Re: Feature test regression failures

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.
>
> 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?
>>
>> *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?
>>
>> 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
>>>
>>
>>
>