Make sure subscriptions / queries are removed

[kinow]

These changes close #453

I think with this the subscriptions (and thus the final merged query) should be correct after the user has left the views modified here.

But will spend some more time writing some e2e tests as this issue has already happened before.

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Appropriate tests are included (unit and/or functional).
• No change log entry required (why? e.g. invisible to users).
• No documentation update required.

[codecov-io]

Codecov Report

Merging #454 into master will increase coverage by 3.30%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
+ Coverage   58.52%   61.83%   +3.30%     
==========================================
  Files          51       52       +1     
  Lines        1015     1027      +12     
  Branches       75       72       -3     
==========================================
+ Hits          594      635      +41     
+ Misses        408      379      -29     
  Partials       13       13              

Flag	Coverage Δ
#e2e	46.51% <75.00%> (+4.03%)	⬆️
#unittests	47.71% <0.00%> (ø)

Impacted Files	Coverage Δ
src/components/cylc/gscan/GScan.vue	72.22% <ø> (+7.22%)	⬆️
src/views/Workflow.vue	71.42% <62.50%> (+20.14%)	⬆️
src/views/Tree.vue	81.81% <66.66%> (ø)
src/main.js	91.66% <100.00%> (+1.66%)	⬆️
src/views/Dashboard.vue	77.27% <100.00%> (+14.77%)	⬆️
src/components/cylc/workflow/index.js	39.13% <0.00%> (+2.17%)	⬆️
src/components/cylc/tree/Tree.vue	59.52% <0.00%> (+11.90%)	⬆️
src/services/gquery.js	96.77% <0.00%> (+12.90%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ea8361...efc36e4. Read the comment docs.

[kinow]

This is necessary, as the View is not just a component, but bound to a Vue Router router. So the lifecycle is different. Views don't get destroyed as Vue components do. So the beforeDestroy doesn't always get called.

[kinow]

Ha! Wrote the first most basic e2e tests:

• User goes to Dashboard ✔️
• User goes to Dashboard, then to Workflows view ✔️
• User goes to Dashboard, then to Workflows view, then back to Dashboard 🪲
  In the third scenario instead of two subscriptions (i.e. two components or views subscribed), in my local environment it got 5 😮

[kinow]

Tests written for the number of subscriptions. Just need to improve the test. Instead of looking only at the number of subscriptions, it must make sure that the subscriptions present are the ones you expect.

For example, the third scenario again.

Dashboard -> Workflow -> Dashboard.

In this case, the GraphQL query that was merged, i.e. the final query, must not contain familyProxies nor cyclePoints, as these are not used by GScan or the Dashboard. After that this should be ready to review.

[kinow]

Ready for review.

Testing the following scenarios:

• User goes to the main page. We must have 2 (gscan/dashboard) subscriptions, and a query without cyclePoints. (i.e. GraphQL response doesn't have cyclePoints alias for familyProxies as in the tree query.)
• User goes to the main page, then to the workflows page. We must have 2 (gscan/tree) subscriptions, and a query with cyclePoints.
• User goes to the main page, then to the workflows page, then back to the dashboard. We must have 2 (gscan/dashboard) subscriptions, and a query without cyclePoints.
• User goes to the tree page. We must have 2 (gscan/tree) subscriptions, and a query with cyclePoints.
• User goes to the tree page, then back to the dashboard. We must have 2 (gscan/dashboard) subscriptions, and a query without cyclePoints.
• User goes to the main page, then to the user profile. We must have 1 subscription (gscan), and a query without cyclePoints.

I used the cyclePoints as at the moment the tree view is the only one that uses it. Likewise, one could write similar tests later for the Graph view/component using nodesEdges. I didn't test the Graph component because at the moment I'm focusing on the deltas with the tree view, but plannign on doing so when we restart work on the graph component.

[kinow]

What I learned with this pull request:

• It is tempting to add cy.wait(500) or similar, to wait a bit while a Vue router is loaded, but that can easily create flaky tests (I got the GitHub action to fail for 1 of the several push-force commits I submitted; kicking GH fixed it, but we don't want to have to kick it)
• Instead it's a bit more of work, but you can use cy.get($selector) and cy.contain($htmlElem, $text) to use Cypress' default mechanisms to wait until elements are displayed
• When doing so, you may also accidentally assume certain components mean the view and data store are ready, while it's not correct. I assumed I could use the Toolbar (displayed only when you select a workflow) would be a good idea, but Vue loads each component in the view asynchronously, with no guarantee of order, and we have some components and views triggering different actions. See screenshot below from Cypress runner, where the Toolbar is briefly displayed with the Dashboard

• We can expose the Vue app to the JS world via the global window object, which is handy for testing/development, but a no-no for production (that's part of this PR)

[oliver-sanders]

Surely this should be done by workflowSevice.unregister?

[kinow]

Ah, all this time I thought I needed both. Looks like the correct is

• Component/view wants to use the service:
  • register()
  • subscribe()
• Component is being destroyed or doesn't need the data anymore (e.g. a lumino widget removed)
  • unregister (which will do the same as unsubscribe())

@oliver-sanders do we need the unsubscribe methods? Both in GQuery.js, and in all the views? Could we perhaps combine register/subscription in a single method?

Updating the PR to use only unregister.

[kinow]

Done. I removed one unsubscribe call but left only some unsubscribe calls in Workflow.vue view. The way I built it, when you create that view, it "registers" the query. Then each time you create a widget it is "subscribed". Then when you remove the widget it just "unsubscribe"s. Then, when you leave the page it now only "unregisters".

Changing that here would increase considerably the amount of changes in this PR. But we can review it later (the goal here is just making sure we have the right amount of subscriptions when you navigate)

[kinow]

Ready for review again.

[oliver-sanders]

Looks good, though views may request multiple subscriptions.

[hjoliver]

Are you suggesting a change here?

[kinow]

Note to self: this change came from #458 . Once/if this gets merged, I need to drop a commit there (otherwise I would have an extra subscription that was getting in the way)

[oliver-sanders]

Code looks good, the only issue I see is what happens when subscriptions change.

The old system, though inelegant allowed views to change their subscription whilst running. Some reasons why subscriptions will have to change on the fly:

• User requests historical data (e.g. view all tasks in previous cycle).
• User requests extended data (e.g. expands view to see more fields).
• User changes the query depth (e.g. from n=1 to n=2).
• User changes the scope of a view (e.g. adding or removing workflows from a view).

[kinow]

Code looks good, the only issue I see is what happens when subscriptions change.

Good point 👍

Maybe we can create an issue to either bring back some of the old code, or work on an improvement later?

At least the part about starting, stopping/removing the subscriptions would stay anyway.

[kinow]

Superseded by #458