Feature test regression failures - Mailing list pgadmin-hackers

Tira's various patches all seem to be flakey :-(:

- The spinner is disabled before the top menus are rendered

- If I refresh the page in the browser, and try to expand Servers as quickly as possible after the spinner disappears, I could very easily get it to load another "Servers" node under Servers.

Ashesh's first patch seems to work. Everything loads as it should as far as I can see, and I was unable to reproduce the Servers under Servers issue.

The second patch also seemed to work, but the Loading Resources pause is extremely annoying, though it only seems to happen 3 times until everything is loaded and it's good from there on. That might be less obvious if the treeview spinner was used, and the treeview is disabled (but not greyed) whilst it's spinning.

I do wonder if Tira is correct when she asserts that there's probably no need to delay-load code for individual modules. As I only see the "Loading dependent resources" spinner with Ashesh's 2nd patch twice, I suspect we're loading most things at once anyway.

I also agree that we shouldn't have any shared/global state unnecessarily, and that aciTree's design might be part of the cause of this (though, at the time I first selected it, it seemed to be the best option, as the other tree controls I tried were missing key features). That was a few years ago now though. Perhaps there are better alternatives now, but I shudder to think how much work it might be to swap it out.

I also wonder if this would tie in with some of the code optimisation work I've been tinkering with. I've been looking at having a build step for packages that creates minimised versions of HTML/CSS/JS files, and automatically using those if DEBUG == False and they are present, falling back to un-minimised if required. I do wonder if as part of that minimisation, we could actually combine all these files together, and load everything in one go, saving not just bandwidth but round trips as well.

Anyway, regardless of other things, we still need to get the feature tests working reliably ASAP (Murtuza is working on some more at the moment). Ashesh, please replace the 1 char variable names with something more meaningful as a first step.

Would it help to get the two of you on a call to try to figure this out and agree on a solution? 

Thanks.

On Wednesday, March 22, 2017, Atira Odhner <aodhner@pivotal.io> wrote:

Hi Ashesh,

 

 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 was referring to the fact that fetchNodeInfo gets called when you hit the refresh button in the context menu, but acitree itself is responsible for loading the tree state otherwise. Maybe there is no good way to have a single code path for these two use cases using the acitree library.

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.

 

I understand that the javascript modules are being loaded dynamically when a corresponding item is first revealed in the tree, and although that seems unnecessary, I still don't think that is the cause of this bug. I think it is far more likely to be a result of sharing state that should not be shared.  I am having a hard time tracking down the bug because I find the code very difficult to read largely because of the one-letter variable names and lack of meaningful isolation between code paths.

 

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.

Clearly if I'm renaming it with the wrong thing, it isn't enough for me to tell 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.

Great. Please change it to itemData.

Thanks, 

Tira & Joao