Re: Feature test regression failures

--

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
>>>>
>>>
>>>
>>
>