Re: Feature test regression failures

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
>