PipelineList tests, fix clearing error banner #50
Conversation
|
/test mlpipeline-presubmit-e2e-test |
| @@ -173,7 +173,11 @@ class Compare extends Page<{}, CompareState> { | |||
| </div>); | |||
| } | |||
|
|
|||
| public async load() { | |||
| public async refresh() { | |||
There was a problem hiding this comment.
Are you sure you want to do this?
This will mean that any errors banners with a refresh button will be nonfunctional.
Maybe there should be some additional logic governing whether or not that refresh button shows up?
Ditto in a number of other files below.
There was a problem hiding this comment.
Error banners also call this new refresh function instead of load.
I did think about showing the refresh button automatically if the Page component implements the refresh function, but I thought that was too much magic, and also I think we should be moving in the direction of auto-refresh.
There was a problem hiding this comment.
Right, but in this case (and some others) the refresh doesn't do anything, correct?
There was a problem hiding this comment.
Ah I see. Thanks for catching that.
I missed those pages because they didn't have a Refresh button in the toolbar. We desperately need those tests!
| message: 'Error: failed to retrieve list of pipelines. Click Details for more information.', | ||
| mode: 'error', | ||
| })); | ||
| tree.unmount(); |
There was a problem hiding this comment.
Any harm with calling this when there's nothing to unmount? Seems like it should just be in afterEach()
There was a problem hiding this comment.
I agree. But then we'll have to maintain a global tree reference that we set and mount in each test, I'm not sure which I like more.
Also, forgetting to unmount isn't going to cause any issues, since these are pretty small footprint.
yebrahim
force-pushed
the
yebrahim/page-tests
branch
from
5e3bf17 to
e1df07a
Compare
| @@ -45,11 +45,11 @@ export abstract class Page<P, S> extends React.Component<P & PageProps, S> { | |||
| public abstract refresh(): Promise<void>; | |||
There was a problem hiding this comment.
Let's defer this for now, but it seems like there's probably a way to ensure clearBanner() is always called on refresh(), like maybe Page has a function _refresh() or something which calls clearBanner() and refresh() and is bound in showPageError() ?
There was a problem hiding this comment.
That's doable, although I've two points here:
- Ideally, the error should only be cleared when the operation succeeds, so the user shouldn't have to see it flicker. The current fix in this PR doesn't take care of this either.
- We should really prioritize auto-refresh. It's easy and will take care of so many UX issues.
|
/lgtm |
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rileyjbauer, yebrahim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fix: Fixing typo in repository URL
Final fix for Commit Checker GH Action
refreshmethod that can be called to only reload the relevant parts to the page, in order to avoid duplicate resource load requests.setStateSafe, which makes sure the component is mounted before callingsetStateon it. This should be used across all Page extenders, but is not the case yet (only PipelineList).This change is