From 8bd5550831453c31428d162568d7d04c332706cf Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Tue, 24 Mar 2020 15:55:22 +0800 Subject: [PATCH 1/3] [UI] Stops experiment list from leaking previous error message --- frontend/src/pages/ExperimentList.test.tsx | 44 ++++++++++++++++++---- frontend/src/pages/ExperimentList.tsx | 6 ++- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/frontend/src/pages/ExperimentList.test.tsx b/frontend/src/pages/ExperimentList.test.tsx index 40509f0721d..f913df00649 100644 --- a/frontend/src/pages/ExperimentList.test.tsx +++ b/frontend/src/pages/ExperimentList.test.tsx @@ -29,7 +29,8 @@ import { RoutePage, QUERY_PARAMS } from '../components/Router'; import { range } from 'lodash'; import { ButtonKeys } from '../lib/Buttons'; import { NamespaceContext } from 'src/lib/KubeflowClient'; -import { render } from '@testing-library/react'; +import { render, act } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; // Default arguments for Apis.experimentServiceApi.listExperiment. const LIST_EXPERIMENT_DEFAULTS = [ @@ -71,17 +72,22 @@ describe('ExperimentList', () => { ); } + function mockListNExpperiment(n: number = 1) { + return () => + Promise.resolve({ + experiments: range(n).map(i => ({ + id: 'test-experiment-id' + i, + name: 'test experiment name' + i, + })), + }); + } + async function mountWithNExperiments( n: number, nRuns: number, { namespace }: { namespace?: string } = {}, ): Promise { - listExperimentsSpy.mockImplementation(() => ({ - experiments: range(n).map(i => ({ - id: 'test-experiment-id' + i, - name: 'test experiment name' + i, - })), - })); + listExperimentsSpy.mockImplementation(mockListNExpperiment(n)); listRunsSpy.mockImplementation(() => ({ runs: range(nRuns).map(i => ({ id: 'test-run-id' + i, name: 'test run name' + i })), })); @@ -473,5 +479,29 @@ describe('ExperimentList', () => { 'test-ns-2', ); }); + + it("doesn't keep error message for request from previous namespace", async () => { + listExperimentsSpy.mockImplementation(() => Promise.reject('namespace cannot be empty')); + const { rerender } = render( + + + + + , + ); + + listExperimentsSpy.mockImplementation(mockListNExpperiment()); + rerender( + + + + + , + ); + await act(TestUtils.flushPromises); + expect(updateBannerSpy).toHaveBeenLastCalledWith( + {}, // Empty object means banner has no error message + ); + }); }); }); diff --git a/frontend/src/pages/ExperimentList.tsx b/frontend/src/pages/ExperimentList.tsx index b9a18c55814..060c12e1e3a 100644 --- a/frontend/src/pages/ExperimentList.tsx +++ b/frontend/src/pages/ExperimentList.tsx @@ -188,6 +188,10 @@ export class ExperimentList extends Page<{ namespace?: string }, ExperimentListS displayExperiments = response.experiments || []; displayExperiments.forEach(exp => (exp.expandState = ExpandState.COLLAPSED)); } catch (err) { + // skip side effect if component is no longer mounted + if (!this._isMounted) { + return ''; + } await this.showPageError('Error: failed to retrieve list of experiments.', err); // No point in continuing if we couldn't retrieve any experiments. return ''; @@ -227,7 +231,7 @@ export class ExperimentList extends Page<{ namespace?: string }, ExperimentListS }), ); - this.setState({ displayExperiments }); + this.setStateSafe({ displayExperiments }); return response.next_page_token || ''; } From fbe9529ea89fc4b34c911cbec04130b85f43f116 Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Tue, 24 Mar 2020 16:00:05 +0800 Subject: [PATCH 2/3] Move the fix to Page component so it's more generic --- frontend/src/pages/ExperimentList.tsx | 4 ---- frontend/src/pages/Page.tsx | 9 +++++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/frontend/src/pages/ExperimentList.tsx b/frontend/src/pages/ExperimentList.tsx index 060c12e1e3a..2fb9d83944e 100644 --- a/frontend/src/pages/ExperimentList.tsx +++ b/frontend/src/pages/ExperimentList.tsx @@ -188,10 +188,6 @@ export class ExperimentList extends Page<{ namespace?: string }, ExperimentListS displayExperiments = response.experiments || []; displayExperiments.forEach(exp => (exp.expandState = ExpandState.COLLAPSED)); } catch (err) { - // skip side effect if component is no longer mounted - if (!this._isMounted) { - return ''; - } await this.showPageError('Error: failed to retrieve list of experiments.', err); // No point in continuing if we couldn't retrieve any experiments. return ''; diff --git a/frontend/src/pages/Page.tsx b/frontend/src/pages/Page.tsx index d66b9a40067..156f6cf4fe5 100644 --- a/frontend/src/pages/Page.tsx +++ b/frontend/src/pages/Page.tsx @@ -54,6 +54,9 @@ export abstract class Page extends React.Component

{ } public clearBanner(): void { + if (!this._isMounted) { + return; + } this.props.updateBanner({}); } @@ -63,6 +66,9 @@ export abstract class Page extends React.Component

{ mode?: 'error' | 'warning', ): Promise { const errorMessage = await errorToMessage(error); + if (!this._isMounted) { + return; + } this.props.updateBanner({ additionalInfo: errorMessage ? errorMessage : undefined, message: message + (errorMessage ? ' Click Details for more information.' : ''), @@ -72,6 +78,9 @@ export abstract class Page extends React.Component

{ } public showErrorDialog(title: string, content: string): void { + if (!this._isMounted) { + return; + } this.props.updateDialog({ buttons: [{ text: 'Dismiss' }], content, From 9aea936b0e4f7028bb5643700d479ba0d4ceb4ec Mon Sep 17 00:00:00 2001 From: "Yuan (Bob) Gong" Date: Wed, 25 Mar 2020 07:44:29 +0800 Subject: [PATCH 3/3] Update ExperimentList.test.tsx --- frontend/src/pages/ExperimentList.test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/pages/ExperimentList.test.tsx b/frontend/src/pages/ExperimentList.test.tsx index f913df00649..7f9c87eb5b4 100644 --- a/frontend/src/pages/ExperimentList.test.tsx +++ b/frontend/src/pages/ExperimentList.test.tsx @@ -72,7 +72,7 @@ describe('ExperimentList', () => { ); } - function mockListNExpperiment(n: number = 1) { + function mockListNExpperiments(n: number = 1) { return () => Promise.resolve({ experiments: range(n).map(i => ({ @@ -87,7 +87,7 @@ describe('ExperimentList', () => { nRuns: number, { namespace }: { namespace?: string } = {}, ): Promise { - listExperimentsSpy.mockImplementation(mockListNExpperiment(n)); + listExperimentsSpy.mockImplementation(mockListNExpperiments(n)); listRunsSpy.mockImplementation(() => ({ runs: range(nRuns).map(i => ({ id: 'test-run-id' + i, name: 'test run name' + i })), })); @@ -490,7 +490,7 @@ describe('ExperimentList', () => { , ); - listExperimentsSpy.mockImplementation(mockListNExpperiment()); + listExperimentsSpy.mockImplementation(mockListNExpperiments()); rerender(