From ae0f2a9348db9e695cbee3e047665356616b862b Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Thu, 24 Aug 2023 16:40:40 -0400 Subject: [PATCH 01/23] display stateful set errors when notebook pod fails to create --- .../src/routes/api/nb-events/eventUtils.ts | 7 +- backend/src/routes/api/nb-events/index.ts | 35 ++++----- frontend/src/api/k8s/events.ts | 19 +++-- frontend/src/api/models/k8s.ts | 7 ++ .../screens/server/StartServerModal.tsx | 13 ++-- .../projects/notebook/StartNotebookModal.tsx | 13 ++-- .../notebook/useWatchNotebookEvents.ts | 70 ++++++++++------- frontend/src/pages/projects/notebook/utils.ts | 13 +++- .../src/services/notebookEventsService.ts | 8 +- .../src/utilities/notebookControllerUtils.ts | 22 +++--- .../src/utilities/useWatchNotebookEvents.tsx | 75 +++++++++++-------- 11 files changed, 167 insertions(+), 115 deletions(-) diff --git a/backend/src/routes/api/nb-events/eventUtils.ts b/backend/src/routes/api/nb-events/eventUtils.ts index 3c0cd98ec8..66e6bd9c15 100644 --- a/backend/src/routes/api/nb-events/eventUtils.ts +++ b/backend/src/routes/api/nb-events/eventUtils.ts @@ -4,7 +4,8 @@ import { KubeFastifyInstance } from '../../../types'; export const getNotebookEvents = async ( fastify: KubeFastifyInstance, namespace: string, - podUID: string, + notebookName: string, + podUID: string | undefined, ): Promise => { return fastify.kube.coreV1Api .listNamespacedEvent( @@ -12,7 +13,9 @@ export const getNotebookEvents = async ( undefined, undefined, undefined, - `involvedObject.kind=Pod,involvedObject.uid=${podUID}`, + podUID + ? `involvedObject.kind=Pod,involvedObject.uid=${podUID}` + : `involvedObject.kind=StatefulSet,involvedObject.name=${notebookName}`, ) .then((res) => { const body = res.body as V1EventList; diff --git a/backend/src/routes/api/nb-events/index.ts b/backend/src/routes/api/nb-events/index.ts index b66a670d1b..7c3b2bdc6a 100644 --- a/backend/src/routes/api/nb-events/index.ts +++ b/backend/src/routes/api/nb-events/index.ts @@ -3,24 +3,21 @@ import { getNotebookEvents } from './eventUtils'; import { secureRoute } from '../../../utils/route-security'; export default async (fastify: FastifyInstance): Promise => { - fastify.get( - '/:namespace/:podUID', - secureRoute(fastify)( - async ( - request: FastifyRequest<{ - Params: { - namespace: string; - podUID: string; - }; - Querystring: { - // TODO: Support server side filtering - from?: string; - }; - }>, - ) => { - const { namespace, podUID } = request.params; - return getNotebookEvents(fastify, namespace, podUID); - }, - ), + const routeHandler = secureRoute(fastify)( + async ( + request: FastifyRequest<{ + Params: { + namespace: string; + notebookName: string; + podUID: string | undefined; + }; + }>, + ) => { + const { namespace, notebookName, podUID } = request.params; + return getNotebookEvents(fastify, namespace, notebookName, podUID); + }, ); + + fastify.get('/:namespace/:notebookName', routeHandler); + fastify.get('/:namespace/:notebookName/:podUID', routeHandler); }; diff --git a/frontend/src/api/k8s/events.ts b/frontend/src/api/k8s/events.ts index 8eb04396dd..4ca93854af 100644 --- a/frontend/src/api/k8s/events.ts +++ b/frontend/src/api/k8s/events.ts @@ -1,17 +1,20 @@ -import { k8sListResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { k8sListResourceItems } from '@openshift/dynamic-plugin-sdk-utils'; import { EventKind } from '~/k8sTypes'; import { EventModel } from '~/api/models'; -export const getNotebookEvents = async (namespace: string, podUid: string): Promise => - k8sListResource({ +export const getNotebookEvents = async ( + namespace: string, + notebookName: string, + podUid: string | undefined, +): Promise => + k8sListResourceItems({ model: EventModel, queryOptions: { ns: namespace, queryParams: { - fieldSelector: `involvedObject.kind=Pod,involvedObject.uid=${podUid}`, + fieldSelector: podUid + ? `involvedObject.kind=Pod,involvedObject.uid=${podUid}` + : `involvedObject.kind=StatefulSet,involvedObject.name=${notebookName}`, }, }, - }).then( - // Filter the events by pods that have the same name as the notebook - (r) => r.items, - ); + }); diff --git a/frontend/src/api/models/k8s.ts b/frontend/src/api/models/k8s.ts index 7da19cbcf0..2fc56ea7b6 100644 --- a/frontend/src/api/models/k8s.ts +++ b/frontend/src/api/models/k8s.ts @@ -18,6 +18,13 @@ export const PodModel: K8sModelCommon = { plural: 'pods', }; +export const StatefulSetModel: K8sModelCommon = { + apiVersion: 'v1', + apiGroup: 'apps', + kind: 'StatefulSet', + plural: 'statefulsets', +}; + export const PVCModel: K8sModelCommon = { apiVersion: 'v1', kind: 'PersistentVolumeClaim', diff --git a/frontend/src/pages/notebookController/screens/server/StartServerModal.tsx b/frontend/src/pages/notebookController/screens/server/StartServerModal.tsx index c1025694e7..3302bcc8a7 100644 --- a/frontend/src/pages/notebookController/screens/server/StartServerModal.tsx +++ b/frontend/src/pages/notebookController/screens/server/StartServerModal.tsx @@ -217,11 +217,14 @@ const StartServerModal: React.FC = ({ open, spawnInProgre isIndented > - {events.reverse().map((event, index) => ( - - {`${getEventTimestamp(event)} [${event.type}] ${event.message}`} - - ))} + {events + .slice() + .reverse() + .map((event, index) => ( + + {`${getEventTimestamp(event)} [${event.type}] ${event.message}`} + + ))} Server requested diff --git a/frontend/src/pages/projects/notebook/StartNotebookModal.tsx b/frontend/src/pages/projects/notebook/StartNotebookModal.tsx index dab3a840e1..bf411eb345 100644 --- a/frontend/src/pages/projects/notebook/StartNotebookModal.tsx +++ b/frontend/src/pages/projects/notebook/StartNotebookModal.tsx @@ -162,11 +162,14 @@ const StartNotebookModal: React.FC = ({ - {events.reverse().map((event, index) => ( - - {getEventFullMessage(event)} - - ))} + {events + .slice() + .reverse() + .map((event, index) => ( + + {getEventFullMessage(event)} + + ))} Server requested diff --git a/frontend/src/pages/projects/notebook/useWatchNotebookEvents.ts b/frontend/src/pages/projects/notebook/useWatchNotebookEvents.ts index 1370b5e1c9..8cac233404 100644 --- a/frontend/src/pages/projects/notebook/useWatchNotebookEvents.ts +++ b/frontend/src/pages/projects/notebook/useWatchNotebookEvents.ts @@ -1,47 +1,61 @@ import * as React from 'react'; -import { EventKind } from '~/k8sTypes'; +import { EventKind, NotebookKind } from '~/k8sTypes'; import { getNotebookEvents } from '~/api'; import { FAST_POLL_INTERVAL } from '~/utilities/const'; export const useWatchNotebookEvents = ( - projectName: string, + notebook: NotebookKind, podUid: string, activeFetch: boolean, ): EventKind[] => { - const [notebookEvents, setNoteBookEvents] = React.useState([]); + const notebookName = notebook.metadata.name; + const namespace = notebook.metadata.namespace; + const [notebookEvents, setNotebookEvents] = React.useState([]); + + // Cached events are returned when activeFetch is false. + // This allows us to reset notebookEvents state when activeFetch becomes + // false to prevent UI blips when activeFetch goes true again. + const notebookEventsCache = React.useRef(notebookEvents); + + // when activeFetch switches to false, reset events state + React.useEffect(() => { + if (!activeFetch) { + setNotebookEvents([]); + } + }, [activeFetch]); React.useEffect(() => { let watchHandle: ReturnType; let cancelled = false; - const clear = () => { - cancelled = true; - clearTimeout(watchHandle); - }; - - if (activeFetch) { + if (activeFetch && namespace && notebookName) { const watchNotebookEvents = () => { - if (projectName && podUid) { - getNotebookEvents(projectName, podUid) - .then((data: EventKind[]) => { - if (cancelled) { - return; - } - setNoteBookEvents(data); - }) - .catch((e) => { - /* eslint-disable-next-line no-console */ - console.error('Error fetching notebook events', e); - clear(); - }); - watchHandle = setTimeout(watchNotebookEvents, FAST_POLL_INTERVAL); - } + getNotebookEvents(namespace, notebookName, podUid) + .then((data: EventKind[]) => { + if (!cancelled) { + notebookEventsCache.current = data; + setNotebookEvents(data); + } + }) + .catch((e) => { + /* eslint-disable-next-line no-console */ + console.error('Error fetching notebook events', e); + }); + watchHandle = setTimeout(watchNotebookEvents, FAST_POLL_INTERVAL); }; - watchNotebookEvents(); + if (!podUid) { + // delay the initial fetch to avoid older StatefulSet event errors from blipping on screen during notebook startup + watchHandle = setTimeout(watchNotebookEvents, Math.max(FAST_POLL_INTERVAL, 3000)); + } else { + watchNotebookEvents(); + } } - return clear; - }, [projectName, podUid, activeFetch]); + return () => { + cancelled = true; + clearTimeout(watchHandle); + }; + }, [namespace, notebookName, podUid, activeFetch]); - return notebookEvents; + return activeFetch ? notebookEvents : notebookEventsCache.current; }; diff --git a/frontend/src/pages/projects/notebook/utils.ts b/frontend/src/pages/projects/notebook/utils.ts index 5a361cbe20..6a66e3c7d1 100644 --- a/frontend/src/pages/projects/notebook/utils.ts +++ b/frontend/src/pages/projects/notebook/utils.ts @@ -70,9 +70,9 @@ const filterEvents = ( allEvents: EventKind[], lastActivity: Date, ): [filterEvents: EventKind[], thisInstanceEvents: EventKind[], gracePeriod: boolean] => { - const thisInstanceEvents = allEvents.filter( - (event) => new Date(getEventTimestamp(event)) >= lastActivity, - ); + const thisInstanceEvents = allEvents + .filter((event) => new Date(getEventTimestamp(event)) >= lastActivity) + .sort((a, b) => getEventTimestamp(a).localeCompare(getEventTimestamp(b))); if (thisInstanceEvents.length === 0) { // Filtered out all of the events, exit early return [[], [], false]; @@ -121,7 +121,7 @@ export const useNotebookStatus = ( podUid: string, spawnInProgress: boolean, ): [status: NotebookStatus | null, events: EventKind[]] => { - const events = useWatchNotebookEvents(notebook.metadata.namespace, podUid, spawnInProgress); + const events = useWatchNotebookEvents(notebook, podUid, spawnInProgress); const annotationTime = notebook?.metadata.annotations?.['notebooks.kubeflow.org/last-activity']; const lastActivity = annotationTime @@ -230,6 +230,11 @@ export const useNotebookStatus = ( status = EventStatus.INFO; break; } + case 'FailedCreate': { + currentEvent = 'Failed to create pod'; + status = EventStatus.ERROR; + break; + } default: { if (!gracePeriod && lastItem.reason === 'FailedScheduling') { currentEvent = 'Insufficient resources to start'; diff --git a/frontend/src/services/notebookEventsService.ts b/frontend/src/services/notebookEventsService.ts index cdf82220bb..52944f125c 100644 --- a/frontend/src/services/notebookEventsService.ts +++ b/frontend/src/services/notebookEventsService.ts @@ -1,7 +1,11 @@ import axios from 'axios'; import { K8sEvent } from '~/types'; -export const getNotebookEvents = (projectName: string, podUID: string): Promise => { - const url = `/api/nb-events/${projectName}/${podUID}`; +export const getNotebookEvents = ( + projectName: string, + notebookName: string, + podUID?: string, +): Promise => { + const url = `/api/nb-events/${projectName}/${notebookName}${podUID ? `/${podUID}` : ''}`; return axios.get(url).then((response) => response.data); }; diff --git a/frontend/src/utilities/notebookControllerUtils.ts b/frontend/src/utilities/notebookControllerUtils.ts index 98ad8aa954..7a83212df4 100644 --- a/frontend/src/utilities/notebookControllerUtils.ts +++ b/frontend/src/utilities/notebookControllerUtils.ts @@ -249,9 +249,9 @@ const filterEvents = ( allEvents: K8sEvent[], lastActivity: Date, ): [filterEvents: K8sEvent[], thisInstanceEvents: K8sEvent[], gracePeroid: boolean] => { - const thisInstanceEvents = allEvents.filter( - (event) => new Date(getEventTimestamp(event)) >= lastActivity, - ); + const thisInstanceEvents = allEvents + .filter((event) => new Date(getEventTimestamp(event)) >= lastActivity) + .sort((a, b) => getEventTimestamp(a).localeCompare(getEventTimestamp(b))); if (thisInstanceEvents.length === 0) { // Filtered out all of the events, exit early return [[], [], false]; @@ -311,7 +311,6 @@ export const useNotebookStatus = ( spawnInProgress: boolean, open: boolean, ): [status: NotebookStatus | null, events: K8sEvent[]] => { - const { notebookNamespace } = useNamespaces(); const { currentUserNotebook: notebook, currentUserNotebookIsRunning: isNotebookRunning, @@ -319,15 +318,9 @@ export const useNotebookStatus = ( } = React.useContext(NotebookControllerContext); const events = useWatchNotebookEvents( - notebookNamespace, + notebook, currentUserNotebookPodUID, spawnInProgress && !isNotebookRunning, - ).filter( - (evt) => - // Note: This looks redundant but is useful -- our state is stale when a new pod starts; this cleans that up - // This is not ideal, but the alternative is expose a reset function & thread it up to the stop call - // This is old code and needs to be removed in time, this will do for now. - evt.involvedObject.uid === currentUserNotebookPodUID, ); const lastActivity = @@ -339,7 +332,7 @@ export const useNotebookStatus = ( ? new Date(notebook.metadata.creationTimestamp ?? 0) : null); - if (!lastActivity) { + if (!notebook || !lastActivity) { // Notebook not started, we don't have a filter time, ignore return [null, []]; } @@ -439,6 +432,11 @@ export const useNotebookStatus = ( status = EventStatus.INFO; break; } + case 'FailedCreate': { + currentEvent = 'Failed to create pod'; + status = EventStatus.ERROR; + break; + } default: { if (!gracePeriod && lastItem.reason === 'FailedScheduling') { currentEvent = 'Insufficient resources to start'; diff --git a/frontend/src/utilities/useWatchNotebookEvents.tsx b/frontend/src/utilities/useWatchNotebookEvents.tsx index dc6e4e62e5..6a0b2d13df 100644 --- a/frontend/src/utilities/useWatchNotebookEvents.tsx +++ b/frontend/src/utilities/useWatchNotebookEvents.tsx @@ -1,49 +1,64 @@ import * as React from 'react'; import { getNotebookEvents } from '~/services/notebookEventsService'; -import { K8sEvent } from '~/types'; +import { K8sEvent, Notebook } from '~/types'; import useNotification from './useNotification'; import { FAST_POLL_INTERVAL } from './const'; export const useWatchNotebookEvents = ( - projectName: string, - podUID: string, + notebook: Notebook | null, + podUid: string, activeFetch: boolean, ): K8sEvent[] => { - const [notebookEvents, setNoteBookEvents] = React.useState([]); + const notebookName = notebook?.metadata.name; + const namespace = notebook?.metadata.namespace; + const [notebookEvents, setNotebookEvents] = React.useState([]); const notification = useNotification(); + // Cached events are returned when activeFetch is false. + // This allows us to reset notebookEvents state when activeFetch becomes + // false to prevent UI blips when activeFetch goes true again. + const notebookEventsCache = React.useRef(notebookEvents); + + // when activeFetch switches to false, reset events state + React.useEffect(() => { + if (!activeFetch) { + setNotebookEvents([]); + } + }, [activeFetch]); + React.useEffect(() => { let watchHandle: ReturnType; let cancelled = false; - const clear = () => { - cancelled = true; - clearTimeout(watchHandle); - }; - - if (activeFetch) { + if (activeFetch && namespace && notebookName) { const watchNotebookEvents = () => { - if (projectName && podUID) { - getNotebookEvents(projectName, podUID) - .then((data: K8sEvent[]) => { - if (cancelled) { - return; - } - setNoteBookEvents(data); - }) - .catch((e) => { - notification.error('Error fetching notebook events', e.response.data.message); - clear(); - }) - .finally(() => { - watchHandle = setTimeout(watchNotebookEvents, FAST_POLL_INTERVAL); - }); - } + getNotebookEvents(namespace, notebookName, podUid) + .then((data: K8sEvent[]) => { + if (!cancelled) { + notebookEventsCache.current = data; + setNotebookEvents(data); + } + }) + .catch((e) => { + notification.error('Error fetching notebook events', e.response.data.message); + /* eslint-disable-next-line no-console */ + console.error('Error fetching notebook events', e); + }); + watchHandle = setTimeout(watchNotebookEvents, FAST_POLL_INTERVAL); }; - watchNotebookEvents(); + + if (!podUid) { + // delay the initial fetch to avoid older StatefulSet event errors from blipping on screen during notebook startup + watchHandle = setTimeout(watchNotebookEvents, Math.max(FAST_POLL_INTERVAL, 3000)); + } else { + watchNotebookEvents(); + } } - return clear; - }, [projectName, podUID, notification, activeFetch]); + return () => { + cancelled = true; + clearTimeout(watchHandle); + }; + }, [namespace, notebookName, podUid, activeFetch, notification]); - return notebookEvents; + return activeFetch ? notebookEvents : notebookEventsCache.current; }; From 534395a895f3084a4f8880a69ab35d1ea3aa2396 Mon Sep 17 00:00:00 2001 From: ppadti Date: Wed, 30 Aug 2023 13:38:51 +0530 Subject: [PATCH 02/23] Truncate long display name in serving runtimes --- frontend/src/components/table/Table.tsx | 2 +- .../customServingRuntimes/CustomServingRuntimeTableRow.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/table/Table.tsx b/frontend/src/components/table/Table.tsx index cf406bb5e8..568a126791 100644 --- a/frontend/src/components/table/Table.tsx +++ b/frontend/src/components/table/Table.tsx @@ -111,7 +111,7 @@ const Table = ({ )} {caption && {caption}} - + {columns.map((col, i) => { if (col.field === CHECKBOX_FIELD_ID && selectAll) { diff --git a/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx b/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx index aef3835cad..7ee5b7dd71 100644 --- a/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx +++ b/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx @@ -34,7 +34,7 @@ const CustomServingRuntimeTableRow: React.FC id: `draggable-row-${servingRuntimeName}`, }} /> - + {getServingRuntimeDisplayNameFromTemplate(template)} From 77ff6681a2e7a7b34c065bc5998bc7bcfab77ace Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Tue, 5 Sep 2023 09:38:08 -0400 Subject: [PATCH 03/23] Limit storage min size onBlur instead of onChange --- frontend/src/pages/projects/components/PVSizeField.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/src/pages/projects/components/PVSizeField.tsx b/frontend/src/pages/projects/components/PVSizeField.tsx index bf22b92c89..4e8366515b 100644 --- a/frontend/src/pages/projects/components/PVSizeField.tsx +++ b/frontend/src/pages/projects/components/PVSizeField.tsx @@ -40,7 +40,13 @@ const PVSizeField: React.FC = ({ fieldID, size, setSize, curre onChange={(event) => { if (isHTMLInputElement(event.target)) { const newSize = Number(event.target.value); - setSize(isNaN(newSize) ? size : Math.max(newSize, minSize)); + setSize(isNaN(newSize) ? size : newSize); + } + }} + onBlur={(event) => { + if (isHTMLInputElement(event.target)) { + const blurSize = Number(event.target.value); + setSize(Math.max(blurSize, minSize)); } }} /> From c1ccd3fd6c2ee84b006c8063f871951199ab9e01 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Tue, 5 Sep 2023 10:38:55 -0400 Subject: [PATCH 04/23] Refresh data connections when deploying model on project details page --- .../screens/projects/ServingRuntimeList.tsx | 7 +---- .../screens/projects/ServingRuntimeTable.tsx | 28 ++++++------------- 2 files changed, 9 insertions(+), 26 deletions(-) diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeList.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeList.tsx index 8b804c2247..49f3a5e8b9 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeList.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeList.tsx @@ -63,12 +63,7 @@ const ServingRuntimeList: React.FC = () => { /> } > - + void; - refreshTokens: () => void; - refreshInferenceServices: () => void; -}; - -const ServingRuntimeTable: React.FC = ({ - modelServers: unsortedModelServers, - refreshServingRuntime, - refreshTokens, - refreshInferenceServices, -}) => { +const ServingRuntimeTable: React.FC = () => { const [deployServingRuntime, setDeployServingRuntime] = React.useState(); const [deleteServingRuntime, setDeleteServingRuntime] = React.useState(); const [editServingRuntime, setEditServingRuntime] = React.useState(); const [expandedColumn, setExpandedColumn] = React.useState(); const { - dataConnections: { data: dataConnections }, - inferenceServices: { data: inferenceServices }, + servingRuntimes: { data: modelServers, refresh: refreshServingRuntime }, + serverSecrets: { refresh: refreshTokens }, + dataConnections: { data: dataConnections, refresh: refreshDataConnections }, + inferenceServices: { data: inferenceServices, refresh: refreshInferenceServices }, filterTokens, currentProject, } = React.useContext(ProjectDetailsContext); - const sort = useTableColumnSort(columns, 1); - - const sortedModelServers = sort.transformData(unsortedModelServers); return ( <> ( = ({ setDeployServingRuntime(undefined); if (submit) { refreshInferenceServices(); + refreshDataConnections(); setExpandedColumn(ServingRuntimeTableTabs.DEPLOYED_MODELS); } }} From 3d1b09b6b2f1133bd19ddc4f28981e1f7459bf70 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Tue, 5 Sep 2023 13:50:09 -0400 Subject: [PATCH 05/23] Add access check before deleting model server --- .../screens/projects/ServingRuntimeTable.tsx | 41 +++++++++++++------ .../projects/ServingRuntimeTableRow.tsx | 14 +++++-- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx index bd407b01b3..60a806871e 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx @@ -1,14 +1,21 @@ import * as React from 'react'; import Table from '~/components/table/Table'; -import { ServingRuntimeKind } from '~/k8sTypes'; +import { AccessReviewResourceAttributes, ServingRuntimeKind } from '~/k8sTypes'; import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; import { ServingRuntimeTableTabs } from '~/pages/modelServing/screens/types'; +import { useAccessReview } from '~/api'; import { columns } from './data'; import ServingRuntimeTableRow from './ServingRuntimeTableRow'; import DeleteServingRuntimeModal from './DeleteServingRuntimeModal'; import ManageServingRuntimeModal from './ServingRuntimeModal/ManageServingRuntimeModal'; import ManageInferenceServiceModal from './InferenceServiceModal/ManageInferenceServiceModal'; +const accessReviewResource: AccessReviewResourceAttributes = { + group: 'rbac.authorization.k8s.io', + resource: 'rolebindings', + verb: 'delete', +}; + const ServingRuntimeTable: React.FC = () => { const [deployServingRuntime, setDeployServingRuntime] = React.useState(); const [deleteServingRuntime, setDeleteServingRuntime] = React.useState(); @@ -24,6 +31,13 @@ const ServingRuntimeTable: React.FC = () => { currentProject, } = React.useContext(ProjectDetailsContext); + const namespace = currentProject.metadata.name; + + const [allowDelete] = useAccessReview({ + ...accessReviewResource, + namespace, + }); + return ( <>
{ onDeployModel={(obj) => setDeployServingRuntime(obj)} expandedColumn={expandedColumn} setExpandedColumn={setExpandedColumn} + allowDelete={allowDelete} /> )} /> - { - if (deleted) { - refreshServingRuntime(); - } - setDeleteServingRuntime(undefined); - }} - /> + {allowDelete && ( + { + if (deleted) { + refreshServingRuntime(); + } + setDeleteServingRuntime(undefined); + }} + /> + )} void; expandedColumn?: ServingRuntimeTableTabs; setExpandedColumn: (column?: ServingRuntimeTableTabs) => void; + allowDelete: boolean; }; const ServingRuntimeTableRow: React.FC = ({ @@ -27,6 +28,7 @@ const ServingRuntimeTableRow: React.FC = ({ onDeployModel, expandedColumn, setExpandedColumn, + allowDelete, }) => { const { inferenceServices: { @@ -135,10 +137,14 @@ const ServingRuntimeTableRow: React.FC = ({ title: 'Edit model server', onClick: () => onEditServingRuntime(obj), }, - { - title: 'Delete model server', - onClick: () => onDeleteServingRuntime(obj), - }, + ...(allowDelete + ? [ + { + title: 'Delete model server', + onClick: () => onDeleteServingRuntime(obj), + }, + ] + : []), ]} /> From 040e7943d866a1c9783e3e250a98fb6baaaa5c9f Mon Sep 17 00:00:00 2001 From: pnaik1 Date: Wed, 6 Sep 2023 15:17:29 +0530 Subject: [PATCH 06/23] tooltip on pipeline screen impede clicking of the slider right above --- .../pipelines/content/tables/renderUtils.tsx | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/frontend/src/concepts/pipelines/content/tables/renderUtils.tsx b/frontend/src/concepts/pipelines/content/tables/renderUtils.tsx index 4c13dd0ae7..bcb0249941 100644 --- a/frontend/src/concepts/pipelines/content/tables/renderUtils.tsx +++ b/frontend/src/concepts/pipelines/content/tables/renderUtils.tsx @@ -169,22 +169,20 @@ export const RunJobStatus: RunJobUtil<{ onToggle: (value: boolean) => Promise - - { - setIsChangingFlag(true); - setError(null); - onToggle(checked).catch((e) => { - setError(e); - setIsChangingFlag(false); - }); - }} - isChecked={isEnabled} - /> - + { + setIsChangingFlag(true); + setError(null); + onToggle(checked).catch((e) => { + setError(e); + setIsChangingFlag(false); + }); + }} + isChecked={isEnabled} + /> {isChangingFlag && } From 77d9c522e65e93031f3dc873476657acb45bdb23 Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Thu, 3 Aug 2023 12:08:14 -0400 Subject: [PATCH 07/23] add utility to make it easier and more robust to test hooks --- frontend/package-lock.json | 194 +++++++++++--- frontend/package.json | 1 + .../__tests__/unit/testUtils/hooks.spec.ts | 163 +++++++++++ .../src/__tests__/unit/testUtils/hooks.ts | 253 ++++++++++++++++++ .../src/__tests__/unit/useFetchState.spec.ts | 97 +++++++ frontend/src/__tests__/unit/useGroups.spec.ts | 125 +++++++++ frontend/src/pages/TestPage.tsx.bak | 55 ++++ frontend/src/utilities/useFetchState.ts | 8 +- 8 files changed, 851 insertions(+), 45 deletions(-) create mode 100644 frontend/src/__tests__/unit/testUtils/hooks.spec.ts create mode 100644 frontend/src/__tests__/unit/testUtils/hooks.ts create mode 100644 frontend/src/__tests__/unit/useFetchState.spec.ts create mode 100644 frontend/src/__tests__/unit/useGroups.spec.ts create mode 100644 frontend/src/pages/TestPage.tsx.bak diff --git a/frontend/package-lock.json b/frontend/package-lock.json index a246363ff2..1e407212c1 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -45,6 +45,7 @@ }, "devDependencies": { "@babel/core": "^7.21.0", + "@testing-library/react": "^14.0.0", "@types/dompurify": "^2.2.6", "@types/lodash-es": "^4.17.8", "@types/node": "^17.0.29", @@ -8826,6 +8827,113 @@ "node": ">=8" } }, + "node_modules/@testing-library/react": { + "version": "14.0.0", + "resolved": "https://registry.npmjs.org/@testing-library/react/-/react-14.0.0.tgz", + "integrity": "sha512-S04gSNJbYE30TlIMLTzv6QCTzt9AqIF5y6s6SzVFILNcNvbV/jU96GeiTPillGQo+Ny64M/5PV7klNYYgv5Dfg==", + "dev": true, + "dependencies": { + "@babel/runtime": "^7.12.5", + "@testing-library/dom": "^9.0.0", + "@types/react-dom": "^18.0.0" + }, + "engines": { + "node": ">=14" + }, + "peerDependencies": { + "react": "^18.0.0", + "react-dom": "^18.0.0" + } + }, + "node_modules/@testing-library/react/node_modules/@testing-library/dom": { + "version": "9.3.1", + "resolved": "https://registry.npmjs.org/@testing-library/dom/-/dom-9.3.1.tgz", + "integrity": "sha512-0DGPd9AR3+iDTjGoMpxIkAsUihHZ3Ai6CneU6bRRrffXMgzCdlNk43jTrD2/5LT6CBb3MWTP8v510JzYtahD2w==", + "dev": true, + "dependencies": { + "@babel/code-frame": "^7.10.4", + "@babel/runtime": "^7.12.5", + "@types/aria-query": "^5.0.1", + "aria-query": "5.1.3", + "chalk": "^4.1.0", + "dom-accessibility-api": "^0.5.9", + "lz-string": "^1.5.0", + "pretty-format": "^27.0.2" + }, + "engines": { + "node": ">=14" + } + }, + "node_modules/@testing-library/react/node_modules/ansi-styles": { + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-4.3.0.tgz", + "integrity": "sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==", + "dev": true, + "dependencies": { + "color-convert": "^2.0.1" + }, + "engines": { + "node": ">=8" + }, + "funding": { + "url": "https://github.com/chalk/ansi-styles?sponsor=1" + } + }, + "node_modules/@testing-library/react/node_modules/chalk": { + "version": "4.1.2", + "resolved": "https://registry.npmjs.org/chalk/-/chalk-4.1.2.tgz", + "integrity": "sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA==", + "dev": true, + "dependencies": { + "ansi-styles": "^4.1.0", + "supports-color": "^7.1.0" + }, + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/chalk/chalk?sponsor=1" + } + }, + "node_modules/@testing-library/react/node_modules/color-convert": { + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/color-convert/-/color-convert-2.0.1.tgz", + "integrity": "sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ==", + "dev": true, + "dependencies": { + "color-name": "~1.1.4" + }, + "engines": { + "node": ">=7.0.0" + } + }, + "node_modules/@testing-library/react/node_modules/color-name": { + "version": "1.1.4", + "resolved": "https://registry.npmjs.org/color-name/-/color-name-1.1.4.tgz", + "integrity": "sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA==", + "dev": true + }, + "node_modules/@testing-library/react/node_modules/has-flag": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/has-flag/-/has-flag-4.0.0.tgz", + "integrity": "sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ==", + "dev": true, + "engines": { + "node": ">=8" + } + }, + "node_modules/@testing-library/react/node_modules/supports-color": { + "version": "7.2.0", + "resolved": "https://registry.npmjs.org/supports-color/-/supports-color-7.2.0.tgz", + "integrity": "sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==", + "dev": true, + "dependencies": { + "has-flag": "^4.0.0" + }, + "engines": { + "node": ">=8" + } + }, "node_modules/@tootallnate/once": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/@tootallnate/once/-/once-2.0.0.tgz", @@ -8848,7 +8956,7 @@ "version": "5.0.1", "resolved": "https://registry.npmjs.org/@types/aria-query/-/aria-query-5.0.1.tgz", "integrity": "sha512-XTIieEY+gvJ39ChLcB4If5zHtPxt3Syj5rgZR+e1ctpmK8NjPf0zFqsz4JpLJT0xla9GFDKjy8Cpu331nrmE1Q==", - "optional": true + "devOptional": true }, "node_modules/@types/babel__core": { "version": "7.20.0", @@ -10666,7 +10774,7 @@ "version": "5.1.3", "resolved": "https://registry.npmjs.org/aria-query/-/aria-query-5.1.3.tgz", "integrity": "sha512-R5iJ5lkuHybztUfuOAznmboyjWq8O6sqNqtK7CLOqdydi54VNbORp49mb14KbWgG1QD3JFO9hJdZ+y4KutfdOQ==", - "optional": true, + "devOptional": true, "dependencies": { "deep-equal": "^2.0.5" } @@ -10819,7 +10927,7 @@ "version": "1.0.5", "resolved": "https://registry.npmjs.org/available-typed-arrays/-/available-typed-arrays-1.0.5.tgz", "integrity": "sha512-DMD0KiN46eipeziST1LPP/STfDU0sufISXmjSgvVsoU2tqxctQeASejWcfNtxYKqETM1UxQ8sp2OrSBWpHY6sw==", - "optional": true, + "devOptional": true, "engines": { "node": ">= 0.4" }, @@ -13918,7 +14026,7 @@ "version": "2.2.0", "resolved": "https://registry.npmjs.org/deep-equal/-/deep-equal-2.2.0.tgz", "integrity": "sha512-RdpzE0Hv4lhowpIUKKMJfeH6C1pXdtT1/it80ubgWqwI3qpuxUBpC1S4hnHg+zjnuOoDkzUtUCEEkG+XG5l3Mw==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "es-get-iterator": "^1.1.2", @@ -14025,7 +14133,7 @@ "version": "1.2.0", "resolved": "https://registry.npmjs.org/define-properties/-/define-properties-1.2.0.tgz", "integrity": "sha512-xvqAVKGfT1+UAvPwKTVw/njhdQ8ZhXK4lI0bCIuCMrp2up9nPnaDftrLtmpTazqd1o+UY4zgzU+avtMbDP+ldA==", - "optional": true, + "devOptional": true, "dependencies": { "has-property-descriptors": "^1.0.0", "object-keys": "^1.1.1" @@ -14235,7 +14343,7 @@ "version": "0.5.16", "resolved": "https://registry.npmjs.org/dom-accessibility-api/-/dom-accessibility-api-0.5.16.tgz", "integrity": "sha512-X7BJ2yElsnOJ30pZF4uIIDfBEVgF4XEBxL9Bxhy6dnrm5hkzqmsWHGTiHqRiITNhMyFLyAiWndIJP7Z1NTteDg==", - "optional": true + "devOptional": true }, "node_modules/dom-converter": { "version": "0.2.0", @@ -14619,7 +14727,7 @@ "version": "1.1.3", "resolved": "https://registry.npmjs.org/es-get-iterator/-/es-get-iterator-1.1.3.tgz", "integrity": "sha512-sPZmqHBe6JIiTfN5q2pEi//TwxmAFHwj/XEuYjTuse78i8KxaqMTTzxPoFKuzRpDpTJ+0NAbpfenkmH2rePtuw==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "get-intrinsic": "^1.1.3", @@ -16254,7 +16362,7 @@ "version": "0.3.3", "resolved": "https://registry.npmjs.org/for-each/-/for-each-0.3.3.tgz", "integrity": "sha512-jqYfLp7mo9vIyQf8ykW2v7A+2N4QjeCeI5+Dz9XraiO1ign81wjiH7Fb9vSOWvQfNtmSa4H2RoQTrrXivdUZmw==", - "optional": true, + "devOptional": true, "dependencies": { "is-callable": "^1.1.3" } @@ -16569,7 +16677,7 @@ "version": "1.2.3", "resolved": "https://registry.npmjs.org/functions-have-names/-/functions-have-names-1.2.3.tgz", "integrity": "sha512-xckBUXyTIqT97tq2x2AMb+g163b5JFysYk0x4qxNFwbfQkmNZoiRHb6sPzI9/QV33WeuvVYBUIiD4NzNIyqaRQ==", - "optional": true, + "devOptional": true, "funding": { "url": "https://github.com/sponsors/ljharb" } @@ -16869,7 +16977,7 @@ "version": "1.0.1", "resolved": "https://registry.npmjs.org/gopd/-/gopd-1.0.1.tgz", "integrity": "sha512-d65bNlIadxvpb/A2abVdlqKqV563juRnZ1Wtk6s1sIR8uNsXR70xqIzVqxVf1eTqDunwT2MkczEeaezCKTZhwA==", - "optional": true, + "devOptional": true, "dependencies": { "get-intrinsic": "^1.1.3" }, @@ -16981,7 +17089,7 @@ "version": "1.0.2", "resolved": "https://registry.npmjs.org/has-bigints/-/has-bigints-1.0.2.tgz", "integrity": "sha512-tSvCKtBr9lkF0Ex0aQiP9N+OpV4zi2r/Nee5VkRDbaqv35RLYMzbwQfFSZZH0kR+Rd6302UJZ2p/bJCEoR3VoQ==", - "optional": true, + "devOptional": true, "funding": { "url": "https://github.com/sponsors/ljharb" } @@ -16999,7 +17107,7 @@ "version": "1.0.0", "resolved": "https://registry.npmjs.org/has-property-descriptors/-/has-property-descriptors-1.0.0.tgz", "integrity": "sha512-62DVLZGoiEBDHQyqG4w9xCuZ7eJEwNmJRWw2VY84Oedb7WFcA27fiEVe8oUQx9hAUJ4ekurquucTGwsyO1XGdQ==", - "optional": true, + "devOptional": true, "dependencies": { "get-intrinsic": "^1.1.1" }, @@ -17035,7 +17143,7 @@ "version": "1.0.0", "resolved": "https://registry.npmjs.org/has-tostringtag/-/has-tostringtag-1.0.0.tgz", "integrity": "sha512-kFjcSNhnlGV1kyoGk7OXKSawH5JOb/LzUc5w9B02hOTO0dfFRjbHQKvg1d6cf3HbeUmtU9VbbV3qzZ2Teh97WQ==", - "optional": true, + "devOptional": true, "dependencies": { "has-symbols": "^1.0.2" }, @@ -17763,7 +17871,7 @@ "version": "1.0.5", "resolved": "https://registry.npmjs.org/internal-slot/-/internal-slot-1.0.5.tgz", "integrity": "sha512-Y+R5hJrzs52QCG2laLn4udYVnxsfny9CpOhNhUvk/SSSVyF6T27FzRbF0sroPidSu3X8oEAkOn2K804mjpt6UQ==", - "optional": true, + "devOptional": true, "dependencies": { "get-intrinsic": "^1.2.0", "has": "^1.0.3", @@ -17826,7 +17934,7 @@ "version": "1.1.1", "resolved": "https://registry.npmjs.org/is-arguments/-/is-arguments-1.1.1.tgz", "integrity": "sha512-8Q7EARjzEnKpt/PCD7e1cgUS0a6X8u5tdSiMqXhojOdoV9TsMsiO+9VLC5vAmO8N7/GmXn7yjR8qnA6bVAEzfA==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "has-tostringtag": "^1.0.0" @@ -17842,7 +17950,7 @@ "version": "3.0.2", "resolved": "https://registry.npmjs.org/is-array-buffer/-/is-array-buffer-3.0.2.tgz", "integrity": "sha512-y+FyyR/w8vfIRq4eQcM1EYgSTnmHXPqaF+IgzgraytCFq5Xh8lllDVmAZolPJiZttZLeFSINPYMaEJ7/vWUa1w==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "get-intrinsic": "^1.2.0", @@ -17862,7 +17970,7 @@ "version": "1.0.4", "resolved": "https://registry.npmjs.org/is-bigint/-/is-bigint-1.0.4.tgz", "integrity": "sha512-zB9CruMamjym81i2JZ3UMn54PKGsQzsJeo6xvN3HJJ4CAsQNB6iRutp2To77OfCNuoxspsIhzaPoO1zyCEhFOg==", - "optional": true, + "devOptional": true, "dependencies": { "has-bigints": "^1.0.1" }, @@ -17885,7 +17993,7 @@ "version": "1.1.2", "resolved": "https://registry.npmjs.org/is-boolean-object/-/is-boolean-object-1.1.2.tgz", "integrity": "sha512-gDYaKHJmnj4aWxyj6YHyXVpdQawtVLHU5cb+eztPGczf6cjuTdwve5ZIEfgXqH4e57An1D1AKf8CZ3kYrQRqYA==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "has-tostringtag": "^1.0.0" @@ -17901,7 +18009,7 @@ "version": "1.2.7", "resolved": "https://registry.npmjs.org/is-callable/-/is-callable-1.2.7.tgz", "integrity": "sha512-1BC0BVFhS/p0qtw6enp8e+8OD0UrK0oFLztSjNzhcKA3WDuJxxAPXzPuPtKkjEY9UUoEWlX/8fgKeu2S8i9JTA==", - "optional": true, + "devOptional": true, "engines": { "node": ">= 0.4" }, @@ -17925,7 +18033,7 @@ "version": "1.0.5", "resolved": "https://registry.npmjs.org/is-date-object/-/is-date-object-1.0.5.tgz", "integrity": "sha512-9YQaSxsAiSwcvS33MBk3wTCVnWK+HhF8VZR2jRxehM16QcVOdHqPn4VPHmRK4lSr38n9JriurInLcP90xsYNfQ==", - "optional": true, + "devOptional": true, "dependencies": { "has-tostringtag": "^1.0.0" }, @@ -18081,7 +18189,7 @@ "version": "2.0.2", "resolved": "https://registry.npmjs.org/is-map/-/is-map-2.0.2.tgz", "integrity": "sha512-cOZFQQozTha1f4MxLFzlgKYPTyj26picdZTx82hbc/Xf4K/tZOOXSCkMvU4pKioRXGDLJRn0GM7Upe7kR721yg==", - "optional": true, + "devOptional": true, "funding": { "url": "https://github.com/sponsors/ljharb" } @@ -18132,7 +18240,7 @@ "version": "1.0.7", "resolved": "https://registry.npmjs.org/is-number-object/-/is-number-object-1.0.7.tgz", "integrity": "sha512-k1U0IRzLMo7ZlYIfzRu23Oh6MiIFasgpb9X76eqfFZAqwH44UI4KTBvBYIZ1dSL9ZzChTB9ShHfLkR4pdW5krQ==", - "optional": true, + "devOptional": true, "dependencies": { "has-tostringtag": "^1.0.0" }, @@ -18192,7 +18300,7 @@ "version": "1.1.4", "resolved": "https://registry.npmjs.org/is-regex/-/is-regex-1.1.4.tgz", "integrity": "sha512-kvRdxDsxZjhzUX07ZnLydzS1TU/TJlTUHHY4YLL87e37oUA49DfkLqgy+VjFocowy29cKvcSiu+kIv728jTTVg==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "has-tostringtag": "^1.0.0" @@ -18208,7 +18316,7 @@ "version": "2.0.2", "resolved": "https://registry.npmjs.org/is-set/-/is-set-2.0.2.tgz", "integrity": "sha512-+2cnTEZeY5z/iXGbLhPrOAaK/Mau5k5eXq9j14CpRTftq0pAJu2MwVRSZhyZWBzx3o6X795Lz6Bpb6R0GKf37g==", - "optional": true, + "devOptional": true, "funding": { "url": "https://github.com/sponsors/ljharb" } @@ -18217,7 +18325,7 @@ "version": "1.0.2", "resolved": "https://registry.npmjs.org/is-shared-array-buffer/-/is-shared-array-buffer-1.0.2.tgz", "integrity": "sha512-sqN2UDu1/0y6uvXyStCOzyhAjCSlHceFoMKJW8W9EU9cvic/QdsZ0kEU93HEy3IUEFZIiH/3w+AH/UQbPHNdhA==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2" }, @@ -18241,7 +18349,7 @@ "version": "1.0.7", "resolved": "https://registry.npmjs.org/is-string/-/is-string-1.0.7.tgz", "integrity": "sha512-tE2UXzivje6ofPW7l23cjDOMa09gb7xlAqG6jG5ej6uPV32TlWP3NKPigtaGeHNu9fohccRYvIiZMfOOnOYUtg==", - "optional": true, + "devOptional": true, "dependencies": { "has-tostringtag": "^1.0.0" }, @@ -18256,7 +18364,7 @@ "version": "1.0.4", "resolved": "https://registry.npmjs.org/is-symbol/-/is-symbol-1.0.4.tgz", "integrity": "sha512-C/CPBqKWnvdcxqIARxyOh4v1UUEOCHpgDa0WYgpKDFMszcrPcffg5uhwSgPCLD2WWxmq6isisz87tzT01tuGhg==", - "optional": true, + "devOptional": true, "dependencies": { "has-symbols": "^1.0.2" }, @@ -18271,7 +18379,7 @@ "version": "1.1.10", "resolved": "https://registry.npmjs.org/is-typed-array/-/is-typed-array-1.1.10.tgz", "integrity": "sha512-PJqgEHiWZvMpaFZ3uTc8kHPM4+4ADTlDniuQL7cU/UDA0Ql7F70yGfHph3cLNe+c9toaigv+DFzTJKhc2CtO6A==", - "optional": true, + "devOptional": true, "dependencies": { "available-typed-arrays": "^1.0.5", "call-bind": "^1.0.2", @@ -18320,7 +18428,7 @@ "version": "2.0.1", "resolved": "https://registry.npmjs.org/is-weakmap/-/is-weakmap-2.0.1.tgz", "integrity": "sha512-NSBR4kH5oVj1Uwvv970ruUkCV7O1mzgVFO4/rev2cLRda9Tm9HrL70ZPut4rOHgY0FNrUu9BCbXA2sdQ+x0chA==", - "optional": true, + "devOptional": true, "funding": { "url": "https://github.com/sponsors/ljharb" } @@ -18341,7 +18449,7 @@ "version": "2.0.2", "resolved": "https://registry.npmjs.org/is-weakset/-/is-weakset-2.0.2.tgz", "integrity": "sha512-t2yVvttHkQktwnNNmBQ98AhENLdPUTDTE21uPqAQ0ARwQfGeQKRVS0NNurH7bTf7RrvcVn1OOge45CnBeHCSmg==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "get-intrinsic": "^1.1.1" @@ -18381,7 +18489,7 @@ "version": "2.0.5", "resolved": "https://registry.npmjs.org/isarray/-/isarray-2.0.5.tgz", "integrity": "sha512-xHjhDr3cNBK0BzdUJSPXZntQUx/mwMS5Rw4A7lPJ90XGAO6ISP/ePDNuo0vhqOZU+UD5JoodwCAAoZQd3FeAKw==", - "optional": true + "devOptional": true }, "node_modules/isexe": { "version": "2.0.0", @@ -23067,7 +23175,7 @@ "version": "1.5.0", "resolved": "https://registry.npmjs.org/lz-string/-/lz-string-1.5.0.tgz", "integrity": "sha512-h5bgJWpxJNswbU7qCrV0tIKQCaS3blPDrqKWx+QxzuzL1zGUzij9XCWLrSLsJPu5t+eWA/ycetzYAO5IOMcWAQ==", - "optional": true, + "devOptional": true, "bin": { "lz-string": "bin/bin.js" } @@ -24330,7 +24438,7 @@ "version": "1.1.5", "resolved": "https://registry.npmjs.org/object-is/-/object-is-1.1.5.tgz", "integrity": "sha512-3cyDsyHgtmi7I7DfSSI2LDp6SK2lwvtbg0p0R1e0RvTqF5ceGx+K2dfSjm1bKDMVCFEDAQvy+o8c6a7VujOddw==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "define-properties": "^1.1.3" @@ -24346,7 +24454,7 @@ "version": "1.1.1", "resolved": "https://registry.npmjs.org/object-keys/-/object-keys-1.1.1.tgz", "integrity": "sha512-NuAESUOUMrlIXOfHKzD6bpPu3tYt3xvjNdRIQ+FeT0lNb4K8WR70CaDxhuNguS2XG+GjkyMwOzsN5ZktImfhLA==", - "optional": true, + "devOptional": true, "engines": { "node": ">= 0.4" } @@ -24355,7 +24463,7 @@ "version": "4.1.4", "resolved": "https://registry.npmjs.org/object.assign/-/object.assign-4.1.4.tgz", "integrity": "sha512-1mxKf0e58bvyjSCtKYY4sRe9itRk3PJpquJOjeIkz885CczcI4IvJJDLPS72oowuSh+pBxUFROpX+TU++hxhZQ==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "define-properties": "^1.1.4", @@ -25364,7 +25472,7 @@ "version": "27.5.1", "resolved": "https://registry.npmjs.org/pretty-format/-/pretty-format-27.5.1.tgz", "integrity": "sha512-Qb1gy5OrP5+zDf2Bvnzdl3jsTf1qXVMazbvCoKhtKqVs4/YK4ozX4gKQJJVyNe+cajNPn0KoC0MC3FUmaHWEmQ==", - "optional": true, + "devOptional": true, "dependencies": { "ansi-regex": "^5.0.1", "ansi-styles": "^5.0.0", @@ -25378,7 +25486,7 @@ "version": "5.2.0", "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-5.2.0.tgz", "integrity": "sha512-Cxwpt2SfTzTtXcfOlzGEee8O+c+MmUgGrNiBcXnuWxuFJHe6a5Hz7qwhwe5OgaSYI0IJvkLqWX1ASG+cJOkEiA==", - "optional": true, + "devOptional": true, "engines": { "node": ">=10" }, @@ -25390,7 +25498,7 @@ "version": "17.0.2", "resolved": "https://registry.npmjs.org/react-is/-/react-is-17.0.2.tgz", "integrity": "sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w==", - "optional": true + "devOptional": true }, "node_modules/pretty-hrtime": { "version": "1.0.3", @@ -26323,7 +26431,7 @@ "version": "1.4.3", "resolved": "https://registry.npmjs.org/regexp.prototype.flags/-/regexp.prototype.flags-1.4.3.tgz", "integrity": "sha512-fjggEOO3slI6Wvgjwflkc4NFRCTZAu5CnNfBd5qOMYhWdn67nJBBu34/TkD++eeFmd8C9r9jfXJ27+nSiRkSUA==", - "optional": true, + "devOptional": true, "dependencies": { "call-bind": "^1.0.2", "define-properties": "^1.1.3", @@ -27590,7 +27698,7 @@ "version": "1.0.0", "resolved": "https://registry.npmjs.org/stop-iteration-iterator/-/stop-iteration-iterator-1.0.0.tgz", "integrity": "sha512-iCGQj+0l0HOdZ2AEeBADlsRC+vsnDsZsbdSiH1yNSjcfKM7fdpCMfqAL/dwF5BLiw/XhRft/Wax6zQbhq2BcjQ==", - "optional": true, + "devOptional": true, "dependencies": { "internal-slot": "^1.0.4" }, @@ -30533,7 +30641,7 @@ "version": "1.0.2", "resolved": "https://registry.npmjs.org/which-boxed-primitive/-/which-boxed-primitive-1.0.2.tgz", "integrity": "sha512-bwZdv0AKLpplFY2KZRX6TvyuN7ojjr7lwkg6ml0roIy9YeuSr7JS372qlNW18UQYzgYK9ziGcerWqZOmEn9VNg==", - "optional": true, + "devOptional": true, "dependencies": { "is-bigint": "^1.0.1", "is-boolean-object": "^1.1.0", @@ -30549,7 +30657,7 @@ "version": "1.0.1", "resolved": "https://registry.npmjs.org/which-collection/-/which-collection-1.0.1.tgz", "integrity": "sha512-W8xeTUwaln8i3K/cY1nGXzdnVZlidBcagyNFtBdD5kxnb4TvGKR7FfSIS3mYpwWS1QUCutfKz8IY8RjftB0+1A==", - "optional": true, + "devOptional": true, "dependencies": { "is-map": "^2.0.1", "is-set": "^2.0.1", @@ -30569,7 +30677,7 @@ "version": "1.1.9", "resolved": "https://registry.npmjs.org/which-typed-array/-/which-typed-array-1.1.9.tgz", "integrity": "sha512-w9c4xkx6mPidwp7180ckYWfMmvxpjlZuIudNtDf4N/tTAUB8VJbX25qZoAsrtGuYNnGw3pa0AXgbGKRB8/EceA==", - "optional": true, + "devOptional": true, "dependencies": { "available-typed-arrays": "^1.0.5", "call-bind": "^1.0.2", diff --git a/frontend/package.json b/frontend/package.json index 09d626ac4a..c0c0fce8bb 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -77,6 +77,7 @@ }, "devDependencies": { "@babel/core": "^7.21.0", + "@testing-library/react": "^14.0.0", "@types/dompurify": "^2.2.6", "@types/lodash-es": "^4.17.8", "@types/node": "^17.0.29", diff --git a/frontend/src/__tests__/unit/testUtils/hooks.spec.ts b/frontend/src/__tests__/unit/testUtils/hooks.spec.ts new file mode 100644 index 0000000000..dac0f2897a --- /dev/null +++ b/frontend/src/__tests__/unit/testUtils/hooks.spec.ts @@ -0,0 +1,163 @@ +import * as React from 'react'; +import { expectHook, renderHook, standardUseFetchState, testHook } from './hooks'; + +const useSayHello = (who: string, showCount = false) => { + const countRef = React.useRef(0); + countRef.current++; + return `Hello ${who}!${showCount && countRef.current > 1 ? ` x${countRef.current}` : ''}`; +}; + +const useSayHelloDelayed = (who: string, delay = 0) => { + const [speech, setSpeech] = React.useState(''); + React.useEffect(() => { + const handle = setTimeout(() => setSpeech(`Hello ${who}!`), delay); + return () => clearTimeout(handle); + }, [who, delay]); + return speech; +}; + +describe('hook test utils', () => { + it('simple testHook', () => { + const renderResult = testHook((who: string) => `Hello ${who}!`, 'world'); + expectHook(renderResult).toBe('Hello world!').toHaveUpdateCount(1); + renderResult.rerender('world'); + expectHook(renderResult).toBe('Hello world!').toBeStable().toHaveUpdateCount(2); + }); + + it('use testHook for rendering', () => { + const renderResult = testHook(useSayHello, 'world'); + expectHook(renderResult) + .toHaveUpdateCount(1) + .toBe('Hello world!') + .toStrictEqual('Hello world!'); + + renderResult.rerender('world', false); + + expectHook(renderResult) + .toHaveUpdateCount(2) + .toBe('Hello world!') + .toStrictEqual('Hello world!') + .toBeStable(); + + renderResult.rerender('world', true); + + expectHook(renderResult) + .toHaveUpdateCount(3) + .toBe('Hello world! x3') + .toStrictEqual('Hello world! x3') + .toBeStable(false); + }); + + it('use renderHook for rendering', () => { + type Props = { + who: string; + showCount?: boolean; + }; + const renderResult = renderHook(({ who, showCount }: Props) => useSayHello(who, showCount), { + initialProps: { + who: 'world', + }, + }); + + expectHook(renderResult) + .toHaveUpdateCount(1) + .toBe('Hello world!') + .toStrictEqual('Hello world!'); + + renderResult.rerender({ + who: 'world', + }); + + expectHook(renderResult) + .toHaveUpdateCount(2) + .toBe('Hello world!') + .toStrictEqual('Hello world!') + .toBeStable(); + + renderResult.rerender({ who: 'world', showCount: true }); + + expectHook(renderResult) + .toHaveUpdateCount(3) + .toBe('Hello world! x3') + .toStrictEqual('Hello world! x3') + .toBeStable(false); + }); + + it('should use waitForNextUpdate for async update testing', async () => { + const renderResult = testHook(useSayHelloDelayed, 'world'); + expectHook(renderResult).toHaveUpdateCount(1).toBe(''); + + await renderResult.waitForNextUpdate(); + expectHook(renderResult).toHaveUpdateCount(2).toBe('Hello world!'); + }); + + it('should throw error if waitForNextUpdate times out', async () => { + const renderResult = renderHook(() => useSayHelloDelayed('', 20)); + + await expect(renderResult.waitForNextUpdate({ timeout: 10, interval: 5 })).rejects.toThrow(); + expectHook(renderResult).toHaveUpdateCount(1); + + // unmount to test waiting for an update that will never happen + renderResult.unmount(); + + await expect(renderResult.waitForNextUpdate({ timeout: 50, interval: 10 })).rejects.toThrow(); + + expectHook(renderResult).toHaveUpdateCount(1); + }); + + it('should not throw if waitForNextUpdate timeout is sufficient', async () => { + const renderResult = renderHook(() => useSayHelloDelayed('', 20)); + + await expect( + renderResult.waitForNextUpdate({ timeout: 50, interval: 10 }), + ).resolves.not.toThrow(); + + expectHook(renderResult).toHaveUpdateCount(2); + }); + + it('should assert stability of results using isStable', () => { + let testValue = 'test'; + const renderResult = renderHook(() => testValue); + expectHook(renderResult).toHaveUpdateCount(1); + + renderResult.rerender(); + expectHook(renderResult).toHaveUpdateCount(2).toBeStable(true); + + testValue = 'new'; + renderResult.rerender(); + expectHook(renderResult).toHaveUpdateCount(3).toBeStable(false); + + renderResult.rerender(); + expectHook(renderResult).toHaveUpdateCount(4).toBeStable(true); + }); + + it('should assert stability of results using isStableArray', () => { + let testValue = 'test'; + // explicitly returns a new array each render to show the difference between `isStable` and `isStableArray` + const renderResult = renderHook(() => [testValue]); + expectHook(renderResult).toHaveUpdateCount(1); + + renderResult.rerender(); + expectHook(renderResult).toHaveUpdateCount(2).toBeStable(false); + expectHook(renderResult).toHaveUpdateCount(2).toBeStable([true]); + + testValue = 'new'; + renderResult.rerender(); + expectHook(renderResult).toHaveUpdateCount(3).toBeStable(false); + expectHook(renderResult).toHaveUpdateCount(3).toBeStable([false]); + + renderResult.rerender(); + expectHook(renderResult).toHaveUpdateCount(4).toBeStable(false); + expectHook(renderResult).toHaveUpdateCount(4).toBeStable([true]); + }); + + it('standardUseFetchState should return an array matching the state of useFetchState', () => { + expect(['test', false, undefined, () => null]).toStrictEqual(standardUseFetchState('test')); + expect(['test', true, undefined, () => null]).toStrictEqual( + standardUseFetchState('test', true), + ); + expect(['test', false, new Error('error'), () => null]).toStrictEqual( + standardUseFetchState('test', false, new Error('error')), + ); + }); +}); diff --git a/frontend/src/__tests__/unit/testUtils/hooks.ts b/frontend/src/__tests__/unit/testUtils/hooks.ts new file mode 100644 index 0000000000..4f13d7ee67 --- /dev/null +++ b/frontend/src/__tests__/unit/testUtils/hooks.ts @@ -0,0 +1,253 @@ +import { + renderHook as renderHookRTL, + RenderHookOptions, + RenderHookResult, + waitFor, + waitForOptions, +} from '@testing-library/react'; +import { queries, Queries } from '@testing-library/dom'; + +/** + * Set of helper functions used to perform assertions on the hook result. + */ +export type RenderHookResultExpect = { + /** + * Check that a value is what you expect. It uses `Object.is` to check strict equality. + * Don't use `toBe` with floating-point numbers. + */ + toBe: (expected: Result) => RenderHookResultExpect; + + /** + * Check that the result has the same types as well as structure. + */ + toStrictEqual: (expected: Result) => RenderHookResultExpect; + + /** + * Check the stability of the result. + * If the expected value is a boolean array, uses `isStableArray` for comparison, otherwise uses `isStable`. + * + * Stability is checked against the previous update. + */ + toBeStable: (expected?: boolean | boolean[]) => RenderHookResultExpect; + + /** + * Check the update count is the expected number. + * Update count increases every time the hook is called. + */ + toHaveUpdateCount: (expected: number) => RenderHookResultExpect; +}; + +/** + * Extension of RTL RenderHookResult providing functions used query the current state of the result. + */ +export type RenderHookResultExt = RenderHookResult & { + /** + * Returns `true` if the previous result is equal to the current result. Uses `Object.is` for comparison. + */ + isStable: () => boolean; + + /** + * Returns `true` if the previous result array items are equal to the current result array items. Uses `Object.is` for comparison. + * The equality of the array instances is not checked. + */ + isStableArray: () => boolean[]; + + /** + * Get the update count for how many times the hook has been rendered. + * An update occurs initially on render, subsequently when re-rendered, and also whenever the hook itself triggers a re-render. + * eg. An `useEffect` triggering a state update. + */ + getUpdateCount: () => number; + + /** + * Returns a Promise that resolves the next time the hook renders, commonly when state is updated as the result of an asynchronous update. + * + * Since `waitForNextUpdate` works using interval checks (backed by `waitFor`), it's possible that multiple updates may occur while waiting. + */ + waitForNextUpdate: (options?: Pick) => Promise; +}; + +/** + * Helper function that wraps a render result and provides a small set of jest Matcher equivalent functions that act directly on the result. + * + * ``` + * expectHook(renderResult).toBeStable().toHaveUpdateCount(2); + * ``` + * Equivalent to: + * ``` + * expect(renderResult.isStable()).toBe(true); + * expect(renderResult.getUpdateCount()).toBe(2); + * ``` + * + * See `RenderHookResultExpect` + */ +export const expectHook = ( + renderResult: Pick< + RenderHookResultExt, + 'result' | 'getUpdateCount' | 'isStableArray' | 'isStable' + >, +): RenderHookResultExpect => { + const expectUtil: RenderHookResultExpect = { + toBe: (expected) => { + expect(renderResult.result.current).toBe(expected); + return expectUtil; + }, + + toStrictEqual: (expected) => { + expect(renderResult.result.current).toStrictEqual(expected); + return expectUtil; + }, + + toBeStable: (expected = true) => { + if (renderResult.getUpdateCount() > 1) { + if (Array.isArray(expected)) { + expect(renderResult.isStableArray()).toStrictEqual(expected); + } else { + expect(renderResult.isStable()).toBe(expected); + } + } else { + // eslint-disable-next-line no-console + console.warn( + 'expectHook#toBeStable cannot assert stability as the hook has not run at least 2 times.', + ); + } + return expectUtil; + }, + + toHaveUpdateCount: (expected) => { + expect(renderResult.getUpdateCount()).toBe(expected); + return expectUtil; + }, + }; + return expectUtil; +}; + +/** + * Wrapper on top of RTL `renderHook` returning a result that implements the `RenderHookResultExt` interface. + * + * `renderHook` provides full control over the rendering of your hook including the ability to wrap the test component. + * This is usually used to add context providers from `React.createContext` for the hook to access with `useContext`. + * `initialProps` and props subsequently set by `rerender` will be provided to the wrapper. + * + * ``` + * const renderResult = renderHook(({ who }: { who: string }) => useSayHello(who), { initialProps: { who: 'world' }}); + * expectHook(renderResult).toBe('Hello world!'); + * renderResult.rerender({ who: 'there' }); + * expectHook(renderResult).toBe('Hello there!'); + * ``` + */ +export const renderHook = < + Result, + Props, + Q extends Queries = typeof queries, + Container extends Element | DocumentFragment = HTMLElement, + BaseElement extends Element | DocumentFragment = Container, +>( + render: (initialProps: Props) => Result, + options?: RenderHookOptions, +): RenderHookResultExt => { + let updateCount = 0; + let prevResult: Result | undefined; + let currentResult: Result | undefined; + + const renderResult = renderHookRTL((props) => { + updateCount++; + prevResult = currentResult; + currentResult = render(props); + return currentResult; + }, options); + + const renderResultExt: RenderHookResultExt = { + ...renderResult, + + isStable: () => (updateCount > 1 ? Object.is(renderResult.result.current, prevResult) : false), + + isStableArray: () => { + // prefill return array with `false` + const stable: boolean[] = Array( + Math.max( + Array.isArray(prevResult) ? prevResult?.length : 0, + Array.isArray(renderResult.result.current) ? renderResult.result.current.length : 0, + ), + ).fill(false); + + if ( + updateCount > 1 && + Array.isArray(prevResult) && + Array.isArray(renderResult.result.current) + ) { + renderResult.result.current.forEach((v, i) => { + stable[i] = Object.is(v, (prevResult as unknown[])[i]); + }); + } + return stable; + }, + + getUpdateCount: () => updateCount, + + waitForNextUpdate: async (options) => { + const expected = updateCount; + try { + await waitFor(() => expect(updateCount).toBeGreaterThan(expected), options); + } catch { + throw new Error('waitForNextUpdate timed out'); + } + }, + }; + + return renderResultExt; +}; + +/** + * Lightweight API for testing a single hook. + * + * Prefer this method of testing over `renderHook` for simplicity. + * + * ``` + * const renderResult = testHook(useSayHello, 'world'); + * expectHook(renderResult).toBe('Hello world!'); + * renderResult.rerender('there'); + * expectHook(renderResult).toBe('Hello there!'); + * ``` + */ + +export const testHook = Result, P extends unknown[]>( + hook: (...params: P) => Result, + ...initialParams: Parameters +) => { + type Params = Parameters; + const renderResult = renderHook(({ $params }: { $params: Params }) => hook(...$params), { + initialProps: { + $params: initialParams, + }, + }); + + return { + ...renderResult, + + rerender: (...params: Params) => renderResult.rerender({ $params: params }), + }; +}; + +/** + * A helper function for asserting the return value of hooks based on `useFetchState`. + * + * eg. + * ``` + * expectHook(renderResult).isStrictEqual(standardUseFetchState('test value', true)) + * ``` + * is equivalent to: + * ``` + * expectHook(renderResult).isStrictEqual(['test value', true, undefined, expect.any(Function)]) + * ``` + */ +export const standardUseFetchState = ( + data: D, + loaded = false, + error?: Error, +): [ + data: D, + loaded: boolean, + loadError: Error | undefined, + refresh: () => Promise, +] => [data, loaded, error, expect.any(Function)]; diff --git a/frontend/src/__tests__/unit/useFetchState.spec.ts b/frontend/src/__tests__/unit/useFetchState.spec.ts new file mode 100644 index 0000000000..220e4bccf7 --- /dev/null +++ b/frontend/src/__tests__/unit/useFetchState.spec.ts @@ -0,0 +1,97 @@ +import { act } from '@testing-library/react'; +import useFetchState from '~/utilities/useFetchState'; +import { expectHook, standardUseFetchState, testHook } from './testUtils/hooks'; + +jest.useFakeTimers(); + +describe('useFetchState', () => { + it('should be successful', async () => { + const renderResult = testHook( + useFetchState, + () => Promise.resolve('success-test-state'), + 'default-test-state', + ); + + expectHook(renderResult) + .toStrictEqual(standardUseFetchState('default-test-state')) + .toHaveUpdateCount(1); + + await renderResult.waitForNextUpdate(); + + expectHook(renderResult) + .toStrictEqual(standardUseFetchState('success-test-state', true)) + .toHaveUpdateCount(2) + .toBeStable([false, false, true, true]); + }); + + it('should fail', async () => { + const renderResult = testHook( + useFetchState, + () => Promise.reject(new Error('error-test-state')), + 'default-test-state', + ); + + expectHook(renderResult) + .toStrictEqual(standardUseFetchState('default-test-state')) + .toHaveUpdateCount(1); + + await renderResult.waitForNextUpdate(); + + expectHook(renderResult) + .toStrictEqual( + standardUseFetchState('default-test-state', false, new Error('error-test-state')), + ) + .toHaveUpdateCount(2) + .toBeStable([true, true, false, true]); + }); + + it('should refresh', async () => { + const renderResult = testHook(useFetchState, () => Promise.resolve([1, 2, 3]), [], { + refreshRate: 1000, + }); + expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + + await renderResult.waitForNextUpdate(); + + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([1, 2, 3], true)) + .toHaveUpdateCount(2) + .toBeStable([false, false, true, true]); + + await act(() => { + jest.advanceTimersByTime(900); + }); + expectHook(renderResult).toHaveUpdateCount(2); + + await act(async () => { + jest.advanceTimersByTime(100); + }); + expectHook(renderResult).toHaveUpdateCount(3); + + await renderResult.waitForNextUpdate(); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([1, 2, 3], true)) + .toHaveUpdateCount(4) + .toBeStable([false, true, true, true]); + }); + + it('should test stability', async () => { + const renderResult = testHook(useFetchState, () => Promise.resolve([1, 2, 3]), []); + expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + + await renderResult.waitForNextUpdate(); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([1, 2, 3], true)) + .toHaveUpdateCount(2) + .toBeStable([false, false, true, true]); + + renderResult.rerender(() => Promise.resolve([1, 2, 4]), []); + expectHook(renderResult).toHaveUpdateCount(3).toBeStable([true, true, true, true]); + + await renderResult.waitForNextUpdate(); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([1, 2, 4], true)) + .toHaveUpdateCount(4) + .toBeStable([false, true, true, true]); + }); +}); diff --git a/frontend/src/__tests__/unit/useGroups.spec.ts b/frontend/src/__tests__/unit/useGroups.spec.ts new file mode 100644 index 0000000000..200531a10f --- /dev/null +++ b/frontend/src/__tests__/unit/useGroups.spec.ts @@ -0,0 +1,125 @@ +import { act } from '@testing-library/react'; +import { k8sListResource } from '@openshift/dynamic-plugin-sdk-utils'; +import useGroups from '~/pages/projects/projectSharing/useGroups'; +import { GroupKind } from '~/k8sTypes'; +import { expectHook, standardUseFetchState, testHook } from './testUtils/hooks'; + +jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ + k8sListResource: jest.fn(), +})); + +const k8sListResourceMock = k8sListResource as jest.Mock; + +describe('useGroups', () => { + it('should return successful list of groups', async () => { + const mockList = { + items: [{ metadata: { name: 'item 1' } }, { metadata: { name: 'item 2' } }] as GroupKind[], + }; + k8sListResourceMock.mockReturnValue(Promise.resolve(mockList)); + + const renderResult = testHook(useGroups); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState(mockList.items, true)) + .toHaveUpdateCount(2) + .toBeStable([false, false, true, true]); + + // refresh + k8sListResourceMock.mockReturnValue(Promise.resolve({ items: [...mockList.items] })); + await act(() => renderResult.result.current[3]()); + expect(k8sListResourceMock).toHaveBeenCalledTimes(2); + expectHook(renderResult).toHaveUpdateCount(3).toBeStable([false, true, true, true]); + }); + + it('should handle 403 as an empty result', async () => { + const error = { + statusObject: { + code: 403, + }, + }; + k8sListResourceMock.mockReturnValue(Promise.reject(error)); + + const renderResult = testHook(useGroups); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([], true)) + .toHaveUpdateCount(2) + .toBeStable([false, false, true, true]); + + // refresh + await act(() => renderResult.result.current[3]()); + // error 403 should cache error and prevent subsequent attempts to fetch + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([], true)) + .toHaveUpdateCount(3) + .toBeStable([false, true, true, true]); + }); + + it('should handle 404 as an error', async () => { + const error = { + statusObject: { + code: 404, + }, + }; + k8sListResourceMock.mockReturnValue(Promise.reject(error)); + + const renderResult = testHook(useGroups); + + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([], false, new Error('No groups found.'))) + .toHaveUpdateCount(2) + .toBeStable([true, true, false, true]); + + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + + // refresh + await act(() => renderResult.result.current[3]()); + expect(k8sListResourceMock).toHaveBeenCalledTimes(2); + // we get a new error because the k8s API is called a 2nd time + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([], false, new Error('No groups found.'))) + .toHaveUpdateCount(3) + .toBeStable([true, true, false, true]); + }); + + it('should handle other errors and rethrow', async () => { + k8sListResourceMock.mockReturnValue(Promise.reject(new Error('error1'))); + + const renderResult = testHook(useGroups); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([], false, new Error('error1'))) + .toHaveUpdateCount(2) + .toBeStable([true, true, false, true]); + + // refresh + k8sListResourceMock.mockReturnValue(Promise.reject(new Error('error2'))); + await act(() => renderResult.result.current[3]()); + expect(k8sListResourceMock).toHaveBeenCalledTimes(2); + expectHook(renderResult) + .toStrictEqual(standardUseFetchState([], false, new Error('error2'))) + .toHaveUpdateCount(3) + .toBeStable([true, true, false, true]); + }); +}); diff --git a/frontend/src/pages/TestPage.tsx.bak b/frontend/src/pages/TestPage.tsx.bak new file mode 100644 index 0000000000..ce068f1c1d --- /dev/null +++ b/frontend/src/pages/TestPage.tsx.bak @@ -0,0 +1,55 @@ +import { K8sModelCommon, useK8sWatchResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { Alert, AlertVariant, Button } from '@patternfly/react-core'; +import * as React from 'react'; +import { ProjectKind } from '~/k8sTypes'; + +const errorMessage = (e: unknown): string => + (typeof e === 'object' ? e?.toString() : typeof e === 'string' ? e : '') || ''; + +const ProjectModel: K8sModelCommon = { + apiVersion: 'v1', + apiGroup: 'project.openshift.io', + kind: 'Project', + plural: 'projects', +}; +const TestPage = () => { + new Object(); + const [limit, setLimit] = React.useState(100); + const [projects, loaded, error] = useK8sWatchResource( + { + groupVersionKind: { + group: 'project.openshift.io', + version: 'v1', + kind: 'Project', + }, + limit, + isList: true, + }, + ProjectModel, + ); + return ( + <> +
+ +
+ {!loaded ?
Loading...
: null} + {error ? ( + + {errorMessage(error)} + + ) : null} +
    + {Array.isArray(projects) + ? projects.map((p) =>
  1. {p.metadata.name}
    2. ) + : null} +
+ + ); +}; + +export default TestPage; diff --git a/frontend/src/utilities/useFetchState.ts b/frontend/src/utilities/useFetchState.ts index 3e723d1014..03c0c3da2f 100644 --- a/frontend/src/utilities/useFetchState.ts +++ b/frontend/src/utilities/useFetchState.ts @@ -223,12 +223,16 @@ const useFetchState = ( }; }, [call, refreshRate]); + // Use a reference for `call` to ensure a stable reference to `refresh` is always returned + const callRef = React.useRef(call); + callRef.current = call; + const refresh = React.useCallback>(() => { abortCallbackRef.current(); - const [callPromise, unload] = call(); + const [callPromise, unload] = callRef.current(); abortCallbackRef.current = unload; return callPromise; - }, [call]); + }, []); return [result, loaded, loadError, refresh]; }; From 8c2ca6340356cbbbb2ddf4974b0389c954d61e2b Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Wed, 30 Aug 2023 14:01:43 -0500 Subject: [PATCH 08/23] Added pipeline run param tab change display name of tab added bold made keys bold --- .../PipelineRunDrawerBottomTabs.tsx | 10 ++++ .../pipelineRun/PipelineRunTabParameters.tsx | 52 +++++++++++++++++++ .../pipelinesDetails/pipelineRun/utils.tsx | 4 +- .../taskDetails/TaskDetailsPrintKeyValues.tsx | 4 +- 4 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunTabParameters.tsx diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerBottomTabs.tsx b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerBottomTabs.tsx index b795747ee6..99c470cd79 100644 --- a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerBottomTabs.tsx +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerBottomTabs.tsx @@ -4,9 +4,11 @@ import PipelineRunTabDetails from '~/concepts/pipelines/content/pipelinesDetails import PipelineDetailsYAML from '~/concepts/pipelines/content/pipelinesDetails/PipelineDetailsYAML'; import { PipelineRunKind } from '~/k8sTypes'; import { PipelineRunKF } from '~/concepts/pipelines/kfTypes'; +import PipelineRunTabParameters from './PipelineRunTabParameters'; export enum RunDetailsTabs { DETAILS = 'Details', + PARAMETERS = 'Input parameters', YAML = 'Run output', } @@ -51,6 +53,14 @@ export const PipelineRunDrawerBottomTabs: React.FC pipelineRunKF={pipelineRunDetails?.kf} /> + = ({ pipelineRunKF }) => { + if (!pipelineRunKF) { + return ( + + + + Loading + + + ); + } + + if ( + !pipelineRunKF?.pipeline_spec.parameters || + pipelineRunKF.pipeline_spec.parameters.length === 0 + ) { + return ( + + + No parameters + + This pipeline run does not have any parameters defined. + + ); + } + + const details: DetailItem[] = pipelineRunKF.pipeline_spec.parameters.map((param) => ({ + key: param.name, + value: param.value, + })); + + return <>{renderDetailItems(details)}; +}; + +export default PipelineRunTabParameters; diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/utils.tsx b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/utils.tsx index a1a90e5d15..b4b18c6cca 100644 --- a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/utils.tsx +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/utils.tsx @@ -20,7 +20,9 @@ export const renderDetailItems = (details: DetailItem[], flexKey?: boolean): Rea {details.map((detail) => ( - {detail.key} + + {detail.key} + {detail.value} diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/taskDetails/TaskDetailsPrintKeyValues.tsx b/frontend/src/concepts/pipelines/content/pipelinesDetails/taskDetails/TaskDetailsPrintKeyValues.tsx index de6b814cc6..1e53303134 100644 --- a/frontend/src/concepts/pipelines/content/pipelinesDetails/taskDetails/TaskDetailsPrintKeyValues.tsx +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/taskDetails/TaskDetailsPrintKeyValues.tsx @@ -9,7 +9,9 @@ const TaskDetailsPrintKeyValues: React.FC = ({ i {items.map((result, i) => ( - {result.name} + + {result.name} + {result.value} ))} From 604a02cd99a5f401f983cf23aa7cd64f71779f47 Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Mon, 11 Sep 2023 09:42:51 -0400 Subject: [PATCH 09/23] co-locate unit tests with their target source --- docs/architecture.md | 14 +++-- frontend/jest.config.js | 7 ++- .../src/__tests__/dockerRepositoryURL.spec.ts | 49 ----------------- frontend/src/pages/TestPage.tsx.bak | 55 ------------------- .../__tests__}/useGroups.spec.ts | 2 +- .../src/utilities/__tests__/const.spec.ts | 51 +++++++++++++++++ .../notebookControllerUtils.spec.ts | 0 .../__tests__}/useFetchState.spec.ts | 2 +- 8 files changed, 68 insertions(+), 112 deletions(-) delete mode 100644 frontend/src/__tests__/dockerRepositoryURL.spec.ts delete mode 100644 frontend/src/pages/TestPage.tsx.bak rename frontend/src/{__tests__/unit => pages/projects/projectSharing/__tests__}/useGroups.spec.ts (97%) create mode 100644 frontend/src/utilities/__tests__/const.spec.ts rename frontend/src/{__tests__/unit => utilities/__tests__}/notebookControllerUtils.spec.ts (100%) rename frontend/src/{__tests__/unit => utilities/__tests__}/useFetchState.spec.ts (96%) diff --git a/docs/architecture.md b/docs/architecture.md index 3bde9492ce..564a3af313 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -100,20 +100,26 @@ When building new client features, there are a few things worth noting about the Tests can be divided into the following categories: unit, integration, accessibility, and end to end testing. To keep organized of the different types of tests, there will be a test folder at the root of the frontend project with the following structure. +E2e and integration tests are located in a single root directory: ``` -/frontend/tests - /integration => ComponentName.stories.tsx, ComponentName.spec.ts - /unit => functionName.test.ts +/frontend/src/__tests__ /e2e => storyName.spec.ts + /integration => ComponentName.stories.tsx, ComponentName.spec.ts ``` Some nesting can be used to organize testing groups together. For example, the _projects_ page has screens for _details_, _projects_, and, _spawner_ which can be all grouped together under a projects folder. +Unit tests are co-located in a `__tests__` directory adjacent to the target source file they are testing. +``` +/frontend/src/**/__tests__ + /targetFile.spec.ts +``` + #### Testing Types ##### Unit Testing -Unit tests cover util functions and other non React based functions. These tests are stored in the `/unit `folder and can be organized into folders depending on their parent page and/or screen. Use Jest to test each function using `describe` to group together the utils file and the specific function. Then each test is described using `it`. Some functions are very basic and don't need a test. Use your best judgment if a test is needed. +Unit tests cover util functions and other non React based functions. Use Jest to test each function using `describe` to group together the utils file and the specific function. Then each test is described using `it`. _Example_ diff --git a/frontend/jest.config.js b/frontend/jest.config.js index 979a7d6349..b30cfd2aef 100644 --- a/frontend/jest.config.js +++ b/frontend/jest.config.js @@ -2,8 +2,11 @@ // https://jestjs.io/docs/en/configuration.html module.exports = { - roots: ['/src/__tests__/unit'], - testMatch: ['**/?(*.)+(spec|test).ts?(x)'], + roots: ['/src/'], + testMatch: [ + '**/src/__tests__/unit/**/?(*.)+(spec|test).ts?(x)', + '**/__tests__/?(*.)+(spec|test).ts?(x)', + ], // Automatically clear mock calls and instances between every test clearMocks: true, diff --git a/frontend/src/__tests__/dockerRepositoryURL.spec.ts b/frontend/src/__tests__/dockerRepositoryURL.spec.ts deleted file mode 100644 index 3f2b316211..0000000000 --- a/frontend/src/__tests__/dockerRepositoryURL.spec.ts +++ /dev/null @@ -1,49 +0,0 @@ -// https://cloud.google.com/artifact-registry/docs/docker/names -// The full name for a container image is one of the following formats: -// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE -// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE:TAG -// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE@IMAGE-DIGEST - -import { REPOSITORY_URL_REGEX } from '~/utilities/const'; - -test('Invalid URL', () => { - const url = 'docker.io'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe(''); -}); - -test('Docker container URL without tag', () => { - const url = 'docker.io/library/mysql'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('docker.io'); - expect(match?.[4]).toBe(undefined); -}); - -test('Docker container URL with tag', () => { - const url = 'docker.io/library/mysql:test-tag'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('docker.io'); - expect(match?.[4]).toBe('test-tag'); -}); - -test('OpenShift internal registry URL without tag', () => { - const url = 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); - expect(match?.[4]).toBe(undefined); -}); - -test('OpenShift internal registry URL with tag', () => { - const url = - 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook:v0.3.0-py36'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); - expect(match?.[4]).toBe('v0.3.0-py36'); -}); - -test('Quay URL with port and tag', () => { - const url = 'quay.io:443/opendatahub/odh-dashboard:main-55e19fa'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('quay.io:443'); - expect(match?.[4]).toBe('main-55e19fa'); -}); diff --git a/frontend/src/pages/TestPage.tsx.bak b/frontend/src/pages/TestPage.tsx.bak deleted file mode 100644 index ce068f1c1d..0000000000 --- a/frontend/src/pages/TestPage.tsx.bak +++ /dev/null @@ -1,55 +0,0 @@ -import { K8sModelCommon, useK8sWatchResource } from '@openshift/dynamic-plugin-sdk-utils'; -import { Alert, AlertVariant, Button } from '@patternfly/react-core'; -import * as React from 'react'; -import { ProjectKind } from '~/k8sTypes'; - -const errorMessage = (e: unknown): string => - (typeof e === 'object' ? e?.toString() : typeof e === 'string' ? e : '') || ''; - -const ProjectModel: K8sModelCommon = { - apiVersion: 'v1', - apiGroup: 'project.openshift.io', - kind: 'Project', - plural: 'projects', -}; -const TestPage = () => { - new Object(); - const [limit, setLimit] = React.useState(100); - const [projects, loaded, error] = useK8sWatchResource( - { - groupVersionKind: { - group: 'project.openshift.io', - version: 'v1', - kind: 'Project', - }, - limit, - isList: true, - }, - ProjectModel, - ); - return ( - <> -
- -
- {!loaded ?
Loading...
: null} - {error ? ( - - {errorMessage(error)} - - ) : null} -
    - {Array.isArray(projects) - ? projects.map((p) =>
  1. {p.metadata.name}
    2. ) - : null} -
- - ); -}; - -export default TestPage; diff --git a/frontend/src/__tests__/unit/useGroups.spec.ts b/frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts similarity index 97% rename from frontend/src/__tests__/unit/useGroups.spec.ts rename to frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts index 200531a10f..fcf404f1a4 100644 --- a/frontend/src/__tests__/unit/useGroups.spec.ts +++ b/frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts @@ -2,7 +2,7 @@ import { act } from '@testing-library/react'; import { k8sListResource } from '@openshift/dynamic-plugin-sdk-utils'; import useGroups from '~/pages/projects/projectSharing/useGroups'; import { GroupKind } from '~/k8sTypes'; -import { expectHook, standardUseFetchState, testHook } from './testUtils/hooks'; +import { expectHook, standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ k8sListResource: jest.fn(), diff --git a/frontend/src/utilities/__tests__/const.spec.ts b/frontend/src/utilities/__tests__/const.spec.ts new file mode 100644 index 0000000000..19c6bd0f9c --- /dev/null +++ b/frontend/src/utilities/__tests__/const.spec.ts @@ -0,0 +1,51 @@ +// https://cloud.google.com/artifact-registry/docs/docker/names +// The full name for a container image is one of the following formats: +// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE +// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE:TAG +// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE@IMAGE-DIGEST + +import { REPOSITORY_URL_REGEX } from '~/utilities/const'; + +describe('REPOSITORY_URL_REGEX', () => { + test('Invalid URL', () => { + const url = 'docker.io'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe(''); + }); + + test('Docker container URL without tag', () => { + const url = 'docker.io/library/mysql'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('docker.io'); + expect(match?.[4]).toBe(undefined); + }); + + test('Docker container URL with tag', () => { + const url = 'docker.io/library/mysql:test-tag'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('docker.io'); + expect(match?.[4]).toBe('test-tag'); + }); + + test('OpenShift internal registry URL without tag', () => { + const url = 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); + expect(match?.[4]).toBe(undefined); + }); + + test('OpenShift internal registry URL with tag', () => { + const url = + 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook:v0.3.0-py36'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); + expect(match?.[4]).toBe('v0.3.0-py36'); + }); + + test('Quay URL with port and tag', () => { + const url = 'quay.io:443/opendatahub/odh-dashboard:main-55e19fa'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('quay.io:443'); + expect(match?.[4]).toBe('main-55e19fa'); + }); +}); diff --git a/frontend/src/__tests__/unit/notebookControllerUtils.spec.ts b/frontend/src/utilities/__tests__/notebookControllerUtils.spec.ts similarity index 100% rename from frontend/src/__tests__/unit/notebookControllerUtils.spec.ts rename to frontend/src/utilities/__tests__/notebookControllerUtils.spec.ts diff --git a/frontend/src/__tests__/unit/useFetchState.spec.ts b/frontend/src/utilities/__tests__/useFetchState.spec.ts similarity index 96% rename from frontend/src/__tests__/unit/useFetchState.spec.ts rename to frontend/src/utilities/__tests__/useFetchState.spec.ts index 220e4bccf7..f29f380a21 100644 --- a/frontend/src/__tests__/unit/useFetchState.spec.ts +++ b/frontend/src/utilities/__tests__/useFetchState.spec.ts @@ -1,6 +1,6 @@ import { act } from '@testing-library/react'; import useFetchState from '~/utilities/useFetchState'; -import { expectHook, standardUseFetchState, testHook } from './testUtils/hooks'; +import { expectHook, standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; jest.useFakeTimers(); From 38b2f181661b0b94ee6d6c7718eeb2ace48473e3 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Mon, 11 Sep 2023 14:30:33 -0400 Subject: [PATCH 10/23] Fix model server expand issue on the project details page --- .../modelServing/ServingRuntimeList.spec.ts | 26 +++++++++++++++---- .../screens/projects/ServingRuntimeTable.tsx | 8 +++--- .../projects/ServingRuntimeTableRow.tsx | 14 +++++++--- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts index f49d3b8068..486bc8233c 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts @@ -45,17 +45,33 @@ test('Legacy Serving Runtime', async ({ page }) => { await page.waitForSelector('text=Add server'); // Check that the legacy serving runtime is shown with the default runtime name - expect(await page.getByText('ovms')).toBeTruthy(); + expect(page.getByText('ovms')).toBeTruthy(); // Check that the legacy serving runtime displays the correct Serving Runtime - expect(await page.getByText('OpenVINO Model Server')).toBeTruthy(); + expect(page.getByText('OpenVINO Model Server')).toBeTruthy(); // Check that the legacy serving runtime has tokens disabled - expect(await page.getByText('Tokens disabled')).toBeTruthy(); + expect(page.getByText('Tokens disabled')).toBeTruthy(); // Check that the serving runtime is shown with the default runtime name - expect(await page.getByText('OVMS Model Serving')).toBeTruthy(); + expect(page.getByText('OVMS Model Serving')).toBeTruthy(); // Check that the serving runtime displays the correct Serving Runtime - expect(await page.getByText('OpenVINO Serving Runtime (Supports GPUs)')).toBeTruthy(); + expect(page.getByText('OpenVINO Serving Runtime (Supports GPUs)')).toBeTruthy(); + + // Get the first and second row + const firstButton = page.getByRole('button', { name: 'ovms', exact: true }); + const secondButton = page.getByRole('button', { name: 'OVMS Model Serving', exact: true }); + const firstRow = page.getByRole('rowgroup').filter({ has: firstButton }); + const secondRow = page.getByRole('rowgroup').filter({ has: secondButton }); + + // Check that both of the rows are not expanded + await expect(firstRow).not.toHaveClass('pf-m-expanded'); + await expect(secondRow).not.toHaveClass('pf-m-expanded'); + + await firstButton.click(); + + // Check that the first row is expanded while the second is not + await expect(firstRow).toHaveClass('pf-m-expanded'); + await expect(secondRow).not.toHaveClass('pf-m-expanded'); }); diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx index 60a806871e..cae3343da8 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx @@ -2,7 +2,6 @@ import * as React from 'react'; import Table from '~/components/table/Table'; import { AccessReviewResourceAttributes, ServingRuntimeKind } from '~/k8sTypes'; import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; -import { ServingRuntimeTableTabs } from '~/pages/modelServing/screens/types'; import { useAccessReview } from '~/api'; import { columns } from './data'; import ServingRuntimeTableRow from './ServingRuntimeTableRow'; @@ -20,7 +19,7 @@ const ServingRuntimeTable: React.FC = () => { const [deployServingRuntime, setDeployServingRuntime] = React.useState(); const [deleteServingRuntime, setDeleteServingRuntime] = React.useState(); const [editServingRuntime, setEditServingRuntime] = React.useState(); - const [expandedColumn, setExpandedColumn] = React.useState(); + const [expandedServingRuntimeName, setExpandedServingRuntimeName] = React.useState(); const { servingRuntimes: { data: modelServers, refresh: refreshServingRuntime }, @@ -52,8 +51,7 @@ const ServingRuntimeTable: React.FC = () => { onDeleteServingRuntime={(obj) => setDeleteServingRuntime(obj)} onEditServingRuntime={(obj) => setEditServingRuntime(obj)} onDeployModel={(obj) => setDeployServingRuntime(obj)} - expandedColumn={expandedColumn} - setExpandedColumn={setExpandedColumn} + expandedServingRuntimeName={expandedServingRuntimeName} allowDelete={allowDelete} /> )} @@ -95,7 +93,7 @@ const ServingRuntimeTable: React.FC = () => { if (submit) { refreshInferenceServices(); refreshDataConnections(); - setExpandedColumn(ServingRuntimeTableTabs.DEPLOYED_MODELS); + setExpandedServingRuntimeName(deployServingRuntime.metadata.name); } }} projectContext={{ diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx index 32be3c29a0..fb94afcb0c 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx @@ -16,8 +16,7 @@ type ServingRuntimeTableRowProps = { onDeleteServingRuntime: (obj: ServingRuntimeKind) => void; onEditServingRuntime: (obj: ServingRuntimeKind) => void; onDeployModel: (obj: ServingRuntimeKind) => void; - expandedColumn?: ServingRuntimeTableTabs; - setExpandedColumn: (column?: ServingRuntimeTableTabs) => void; + expandedServingRuntimeName?: string; allowDelete: boolean; }; @@ -26,8 +25,7 @@ const ServingRuntimeTableRow: React.FC = ({ onDeleteServingRuntime, onEditServingRuntime, onDeployModel, - expandedColumn, - setExpandedColumn, + expandedServingRuntimeName, allowDelete, }) => { const { @@ -40,6 +38,14 @@ const ServingRuntimeTableRow: React.FC = ({ filterTokens, } = React.useContext(ProjectDetailsContext); + const [expandedColumn, setExpandedColumn] = React.useState(); + + React.useEffect(() => { + if (expandedServingRuntimeName === obj.metadata.name) { + setExpandedColumn(ServingRuntimeTableTabs.DEPLOYED_MODELS); + } + }, [expandedServingRuntimeName, obj.metadata.name]); + const tokens = filterTokens(obj.metadata.name); const modelInferenceServices = getInferenceServiceFromServingRuntime(inferenceServices, obj); From ab2614f6aa07f4feaecc5a87689e3d9c1c7f1ab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Ko=C5=82osowski?= Date: Sat, 2 Sep 2023 19:36:41 +0200 Subject: [PATCH 11/23] Fixed isCpuLimitEqual and isMemoryLimitEqual wrongly comparing null/undefined values. Also added some unit tests for good measure --- .../utilities/__tests__/valueUnits.spec.ts | 27 +++++++++++++++++++ frontend/src/utilities/valueUnits.ts | 8 ++++++ 2 files changed, 35 insertions(+) create mode 100644 frontend/src/utilities/__tests__/valueUnits.spec.ts diff --git a/frontend/src/utilities/__tests__/valueUnits.spec.ts b/frontend/src/utilities/__tests__/valueUnits.spec.ts new file mode 100644 index 0000000000..d16500bdd4 --- /dev/null +++ b/frontend/src/utilities/__tests__/valueUnits.spec.ts @@ -0,0 +1,27 @@ +import { isCpuLimitEqual, isMemoryLimitEqual } from '~/utilities/valueUnits'; + +describe('isCpuLimitEqual', () => { + test('correctly compares non-undefined values', () => { + expect(isCpuLimitEqual('1', '1')).toBe(true); + expect(isCpuLimitEqual('1000m', '1')).toBe(true); + expect(isCpuLimitEqual('1001m', '1')).toBe(false); + }); + test('correctly compares undefined values', () => { + expect(isCpuLimitEqual('1000m', undefined)).toBe(false); + expect(isCpuLimitEqual('1', undefined)).toBe(false); + expect(isCpuLimitEqual(undefined, undefined)).toBe(true); + }); +}); + +describe('isMemoryLimitEqual', () => { + test('correctly compares non-undefined values', () => { + expect(isMemoryLimitEqual('1Gi', '1Gi')).toBe(true); + expect(isMemoryLimitEqual('1Gi', '1024Mi')).toBe(true); + expect(isMemoryLimitEqual('1Gi', '1025Mi')).toBe(false); + }); + test('correctly compares undefined values', () => { + expect(isMemoryLimitEqual('1Gi', undefined)).toBe(false); + expect(isMemoryLimitEqual('1024Mi', undefined)).toBe(false); + expect(isMemoryLimitEqual(undefined, undefined)).toBe(true); + }); +}); diff --git a/frontend/src/utilities/valueUnits.ts b/frontend/src/utilities/valueUnits.ts index 3d6487bac7..55fd54c6ef 100644 --- a/frontend/src/utilities/valueUnits.ts +++ b/frontend/src/utilities/valueUnits.ts @@ -52,6 +52,10 @@ export const isEqual = ( ): boolean => calculateDelta(value1, value2, units) === 0; export const isCpuLimitEqual = (cpu1?: ValueUnitString, cpu2?: ValueUnitString): boolean => { + if (!cpu1 && !cpu2) { + return true; + } + if (!cpu1 || !cpu2) { return false; } @@ -63,6 +67,10 @@ export const isMemoryLimitEqual = ( memory1?: ValueUnitString, memory2?: ValueUnitString, ): boolean => { + if (!memory1 && !memory2) { + return true; + } + if (!memory1 || !memory2) { return false; } From d92d749f6478f0aa23aa196ca29165b73da95d23 Mon Sep 17 00:00:00 2001 From: Dipanshu Gupta Date: Wed, 30 Aug 2023 17:47:54 +0530 Subject: [PATCH 12/23] Divider disappears after create pipeline --- .../detail/ProjectDetailsComponents.tsx | 23 +------- .../data-connections/DataConnectionsList.tsx | 7 ++- .../screens/detail/notebooks/NotebookList.tsx | 54 ++++++++++--------- .../detail/pipelines/PipelinesList.tsx | 12 ++++- .../detail/pipelines/PipelinesSection.tsx | 50 +++++++++-------- .../screens/detail/storage/StorageList.tsx | 7 ++- 6 files changed, 80 insertions(+), 73 deletions(-) diff --git a/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx b/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx index 6f0126e7b8..819130f635 100644 --- a/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx +++ b/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx @@ -1,12 +1,10 @@ import * as React from 'react'; -import { Divider, PageSection, Stack, StackItem } from '@patternfly/react-core'; +import { PageSection, Stack, StackItem } from '@patternfly/react-core'; import GenericSidebar from '~/components/GenericSidebar'; import { useAppContext } from '~/app/AppContext'; import ServingRuntimeList from '~/pages/modelServing/screens/projects/ServingRuntimeList'; -import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; import { featureFlagEnabled } from '~/utilities/utils'; import PipelinesSection from '~/pages/projects/screens/detail/pipelines/PipelinesSection'; -import { usePipelinesAPI } from '~/concepts/pipelines/context'; import NotebooksList from './notebooks/NotebookList'; import { ProjectSectionID } from './types'; import StorageList from './storage/StorageList'; @@ -17,19 +15,10 @@ import useCheckLogoutParams from './useCheckLogoutParams'; type SectionType = { id: ProjectSectionID; component: React.ReactNode; - isEmpty: boolean; }; const ProjectDetailsComponents: React.FC = () => { const { dashboardConfig } = useAppContext(); - const { - notebooks: { data: notebookStates, loaded: notebookStatesLoaded }, - pvcs: { data: pvcs, loaded: pvcsLoaded }, - dataConnections: { data: connections, loaded: connectionsLoaded }, - servingRuntimes: { data: modelServers, loaded: modelServersLoaded }, - } = React.useContext(ProjectDetailsContext); - const { pipelinesServer } = usePipelinesAPI(); - const modelServingEnabled = featureFlagEnabled( dashboardConfig.spec.dashboardConfig.disableModelServing, ); @@ -42,24 +31,20 @@ const ProjectDetailsComponents: React.FC = () => { { id: ProjectSectionID.WORKBENCHES, component: , - isEmpty: notebookStatesLoaded && notebookStates.length === 0, }, { id: ProjectSectionID.CLUSTER_STORAGES, component: , - isEmpty: pvcsLoaded && pvcs.length === 0, }, { id: ProjectSectionID.DATA_CONNECTIONS, component: , - isEmpty: connectionsLoaded && connections.length === 0, }, ...(pipelinesEnabled ? [ { id: ProjectSectionID.PIPELINES, component: , - isEmpty: !pipelinesServer.installed, }, ] : []), @@ -68,7 +53,6 @@ const ProjectDetailsComponents: React.FC = () => { { id: ProjectSectionID.MODEL_SERVER, component: , - isEmpty: modelServersLoaded && modelServers.length === 0, }, ] : []), @@ -82,7 +66,7 @@ const ProjectDetailsComponents: React.FC = () => { maxWidth={175} > - {sections.map(({ id, component, isEmpty }, index) => ( + {sections.map(({ id, component }) => ( { > {component} - {index !== sections.length - 1 && isEmpty && ( - - )} ))} diff --git a/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx b/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx index 18d23c0fe7..92a686590e 100644 --- a/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx +++ b/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Button } from '@patternfly/react-core'; +import { Button, Divider } from '@patternfly/react-core'; import EmptyDetailsList from '~/pages/projects/screens/detail/EmptyDetailsList'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; import DetailsSection from '~/pages/projects/screens/detail/DetailsSection'; @@ -15,6 +15,8 @@ const DataConnectionsList: React.FC = () => { } = React.useContext(ProjectDetailsContext); const [open, setOpen] = React.useState(false); + const isDataConnectionsEmpty = connections.length === 0; + return ( <> { , ]} isLoading={!loaded} - isEmpty={connections.length === 0} + isEmpty={isDataConnectionsEmpty} loadError={error} emptyState={ { > + {isDataConnectionsEmpty && } { diff --git a/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx b/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx index 0396239c96..5759aea454 100644 --- a/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx +++ b/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Button } from '@patternfly/react-core'; +import { Button, Divider } from '@patternfly/react-core'; import { useNavigate } from 'react-router-dom'; import EmptyDetailsList from '~/pages/projects/screens/detail/EmptyDetailsList'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; @@ -17,6 +17,7 @@ const NotebookList: React.FC = () => { } = React.useContext(ProjectDetailsContext); const navigate = useNavigate(); const projectName = currentProject.metadata.name; + const isNotebooksEmpty = notebookStates.length === 0; React.useEffect(() => { let interval: ReturnType; @@ -27,30 +28,33 @@ const NotebookList: React.FC = () => { }, [notebookStates, refreshNotebooks]); return ( - navigate(`/projects/${projectName}/spawner`)} - variant="secondary" - > - Create workbench - , - ]} - isLoading={!loaded} - loadError={loadError} - isEmpty={notebookStates.length === 0} - emptyState={ - - } - > - - + <> + navigate(`/projects/${projectName}/spawner`)} + variant="secondary" + > + Create workbench + , + ]} + isLoading={!loaded} + loadError={loadError} + isEmpty={isNotebooksEmpty} + emptyState={ + + } + > + + + {isNotebooksEmpty && } + ); }; diff --git a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx index f038c0d882..4b41778b1e 100644 --- a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx +++ b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx @@ -9,11 +9,21 @@ import { usePipelinesAPI } from '~/concepts/pipelines/context'; import EmptyStateErrorMessage from '~/components/EmptyStateErrorMessage'; import { TABLE_CONTENT_LIMIT, LIMIT_MAX_ITEM_COUNT } from '~/concepts/pipelines/const'; -const PipelinesList: React.FC = () => { +type PipelinesListProps = { + setIsPipelinesEmpty: (isEmpty: boolean) => void; +}; + +const PipelinesList: React.FC = ({ setIsPipelinesEmpty }) => { const { namespace } = usePipelinesAPI(); const [pipelines, loaded, loadError, refresh] = usePipelines(LIMIT_MAX_ITEM_COUNT); const navigate = useNavigate(); + const isPipelinesEmpty = pipelines.length === 0; + + React.useEffect(() => { + setIsPipelinesEmpty(isPipelinesEmpty); + }, [isPipelinesEmpty, setIsPipelinesEmpty]); + if (loadError) { return ( diff --git a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx index ed36a91cd9..d8039dde92 100644 --- a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx +++ b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx @@ -1,4 +1,5 @@ import * as React from 'react'; +import { Divider } from '@patternfly/react-core'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; import { ProjectSectionTitles } from '~/pages/projects/screens/detail/const'; import DetailsSection from '~/pages/projects/screens/detail/DetailsSection'; @@ -13,29 +14,34 @@ const PipelinesSection: React.FC = () => { pipelinesServer: { initializing, installed, timedOut }, } = usePipelinesAPI(); + const [isPipelinesEmpty, setIsPipelinesEmpty] = React.useState(false); + return ( - , - ]} - isLoading={initializing} - isEmpty={!installed} - emptyState={} - > - {timedOut ? ( - - ) : ( - - - - )} - + <> + , + ]} + isLoading={initializing} + isEmpty={!installed} + emptyState={} + > + {timedOut ? ( + + ) : ( + + + + )} + + {(isPipelinesEmpty || !installed) && } + ); }; diff --git a/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx b/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx index 439f8f86b8..3ebdd5c4f8 100644 --- a/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx +++ b/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Button } from '@patternfly/react-core'; +import { Button, Divider } from '@patternfly/react-core'; import EmptyDetailsList from '~/pages/projects/screens/detail/EmptyDetailsList'; import DetailsSection from '~/pages/projects/screens/detail/DetailsSection'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; @@ -15,6 +15,8 @@ const StorageList: React.FC = () => { refreshAllProjectData: refresh, } = React.useContext(ProjectDetailsContext); + const isPvcsEmpty = pvcs.length === 0; + return ( <> { , ]} isLoading={!loaded} - isEmpty={pvcs.length === 0} + isEmpty={isPvcsEmpty} loadError={loadError} emptyState={ { > setOpen(true)} /> + {isPvcsEmpty && } { From 92113b81c4a7717913e7773383d4d325c40d355e Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Wed, 6 Sep 2023 11:51:25 -0400 Subject: [PATCH 13/23] Add shm to ServingRuntime volumes and volumeMounts --- backend/src/types.ts | 2 ++ frontend/src/api/k8s/notebooks.ts | 12 +--------- frontend/src/api/k8s/servingRuntimes.ts | 29 +++++++++++++++++++------ frontend/src/api/k8s/utils.ts | 12 ++++++++++ frontend/src/k8sTypes.ts | 3 +++ 5 files changed, 40 insertions(+), 18 deletions(-) diff --git a/backend/src/types.ts b/backend/src/types.ts index 73fef553b3..8dc3a0cac8 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -901,8 +901,10 @@ export type ServingRuntime = K8sResourceCommon & { image: string; name: string; resources: ContainerResources; + volumeMounts?: VolumeMount[]; }[]; supportedModelFormats: SupportedModelFormats[]; replicas: number; + volumes?: Volume[]; }; }; diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index 8c1ddb8d80..d7a86ba07e 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -26,17 +26,7 @@ import { } from '~/concepts/pipelines/elyra/utils'; import { createRoleBinding } from '~/api'; import { Volume, VolumeMount } from '~/types'; -import { assemblePodSpecOptions } from './utils'; - -const getshmVolumeMount = (): VolumeMount => ({ - name: 'shm', - mountPath: '/dev/shm', -}); - -const getshmVolume = (): Volume => ({ - name: 'shm', - emptyDir: { medium: 'Memory' }, -}); +import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; const assembleNotebook = ( data: StartNotebookData, diff --git a/frontend/src/api/k8s/servingRuntimes.ts b/frontend/src/api/k8s/servingRuntimes.ts index 3842828d48..a8b4699cac 100644 --- a/frontend/src/api/k8s/servingRuntimes.ts +++ b/frontend/src/api/k8s/servingRuntimes.ts @@ -14,7 +14,7 @@ import { getModelServingRuntimeName } from '~/pages/modelServing/utils'; import { getDisplayNameFromK8sResource, translateDisplayNameForK8s } from '~/pages/projects/utils'; import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; import { getModelServingProjects } from './projects'; -import { assemblePodSpecOptions } from './utils'; +import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; const assembleServingRuntime = ( data: CreatingServingRuntimeObject, @@ -79,12 +79,27 @@ const assembleServingRuntime = ( const { affinity, tolerations, resources } = assemblePodSpecOptions(resourceSettings, gpus); - updatedServingRuntime.spec.containers = servingRuntime.spec.containers.map((container) => ({ - ...container, - resources, - affinity, - tolerations, - })); + const volumes = updatedServingRuntime.spec.volumes || []; + if (!volumes.find((volume) => volume.name === 'shm')) { + volumes.push(getshmVolume('2Gi')); + } + + updatedServingRuntime.spec.volumes = volumes; + + updatedServingRuntime.spec.containers = servingRuntime.spec.containers.map((container) => { + const volumeMounts = container.volumeMounts || []; + if (!volumeMounts.find((volumeMount) => volumeMount.mountPath === '/dev/shm')) { + volumeMounts.push(getshmVolumeMount()); + } + + return { + ...container, + resources, + affinity, + tolerations, + volumeMounts, + }; + }); return updatedServingRuntime; }; diff --git a/frontend/src/api/k8s/utils.ts b/frontend/src/api/k8s/utils.ts index 920415d757..ce2867007c 100644 --- a/frontend/src/api/k8s/utils.ts +++ b/frontend/src/api/k8s/utils.ts @@ -4,6 +4,8 @@ import { PodToleration, TolerationSettings, ContainerResourceAttributes, + VolumeMount, + Volume, } from '~/types'; import { determineTolerations } from '~/utilities/tolerations'; @@ -54,3 +56,13 @@ export const assemblePodSpecOptions = ( const tolerations = determineTolerations(gpus > 0, tolerationSettings); return { affinity, tolerations, resources }; }; + +export const getshmVolumeMount = (): VolumeMount => ({ + name: 'shm', + mountPath: '/dev/shm', +}); + +export const getshmVolume = (sizeLimit?: string): Volume => ({ + name: 'shm', + emptyDir: { medium: 'Memory', ...(sizeLimit && { sizeLimit }) }, +}); diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index 3b4f16524e..2795ef2eed 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -12,6 +12,7 @@ import { TolerationSettings, ImageStreamStatusTagItem, ImageStreamStatusTagCondition, + VolumeMount, } from './types'; import { ServingRuntimeSize } from './pages/modelServing/screens/types'; @@ -327,9 +328,11 @@ export type ServingRuntimeKind = K8sResourceCommon & { image: string; name: string; resources: ContainerResources; + volumeMounts?: VolumeMount[]; }[]; supportedModelFormats: SupportedModelFormats[]; replicas: number; + volumes?: Volume[]; }; }; From 27d87fed3ab8b1d48e756a0a1f2dc7aea57539ca Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Mon, 11 Sep 2023 16:30:11 -0400 Subject: [PATCH 14/23] better error handling when parsing s3 endpoint --- frontend/jest.config.js | 2 +- .../__tests__/utils.spec.ts | 130 ++++++++++++++++++ .../content/configurePipelinesServer/utils.ts | 25 ++-- 3 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts diff --git a/frontend/jest.config.js b/frontend/jest.config.js index b30cfd2aef..68a37f4e48 100644 --- a/frontend/jest.config.js +++ b/frontend/jest.config.js @@ -26,7 +26,7 @@ module.exports = { testEnvironment: 'jest-environment-jsdom', // include projects from node_modules as required - transformIgnorePatterns: ['node_modules/(?!yaml)'], + transformIgnorePatterns: ['node_modules/(?!yaml|@openshift|lodash-es|uuid)'], // A list of paths to snapshot serializer modules Jest should use for snapshot testing snapshotSerializers: [], diff --git a/frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts b/frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts new file mode 100644 index 0000000000..20fe6b4266 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts @@ -0,0 +1,130 @@ +import { AWS_KEYS } from '~/pages/projects/dataConnections/const'; +import { PipelineServerConfigType } from '~/concepts/pipelines/content/configurePipelinesServer/types'; +import { createDSPipelineResourceSpec } from '~/concepts/pipelines/content/configurePipelinesServer/utils'; + +describe('configure pipeline server utils', () => { + describe('createDSPipelineResourceSpec', () => { + const createPipelineServerConfig = () => + ({ + database: { + useDefault: true, + value: [], + }, + objectStorage: { + useExisting: true, + existingName: '', + existingValue: [], + }, + } as PipelineServerConfigType); + + type SecretsResponse = Parameters[1]; + + const createSecretsResponse = ( + databaseSecret?: SecretsResponse[0], + objectStorageSecret?: SecretsResponse[1], + ): SecretsResponse => [databaseSecret, objectStorageSecret ?? { secretName: '', awsData: [] }]; + + it('should create resource spec', () => { + const spec = createDSPipelineResourceSpec( + createPipelineServerConfig(), + createSecretsResponse(), + ); + expect(spec).toEqual({ + database: undefined, + objectStorage: { + externalStorage: { + bucket: '', + host: '', + s3CredentialsSecret: { + accessKey: 'AWS_ACCESS_KEY_ID', + secretKey: 'AWS_SECRET_ACCESS_KEY', + secretName: '', + }, + scheme: 'https', + }, + }, + }); + }); + + it('should parse S3 endpoint with scheme', () => { + const secretsResponse = createSecretsResponse(); + secretsResponse[1].awsData = [ + { key: AWS_KEYS.S3_ENDPOINT, value: 'http://s3.amazonaws.com' }, + ]; + const spec = createDSPipelineResourceSpec(createPipelineServerConfig(), secretsResponse); + expect(spec.objectStorage.externalStorage?.scheme).toBe('http'); + expect(spec.objectStorage.externalStorage?.host).toBe('s3.amazonaws.com'); + }); + + it('should parse S3 endpoint without scheme', () => { + const secretsResponse = createSecretsResponse(); + + secretsResponse[1].awsData = [{ key: AWS_KEYS.S3_ENDPOINT, value: 's3.amazonaws.com' }]; + const spec = createDSPipelineResourceSpec(createPipelineServerConfig(), secretsResponse); + expect(spec.objectStorage.externalStorage?.scheme).toBe('https'); + expect(spec.objectStorage.externalStorage?.host).toBe('s3.amazonaws.com'); + }); + + it('should include bucket', () => { + const secretsResponse = createSecretsResponse(); + secretsResponse[1].awsData = [{ key: AWS_KEYS.AWS_S3_BUCKET, value: 'my-bucket' }]; + const spec = createDSPipelineResourceSpec(createPipelineServerConfig(), secretsResponse); + expect(spec.objectStorage.externalStorage?.bucket).toBe('my-bucket'); + }); + + it('should create spec with database object', () => { + const config = createPipelineServerConfig(); + config.database.value = [ + { + key: 'Username', + value: 'test-user', + }, + { + key: 'Port', + value: '8080', + }, + { + key: 'Host', + value: 'test.host.com', + }, + { + key: 'Database', + value: 'db-name', + }, + ]; + const spec = createDSPipelineResourceSpec( + config, + createSecretsResponse({ + key: 'password-key', + name: 'password-name', + }), + ); + expect(spec).toEqual({ + objectStorage: { + externalStorage: { + bucket: '', + host: '', + s3CredentialsSecret: { + accessKey: 'AWS_ACCESS_KEY_ID', + secretKey: 'AWS_SECRET_ACCESS_KEY', + secretName: '', + }, + scheme: 'https', + }, + }, + database: { + externalDB: { + host: 'test.host.com', + passwordSecret: { + key: 'password-key', + name: 'password-name', + }, + pipelineDBName: 'db-name', + port: '8080', + username: 'test-user', + }, + }, + }); + }); + }); +}); diff --git a/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts b/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts index 06c5b00164..d4cb2f6553 100644 --- a/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts +++ b/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts @@ -116,22 +116,22 @@ const createSecrets = (config: PipelineServerConfigType, projectName: string) => .catch(reject); }); -export const configureDSPipelineResourceSpec = ( +export const createDSPipelineResourceSpec = ( config: PipelineServerConfigType, - projectName: string, -): Promise => - createSecrets(config, projectName).then(([databaseSecret, objectStorageSecret]) => { + [databaseSecret, objectStorageSecret]: SecretsResponse, +): DSPipelineKind['spec'] => { + { const awsRecord = dataEntryToRecord(objectStorageSecret.awsData); const databaseRecord = dataEntryToRecord(config.database.value); const [, externalStorageScheme, externalStorageHost] = - awsRecord.AWS_S3_ENDPOINT?.match(/^(\w+):\/\/(.*)/) ?? []; + awsRecord.AWS_S3_ENDPOINT?.match(/^(?:(\w+):\/\/)?(.*)/) ?? []; return { objectStorage: { externalStorage: { - host: externalStorageHost.replace(/\/$/, ''), - scheme: externalStorageScheme, + host: externalStorageHost?.replace(/\/$/, '') || '', + scheme: externalStorageScheme || 'https', bucket: awsRecord.AWS_S3_BUCKET || '', s3CredentialsSecret: { accessKey: AWS_KEYS.ACCESS_KEY_ID, @@ -155,4 +155,13 @@ export const configureDSPipelineResourceSpec = ( } : undefined, }; - }); + } +}; + +export const configureDSPipelineResourceSpec = ( + config: PipelineServerConfigType, + projectName: string, +): Promise => + createSecrets(config, projectName).then((secretsResponse) => + createDSPipelineResourceSpec(config, secretsResponse), + ); From 05868024413f0ab15bf3849b885fa79bd3ed9fdc Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Fri, 15 Sep 2023 11:40:29 -0400 Subject: [PATCH 15/23] Added Edge to SME areas --- docs/smes.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/smes.md b/docs/smes.md index 614dc6d8d3..da59e636cb 100644 --- a/docs/smes.md +++ b/docs/smes.md @@ -9,7 +9,7 @@ Below there will be some terms like “previous” and “backup”, these are f - **backup** – a good person to lean on if there is a need for any 2nd opinions, for bouncing ideas off of, or any larger discussion about direction - **and** – Ping both during conversations – could be onboarding, could be a need to share information, best get both people involved at the same time -General Dashboard ownership +## General Dashboard ownership - Infrastructure / direction - Architect: `Andrew` ([andrewballantyne]) - General UX: `Kyle` ([kywalker-rh]) @@ -19,7 +19,7 @@ General Dashboard ownership - Performance - Area lead: `Lucas` ([lucferbux]) -Dashboard features +## Dashboard feature areas - Data Science Projects - Feature lead: `Andrew` ([andrewballantyne]) - UX: `Kyle` ([kywalker-rh]) **and** `Kun` ([xianli123]) @@ -40,13 +40,16 @@ Dashboard features - Feature lead: `Juntao` ([DaoDaoNoCode]) - Backup: `Andrew` ([andrewballantyne]) - UX: `Vince` ([vconzola]) -- Habana / Accelerators - - Feature lead: `Gage` ([Gkrumbach07]); +- Accelerators (Habana, GPU, etc) + - Feature lead: `Gage` ([Gkrumbach07]) - Backup: `Andrew` ([andrewballantyne]) - UX: `Yan` ([yannnz]) - Model Registry - Feature lead: TBD - UX: `Sim` ([simrandhaliw]) **and** `Haley` ([yih-wang]) +- Edge + - Feature lead: TBD + - UX: `Vince` ([vconzola]) [andrewballantyne]: https://github.com/andrewballantyne From 9f8ff52435df54c8631dcb316495631109c72217 Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Fri, 15 Sep 2023 16:25:39 -0400 Subject: [PATCH 16/23] Add UX and Tracker internal templates --- .github/ISSUE_TEMPLATE/internal_tracker.yml | 64 +++++++++++++++++++++ .github/ISSUE_TEMPLATE/internal_ux.yml | 53 +++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/internal_tracker.yml create mode 100644 .github/ISSUE_TEMPLATE/internal_ux.yml diff --git a/.github/ISSUE_TEMPLATE/internal_tracker.yml b/.github/ISSUE_TEMPLATE/internal_tracker.yml new file mode 100644 index 0000000000..4345f6215c --- /dev/null +++ b/.github/ISSUE_TEMPLATE/internal_tracker.yml @@ -0,0 +1,64 @@ +name: (Internal) Tracker Template +description: Intended to help with a template for tracking larger grouped items. +title: "[Tracker]: " +labels: ["tracker"] +body: + - type: textarea + id: description + attributes: + label: Description + description: A introductory description of the larger task + validations: + required: + true + - type: input + id: branch + attributes: + label: Target Branch + description: What is the feature branch to contain this effort? If not known at this time, replace with `TBD` + placeholder: f/ + validations: + required: true + - type: textarea + id: requirements + attributes: + label: Requirements + description: A series of requirements to consider this tracker complete. + placeholder: | + * P0: Show something + * P2: Allow users to change permissions + validations: + required: true + - type: textarea + id: ux-issues + attributes: + label: Itemized UX Issues + description: | + List the tickets that UX will work on. + + Tip: Using a bullet list will help display links to other tickets by unraveling the name and status of that ticket. + placeholder: | + * #1234 + * Design mocks - Ticket TBD + validations: + required: true + - type: textarea + id: dev-issues + attributes: + label: Itemized Dev Issues + description: | + List the tickets that Development will work on. If unknown at this time, add `TBD` + + Tip: Using a bullet list will help display links to other tickets by unraveling the name and status of that ticket. + placeholder: | + * #1234 + * Implement Table Page - Ticket TBD + validations: + required: true + - type: textarea + id: artifacts + attributes: + label: Related artifacts + description: Any additional artifacts that will help with the tracker goals + validations: + required: false diff --git a/.github/ISSUE_TEMPLATE/internal_ux.yml b/.github/ISSUE_TEMPLATE/internal_ux.yml new file mode 100644 index 0000000000..8cf69b0b96 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/internal_ux.yml @@ -0,0 +1,53 @@ +name: (Internal) UX Template +description: Intended to help ux create internal flows. +title: "[UX]: " +labels: ["kind/ux"] +body: + - type: textarea + id: description + attributes: + label: Description + description: A introductory description of the task + validations: + required: + true + - type: textarea + id: goals + attributes: + label: Goals + description: An itemized list of goals to complete for this ticket + placeholder: | + * Research... + * Design... + validations: + required: false + - type: textarea + id: output + attributes: + label: Expected Output + description: What would be considered the end result? + validations: + required: false + - type: textarea + id: related-issues + attributes: + label: Related Issues + description: | + Any related issues that might be useful to mention as it relates to this ticket's goals, expectations, or follow ups. + + Tip: Using a bullet list will help display links to other tickets by unraveling the name and status of that ticket. + placeholder: | + * #1234 + * Create figma designs - Ticket TBD + validations: + required: false + - type: textarea + id: artifacts + attributes: + label: Completed artifacts + description: | + Any artifacts you want to easily note as results of the effort. + + Typically this is left empty at the start. Also useful to include useful links or information you would like to share for additional context. + validations: + required: false From c79dd541a8452b46af5902db8a82b516d10dfb6f Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Mon, 18 Sep 2023 11:19:14 -0400 Subject: [PATCH 17/23] update scroll container selector for JumpLinks --- frontend/src/app/App.tsx | 4 ++-- frontend/src/components/GenericSidebar.tsx | 4 ++-- frontend/src/utilities/const.ts | 6 +++++- frontend/src/utilities/utils.ts | 4 ++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/frontend/src/app/App.tsx b/frontend/src/app/App.tsx index c42d128ec2..2d8e49dfdf 100644 --- a/frontend/src/app/App.tsx +++ b/frontend/src/app/App.tsx @@ -15,7 +15,7 @@ import ErrorBoundary from '~/components/error/ErrorBoundary'; import ToastNotifications from '~/components/ToastNotifications'; import { useWatchBuildStatus } from '~/utilities/useWatchBuildStatus'; import { useUser } from '~/redux/selectors'; -import { DASHBOARD_MAIN_CONTAINER_SELECTOR } from '~/utilities/const'; +import { DASHBOARD_MAIN_CONTAINER_ID } from '~/utilities/const'; import useDetectUser from '~/utilities/useDetectUser'; import ProjectsContextProvider from '~/concepts/projects/ProjectsContext'; import Header from './Header'; @@ -96,7 +96,7 @@ const App: React.FC = () => { sidebar={isAllowed ? : undefined} notificationDrawer={ setNotificationsOpen(false)} />} isNotificationDrawerExpanded={notificationsOpen} - mainContainerId={DASHBOARD_MAIN_CONTAINER_SELECTOR} + mainContainerId={DASHBOARD_MAIN_CONTAINER_ID} > diff --git a/frontend/src/components/GenericSidebar.tsx b/frontend/src/components/GenericSidebar.tsx index 2a4a268bde..b6ab8ca090 100644 --- a/frontend/src/components/GenericSidebar.tsx +++ b/frontend/src/components/GenericSidebar.tsx @@ -6,7 +6,7 @@ import { SidebarContent, SidebarPanel, } from '@patternfly/react-core'; -import { DASHBOARD_MAIN_CONTAINER_SELECTOR } from '~/utilities/const'; +import { DASHBOARD_SCROLL_CONTAINER_SELECTOR } from '~/utilities/const'; type GenericSidebarProps = { sections: string[]; @@ -20,7 +20,7 @@ const GenericSidebar: React.FC = ({ children, sections, titles, - scrollableSelector = `#${DASHBOARD_MAIN_CONTAINER_SELECTOR}`, + scrollableSelector = DASHBOARD_SCROLL_CONTAINER_SELECTOR, maxWidth, }) => ( diff --git a/frontend/src/utilities/const.ts b/frontend/src/utilities/const.ts index 8379731c07..1589247229 100644 --- a/frontend/src/utilities/const.ts +++ b/frontend/src/utilities/const.ts @@ -46,4 +46,8 @@ export const DEFAULT_CONTEXT_DATA: ContextResourceData = { export const REPOSITORY_URL_REGEX = /^([\w.\-_]+((?::\d+|)(?=\/[a-z0-9._-]+\/[a-z0-9._-]+))|)(?:\/|)([a-z0-9.\-_]+(?:\/[a-z0-9.\-_]+|))(?::([\w.\-_]{1,127})|)/; -export const DASHBOARD_MAIN_CONTAINER_SELECTOR = 'dashboard-page-main'; +export const DASHBOARD_MAIN_CONTAINER_ID = 'dashboard-page-main'; + +// Quick starts drawer creates a new scroll container within its DrawerContentBody. +// Not an ideal selector but components such as JumpLinks require the use of a selector instead of a direct node reference. +export const DASHBOARD_SCROLL_CONTAINER_SELECTOR = `#${DASHBOARD_MAIN_CONTAINER_ID} > .pf-c-drawer > .pf-c-drawer__main > .pf-c-drawer__content`; diff --git a/frontend/src/utilities/utils.ts b/frontend/src/utilities/utils.ts index 5a81adceee..eff87fc7bd 100644 --- a/frontend/src/utilities/utils.ts +++ b/frontend/src/utilities/utils.ts @@ -1,5 +1,5 @@ import { OdhApplication, OdhDocument, OdhDocumentType } from '~/types'; -import { CATEGORY_ANNOTATION, DASHBOARD_MAIN_CONTAINER_SELECTOR, ODH_PRODUCT_NAME } from './const'; +import { CATEGORY_ANNOTATION, DASHBOARD_MAIN_CONTAINER_ID, ODH_PRODUCT_NAME } from './const'; /** * Feature flags are required in the config -- but upgrades can be mixed and omission of the property @@ -144,7 +144,7 @@ export const isGroupEmpty = (groupList: Array groupList.filter((element) => element.enabled).length === 0; export const getDashboardMainContainer = (): HTMLElement => - document.getElementById(DASHBOARD_MAIN_CONTAINER_SELECTOR) || document.body; + document.getElementById(DASHBOARD_MAIN_CONTAINER_ID) || document.body; export const isHTMLInputElement = (object: unknown): object is HTMLInputElement => (object as HTMLInputElement).value !== undefined; From eb83a857f9dd8900ff09393a38048eb973be5923 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Mon, 18 Sep 2023 15:23:54 -0400 Subject: [PATCH 18/23] Add duplicate option for non-OOTB custom serving runtimes --- .../CustomServingRuntimeTableRow.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx b/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx index 7ee5b7dd71..135a66870b 100644 --- a/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx +++ b/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeTableRow.tsx @@ -62,6 +62,16 @@ const CustomServingRuntimeTableRow: React.FC onClick: () => navigate(`/servingRuntimes/editServingRuntime/${servingRuntimeName}`), }, + { + title: 'Duplicate', + onClick: () => + navigate('/servingRuntimes/addServingRuntime', { + state: { template: template }, + }), + }, + { + isSeparator: true, + }, { title: 'Delete', onClick: () => onDeleteTemplate(template), From 3351661263cf213cc4416fdaf512b80050fcf8aa Mon Sep 17 00:00:00 2001 From: Lucas Fernandez Date: Tue, 19 Sep 2023 16:47:17 +0200 Subject: [PATCH 19/23] Fix issue displaying error in Inference Service --- .../mockInferenceServiceK8sResource.ts | 22 ++++++++++ .../modelServing/ModelServingGlobal.spec.ts | 43 +++++++++++++++++++ .../ModelServingGlobal.stories.tsx | 14 +++++- .../ManageInferenceServiceModal.tsx | 1 + 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/frontend/src/__mocks__/mockInferenceServiceK8sResource.ts b/frontend/src/__mocks__/mockInferenceServiceK8sResource.ts index f47a9d78c5..723b5dc3e3 100644 --- a/frontend/src/__mocks__/mockInferenceServiceK8sResource.ts +++ b/frontend/src/__mocks__/mockInferenceServiceK8sResource.ts @@ -9,6 +9,28 @@ type MockResourceConfigType = { secretName?: string; }; +export const mockInferenceServicek8sError = () => ({ + kind: 'Status', + apiVersion: 'v1', + metadata: {}, + status: 'Failure', + message: + 'InferenceService.serving.kserve.io "trigger-error" is invalid: [metadata.name: Invalid value: "trigger-error": is invalid, metadata.labels: Invalid value: "trigger-error": must have proper format]', + reason: 'Invalid', + details: { + name: 'trigger-error', + group: 'serving.kserve.io', + kind: 'InferenceService', + causes: [ + { + reason: 'FieldValueInvalid', + message: 'Invalid value: "trigger-error": must have proper format', + field: 'metadata.name', + }, + ], + }, +}); + export const mockInferenceServiceK8sResource = ({ name = 'test-inference-service', namespace = 'test-project', diff --git a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts index 9b21197786..dee2ce86ae 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts @@ -92,3 +92,46 @@ test('Create model', async ({ page }) => { await page.getByLabel('Path').fill('test-model/'); await expect(await page.getByRole('button', { name: 'Deploy' })).toBeEnabled(); }); + +test('Create model error', async ({ page }) => { + await page.goto( + './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--deploy-model&viewMode=story', + ); + + // wait for page to load + await page.waitForSelector('text=Deploy model'); + + // test that you can not submit on empty + await expect(await page.getByRole('button', { name: 'Deploy' })).toBeDisabled(); + + // test filling in minimum required fields + await page.locator('#existing-project-selection').click(); + await page.getByRole('option', { name: 'Test Project' }).click(); + await page.getByLabel('Model Name *').fill('trigger-error'); + await page.locator('#inference-service-model-selection').click(); + await page.getByRole('option', { name: 'ovms' }).click(); + await expect(page.getByText('Model framework (name - version)')).toBeTruthy(); + await page.locator('#inference-service-framework-selection').click(); + await page.getByRole('option', { name: 'onnx - 1' }).click(); + await expect(await page.getByRole('button', { name: 'Deploy' })).toBeDisabled(); + await page + .getByRole('group', { name: 'Model location' }) + .getByRole('button', { name: 'Options menu' }) + .click(); + await page.getByRole('option', { name: 'Test Secret' }).click(); + await page.getByLabel('Path').fill('test-model/'); + await expect(await page.getByRole('button', { name: 'Deploy' })).toBeEnabled(); + await page.getByLabel('Path').fill('test-model/'); + await expect(await page.getByRole('button', { name: 'Deploy' })).toBeEnabled(); + + // Submit and check the invalid error message + await page.getByRole('button', { name: 'Deploy' }).click(); + await page.waitForSelector('text=Error creating model server'); + + // Close the modal + await page.getByRole('button', { name: 'Cancel' }).click(); + + // Check that the error message is gone + await page.getByRole('button', { name: 'Deploy model' }).click(); + expect(await page.isVisible('text=Error creating model server')).toBeFalsy(); +}); diff --git a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx index 5861976db9..488b034124 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx +++ b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx @@ -8,7 +8,10 @@ import { Route, Routes } from 'react-router-dom'; import { mockK8sResourceList } from '~/__mocks__/mockK8sResourceList'; import { mockProjectK8sResource } from '~/__mocks__/mockProjectK8sResource'; import { mockServingRuntimeK8sResource } from '~/__mocks__/mockServingRuntimeK8sResource'; -import { mockInferenceServiceK8sResource } from '~/__mocks__/mockInferenceServiceK8sResource'; +import { + mockInferenceServiceK8sResource, + mockInferenceServicek8sError, +} from '~/__mocks__/mockInferenceServiceK8sResource'; import { mockSecretK8sResource } from '~/__mocks__/mockSecretK8sResource'; import ModelServingContextProvider from '~/pages/modelServing/ModelServingContext'; import ModelServingGlobal from '~/pages/modelServing/screens/global/ModelServingGlobal'; @@ -38,6 +41,15 @@ export default { rest.get('/api/k8s/apis/project.openshift.io/v1/projects', (req, res, ctx) => res(ctx.json(mockK8sResourceList([mockProjectK8sResource({})]))), ), + rest.post( + 'api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices/test', + (req, res, ctx) => res(ctx.json(mockInferenceServiceK8sResource({}))), + ), + rest.post( + 'api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices/trigger-error', + (req, res, ctx) => + res(ctx.status(422, 'Unprocessable Entity'), ctx.json(mockInferenceServicek8sError())), + ), ], }, }, diff --git a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx index 79e5da78fd..11973e4de3 100644 --- a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx +++ b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx @@ -78,6 +78,7 @@ const ManageInferenceServiceModal: React.FC = const onBeforeClose = (submitted: boolean) => { onClose(submitted); + setError(undefined); setActionInProgress(false); resetData(); }; From e8b7723c2b9d27ca987307c4f1229fe180f59554 Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Tue, 19 Sep 2023 15:32:27 -0400 Subject: [PATCH 20/23] Improve logging with objects --- backend/.eslintrc | 4 ++++ backend/src/plugins/kube.ts | 5 ++++- .../api/cluster-settings/clusterSettingsUtils.ts | 10 ++++------ backend/src/routes/api/dev-impersonate/index.ts | 2 ++ .../routes/api/groups-config/groupsConfigUtil.ts | 4 ++-- backend/src/routes/api/images/imageUtils.ts | 8 ++++---- .../routes/api/segment-key/segmentKeyUtils.ts | 2 +- backend/src/server.ts | 16 ++++++++++++++-- backend/src/utils/adminUtils.ts | 6 +++--- backend/src/utils/prometheusUtils.ts | 2 +- 10 files changed, 39 insertions(+), 20 deletions(-) diff --git a/backend/.eslintrc b/backend/.eslintrc index a0d326e003..c28a9daf75 100755 --- a/backend/.eslintrc +++ b/backend/.eslintrc @@ -35,6 +35,10 @@ "settings": { }, "rules": { + "no-restricted-properties": [ "error", { + "property": "toString", + "message": "e.toString() should be fastify.log.error(e, 'your string'). Other use-cases should avoid obj.toString() on principle. Craft the string you want instead." + }], "@typescript-eslint/explicit-function-return-type": "off", "@typescript-eslint/interface-name-prefix": "off", "@typescript-eslint/no-var-requires": "off", diff --git a/backend/src/plugins/kube.ts b/backend/src/plugins/kube.ts index ee1d78c87f..ed1c43eaa6 100644 --- a/backend/src/plugins/kube.ts +++ b/backend/src/plugins/kube.ts @@ -46,7 +46,10 @@ export default fp(async (fastify: FastifyInstance) => { ); clusterID = (clusterVersion.body as { spec: { clusterID: string } }).spec.clusterID; } catch (e) { - fastify.log.error(`Failed to retrieve cluster id: ${e.response?.body?.message || e.message}.`); + fastify.log.error( + e, + `Failed to retrieve cluster id: ${e.response?.body?.message || e.message}.`, + ); } let clusterBranding = 'okd'; try { diff --git a/backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts b/backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts index 57692c5ca0..4fab3ccd15 100644 --- a/backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts +++ b/backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts @@ -111,9 +111,7 @@ export const updateClusterSettings = async ( } return { success: true, error: null }; } catch (e) { - fastify.log.error( - 'Setting cluster settings error: ' + e.toString() + e.response?.body?.message, - ); + fastify.log.error(e, 'Setting cluster settings error: ' + e.response?.body?.message); if (e.response?.statusCode !== 404) { return { success: false, error: 'Unable to update cluster settings. ' + e.message }; } @@ -137,7 +135,7 @@ export const getClusterSettings = async ( clusterSettings.userTrackingEnabled = segmentEnabledRes.body.data.segmentKeyEnabled === 'true'; } catch (e) { - fastify.log.error('Error retrieving segment key enabled: ' + e.toString()); + fastify.log.error(e, 'Error retrieving segment key enabled.'); } } if (isJupyterEnabled) { @@ -165,7 +163,7 @@ export const getClusterSettings = async ( if (e.statusCode === 404) { fastify.log.warn('Notebook controller culling config not found, culling disabled...'); } else { - fastify.log.error('Error getting notebook controller culling settings: ' + e.toString()); + fastify.log.error(e, 'Error getting notebook controller culling settings.'); throw e; } }); @@ -175,7 +173,7 @@ export const getClusterSettings = async ( clusterSettings.pvcSize = pvcSize; clusterSettings.cullerTimeout = cullerTimeout; } catch (e) { - fastify.log.error('Error retrieving cluster settings: ' + e.toString()); + fastify.log.error(e, 'Error retrieving cluster settings.'); } } diff --git a/backend/src/routes/api/dev-impersonate/index.ts b/backend/src/routes/api/dev-impersonate/index.ts index d5f4a2e9f8..778add7672 100644 --- a/backend/src/routes/api/dev-impersonate/index.ts +++ b/backend/src/routes/api/dev-impersonate/index.ts @@ -23,6 +23,8 @@ export default async (fastify: KubeFastifyInstance): Promise => { url, { headers: { + // This usage of toString is fine for internal dev flows + // eslint-disable-next-line no-restricted-properties Authorization: `Basic ${Buffer.from( `${DEV_IMPERSONATE_USER}:${DEV_IMPERSONATE_PASSWORD}`, ).toString('base64')}`, diff --git a/backend/src/routes/api/groups-config/groupsConfigUtil.ts b/backend/src/routes/api/groups-config/groupsConfigUtil.ts index f5b082caae..488993efb8 100644 --- a/backend/src/routes/api/groups-config/groupsConfigUtil.ts +++ b/backend/src/routes/api/groups-config/groupsConfigUtil.ts @@ -25,7 +25,7 @@ export const getGroupsConfig = async (fastify: KubeFastifyInstance): Promise 0) { - fastify.log.error('Duplicate name unable to add notebook image'); + fastify.log.error('Duplicate name unable to add notebook image.'); return { success: false, error: 'Unable to add notebook image: ' + body.name }; } @@ -397,7 +397,7 @@ export const updateImage = async ( return { success: true, error: null }; } catch (e) { if (e.response?.statusCode !== 404) { - fastify.log.error('Unable to update notebook image: ' + e.toString()); + fastify.log.error(e, 'Unable to update notebook image.'); return { success: false, error: 'Unable to update notebook image: ' + e.message }; } } diff --git a/backend/src/routes/api/segment-key/segmentKeyUtils.ts b/backend/src/routes/api/segment-key/segmentKeyUtils.ts index b4c02729a9..8289a4c5c4 100644 --- a/backend/src/routes/api/segment-key/segmentKeyUtils.ts +++ b/backend/src/routes/api/segment-key/segmentKeyUtils.ts @@ -11,7 +11,7 @@ export const getSegmentKey = async (fastify: KubeFastifyInstance): Promise { +app.listen({ port: PORT, host: IP }, (err) => { if (err) { app.log.error(err); process.exit(1); // eslint-disable-line diff --git a/backend/src/utils/adminUtils.ts b/backend/src/utils/adminUtils.ts index da2a96d6d8..66daf92dc5 100644 --- a/backend/src/utils/adminUtils.ts +++ b/backend/src/utils/adminUtils.ts @@ -52,7 +52,7 @@ export const getGroupsConfig = async ( return await checkUserInGroups(fastify, customObjectApi, adminGroupsList, username); } } catch (e) { - fastify.log.error(e.toString()); + fastify.log.error(e, 'Error getting groups config'); return false; } }; @@ -84,7 +84,7 @@ export const isUserAllowed = async ( ); } } catch (e) { - fastify.log.error(e.toString()); + fastify.log.error(e, 'Error determining isUserAllowed.'); return false; } }; @@ -142,7 +142,7 @@ const checkUserInGroups = async ( return true; } } catch (e) { - fastify.log.error(e.toString()); + fastify.log.error(e, 'Error checking if user is in group.'); } } return false; diff --git a/backend/src/utils/prometheusUtils.ts b/backend/src/utils/prometheusUtils.ts index 141f3025de..bee83cb6b6 100644 --- a/backend/src/utils/prometheusUtils.ts +++ b/backend/src/utils/prometheusUtils.ts @@ -41,7 +41,7 @@ const callPrometheus = async ( fastify.log.info('Successful response from Prometheus.'); return { code: 200, response: parsedData }; } catch (e) { - const errorMessage = e.message || e.toString(); + const errorMessage = e.message || 'Unknown reason.'; fastify.log.error(`Failure parsing the response from Prometheus. ${errorMessage}`); if (errorMessage.includes('Unexpected token < in JSON')) { throw { code: 422, response: 'Unprocessable prometheus response' }; From 2b3949d7b8bb73f17c9f265f986ea0c1fbadaf2f Mon Sep 17 00:00:00 2001 From: Lucas Fernandez Date: Mon, 18 Sep 2023 15:59:10 +0200 Subject: [PATCH 21/23] Revamp empty state in Model Serving Global --- docs/architecture.md | 8 +--- .../integration/hooks/useFetchState.spec.ts | 17 +++---- .../clusterSettings/ClusterSettings.spec.ts | 5 +-- .../modelServing/ModelServingGlobal.spec.ts | 42 ++++++++++++----- .../ModelServingGlobal.stories.tsx | 45 +++++++++++++++++++ .../modelServing/ServingRuntimeList.spec.ts | 7 ++- .../pages/projects/ProjectDetails.spec.ts | 9 ++-- .../pages/projects/ProjectView.spec.ts | 13 ++---- frontend/src/__tests__/integration/utils.ts | 2 + .../screens/global/EmptyModelServing.tsx | 26 +++++++---- frontend/tsconfig.json | 3 +- 11 files changed, 116 insertions(+), 61 deletions(-) create mode 100644 frontend/src/__tests__/integration/utils.ts diff --git a/docs/architecture.md b/docs/architecture.md index 564a3af313..7e10452388 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -196,9 +196,7 @@ export const EditModel = { import { test, expect } from '@playwright/test'; test('Create project', async ({ page }) => { - await page.goto( - './iframe.html?id=tests-stories-pages-projects-projectview--create-project&viewMode=story', - ); + await page.goto(navigateToStory('projects-projectview', 'create-project')); // wait for page to load await page.waitForSelector('text=Create data science project'); @@ -217,9 +215,7 @@ test('Create project', async ({ page }) => { To run storybook UI: `cd ./frontend && npm run storybook` ```ts -await page.goto( - './iframe.html?id=tests-stories-pages-projects-projectview--create-project&viewMode=story', -); +await page.goto(navigateToStory('projects-projectview', 'create-project')); ``` 6. Wait for the page to load and the story to settle before performing any assertions or actions. Use `page.waitForSelector()` to wait for a specific element to appear as an indication of the story being loaded. diff --git a/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts b/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts index 940f18088e..eae447c826 100644 --- a/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts +++ b/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts @@ -1,9 +1,8 @@ import { test, expect } from '@playwright/test'; +import { navigateToStory } from '~/__tests__/integration/utils'; test('Success', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-hooks-usefetchstate--success&viewMode=story', - ); + await page.goto(navigateToStory('hooks-usefetchstate', 'success')); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -16,9 +15,7 @@ test('Success', async ({ page }) => { }); test('Failure', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-hooks-usefetchstate--failure&viewMode=story', - ); + await page.goto(navigateToStory('hooks-usefetchstate', 'failure')); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -34,9 +31,7 @@ test('Failure', async ({ page }) => { }); test('Stable', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-hooks-usefetchstate--stable&viewMode=story', - ); + await page.goto(navigateToStory('hooks-usefetchstate', 'stable')); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -76,9 +71,7 @@ test('Stable', async ({ page }) => { }); test('Refresh rate', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-hooks-usefetchstate--refresh-rate&viewMode=story', - ); + await page.goto(navigateToStory('hooks-usefetchstate', 'refresh-rate')); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); diff --git a/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts b/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts index 0a68a8faca..b4b861499a 100644 --- a/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts +++ b/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts @@ -1,9 +1,8 @@ import { test, expect } from '@playwright/test'; +import { navigateToStory } from '~/__tests__/integration/utils'; test('Cluster settings', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-pages-clustersettings-clustersettings--default&viewMode=story', - ); + await page.goto(navigateToStory('pages-clustersettings-clustersettings', 'default')); // wait for page to load await page.waitForSelector('text=Save changes'); const submitButton = page.locator('[data-id="submit-cluster-settings"]'); diff --git a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts index dee2ce86ae..7a8aa986e6 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts @@ -1,10 +1,36 @@ import { test, expect } from '@playwright/test'; +import { navigateToStory } from '~/__tests__/integration/utils'; -test('Delete model', async ({ page }) => { +test('Empty State No Serving Runtime', async ({ page }) => { + await page.goto( + navigateToStory('pages-modelserving-modelservingglobal', 'empty-state-no-serving-runtime'), + ); + + // wait for page to load + await page.waitForSelector('text=No deployed models yet'); + + // Test that the button is enabled + await expect(page.getByRole('button', { name: 'Go to the Projects page' })).toBeTruthy(); +}); + +test('Empty State No Inference Service', async ({ page }) => { await page.goto( - './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--delete-model&viewMode=story', + navigateToStory('pages-modelserving-modelservingglobal', 'empty-state-no-inference-service'), ); + // wait for page to load + await page.waitForSelector('text=No deployed models'); + + // Test that the button is enabled + await page.getByRole('button', { name: 'Deploy model' }).click(); + + // test that you can not submit on empty + await expect(await page.getByRole('button', { name: 'Deploy' })).toBeDisabled(); +}); + +test('Delete model', async ({ page }) => { + await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'delete-model')); + // wait for page to load await page.waitForSelector('text=Delete deployed model?'); @@ -19,9 +45,7 @@ test('Delete model', async ({ page }) => { }); test('Edit model', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--edit-model&viewMode=story', - ); + await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'edit-model')); // wait for page to load await page.waitForSelector('text=Deploy model'); @@ -53,9 +77,7 @@ test('Edit model', async ({ page }) => { }); test('Create model', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--deploy-model&viewMode=story', - ); + await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'deploy-model')); // wait for page to load await page.waitForSelector('text=Deploy model'); @@ -94,9 +116,7 @@ test('Create model', async ({ page }) => { }); test('Create model error', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--deploy-model&viewMode=story', - ); + await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'deploy-model')); // wait for page to load await page.waitForSelector('text=Deploy model'); diff --git a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx index 488b034124..348d608ea5 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx +++ b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx @@ -63,6 +63,51 @@ const Template: StoryFn = (args) => ( ); +export const EmptyStateNoServingRuntime: StoryObj = { + render: Template, + + parameters: { + msw: { + handlers: [ + rest.get( + 'api/k8s/apis/serving.kserve.io/v1alpha1/namespaces/test-project/servingruntimes', + (req, res, ctx) => res(ctx.json(mockK8sResourceList([]))), + ), + rest.get( + 'api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices', + (req, res, ctx) => res(ctx.json(mockK8sResourceList([]))), + ), + rest.get('/api/k8s/apis/project.openshift.io/v1/projects', (req, res, ctx) => + res(ctx.json(mockK8sResourceList([mockProjectK8sResource({})]))), + ), + ], + }, + }, +}; + +export const EmptyStateNoInferenceServices: StoryObj = { + render: Template, + + parameters: { + msw: { + handlers: [ + rest.get( + 'api/k8s/apis/serving.kserve.io/v1alpha1/namespaces/test-project/servingruntimes', + (req, res, ctx) => + res(ctx.json(mockK8sResourceList([mockServingRuntimeK8sResource({})]))), + ), + rest.get( + 'api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices', + (req, res, ctx) => res(ctx.json(mockK8sResourceList([]))), + ), + rest.get('/api/k8s/apis/project.openshift.io/v1/projects', (req, res, ctx) => + res(ctx.json(mockK8sResourceList([mockProjectK8sResource({})]))), + ), + ], + }, + }, +}; + export const EditModel: StoryObj = { render: Template, diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts index 486bc8233c..585f794d0d 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts @@ -1,9 +1,8 @@ import { test, expect } from '@playwright/test'; +import { navigateToStory } from '~/__tests__/integration/utils'; test('Deploy model', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-pages-modelserving-servingruntimelist--deploy-model&viewMode=story', - ); + await page.goto(navigateToStory('pages-modelserving-servingruntimelist', 'deploy-model')); // wait for page to load await page.waitForSelector('text=Deploy model'); @@ -38,7 +37,7 @@ test('Deploy model', async ({ page }) => { test('Legacy Serving Runtime', async ({ page }) => { await page.goto( - './iframe.html?args=&id=tests-integration-pages-modelserving-servingruntimelist--list-available-models&viewMode=story', + navigateToStory('pages-modelserving-servingruntimelist', 'list-available-models'), ); // wait for page to load diff --git a/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts b/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts index bb17017d8e..a89ca6bf86 100644 --- a/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts +++ b/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts @@ -1,9 +1,8 @@ import { test, expect } from '@playwright/test'; +import { navigateToStory } from '~/__tests__/integration/utils'; test('Empty project', async ({ page }) => { - await page.goto( - './iframe.html?args=&id=tests-integration-pages-projects-projectdetails--empty-details-page&viewMode=story', - ); + await page.goto(navigateToStory('pages-projects-projectdetails', 'empty-details-page')); // wait for page to load await page.waitForSelector('text=No model servers'); @@ -16,9 +15,7 @@ test('Empty project', async ({ page }) => { }); test('Non-empty project', async ({ page }) => { - await page.goto( - './iframe.html?id=tests-integration-pages-projects-projectdetails--default&viewMode=story', - ); + await page.goto(navigateToStory('pages-projects-projectdetails', 'default')); // wait for page to load await page.waitForSelector('text=Test Notebook'); diff --git a/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts b/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts index 7f4800427b..3ded1a4dd2 100644 --- a/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts +++ b/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts @@ -1,9 +1,8 @@ import { test, expect } from '@playwright/test'; +import { navigateToStory } from '~/__tests__/integration/utils'; test('Create project', async ({ page }) => { - await page.goto( - './iframe.html?id=tests-integration-pages-projects-projectview--create-project&viewMode=story', - ); + await page.goto(navigateToStory('pages-projects-projectview', 'create-project')); // wait for page to load await page.waitForSelector('text=Create data science project'); @@ -52,9 +51,7 @@ test('Create project', async ({ page }) => { }); test('Edit project', async ({ page }) => { - await page.goto( - './iframe.html?id=tests-integration-pages-projects-projectview--edit-project&viewMode=story', - ); + await page.goto(navigateToStory('pages-projects-projectview', 'edit-project')); // wait for page to load await page.waitForSelector('text=Edit data science project'); @@ -71,9 +68,7 @@ test('Edit project', async ({ page }) => { }); test('Delete project', async ({ page }) => { - await page.goto( - './iframe.html?id=tests-integration-pages-projects-projectview--delete-project&viewMode=story', - ); + await page.goto(navigateToStory('pages-projects-projectview', 'delete-project')); // wait for page to load await page.waitForSelector('text=Delete project?'); diff --git a/frontend/src/__tests__/integration/utils.ts b/frontend/src/__tests__/integration/utils.ts new file mode 100644 index 0000000000..6c7a2a8f22 --- /dev/null +++ b/frontend/src/__tests__/integration/utils.ts @@ -0,0 +1,2 @@ +export const navigateToStory = (folder: string, storyId: string) => + `./iframe.html?args=&id=tests-integration-${folder}--${storyId}&viewMode=story`; diff --git a/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx b/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx index 4bd7471bd9..afe0a54481 100644 --- a/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx +++ b/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx @@ -1,6 +1,13 @@ import * as React from 'react'; -import { Button, EmptyState, EmptyStateBody, EmptyStateIcon, Title } from '@patternfly/react-core'; -import { PlusCircleIcon } from '@patternfly/react-icons'; +import { + Button, + EmptyState, + EmptyStateBody, + EmptyStateIcon, + EmptyStateVariant, + Title, +} from '@patternfly/react-core'; +import { PlusCircleIcon, WrenchIcon } from '@patternfly/react-icons'; import { useNavigate } from 'react-router-dom'; import { ModelServingContext } from '~/pages/modelServing/ModelServingContext'; import ServeModelButton from './ServeModelButton'; @@ -13,16 +20,17 @@ const EmptyModelServing: React.FC = () => { if (servingRuntimes.length === 0) { return ( - - + + - No model servers + No deployed models yet - Before deploying a model, you must first configure a model server. + To get started, deploy a model from the Models and model servers section + of a project. - ); @@ -32,7 +40,7 @@ const EmptyModelServing: React.FC = () => { - No deployed models. + No deployed models To get started, use existing model servers to serve a model. diff --git a/frontend/tsconfig.json b/frontend/tsconfig.json index 32c295a529..e584ca32d3 100644 --- a/frontend/tsconfig.json +++ b/frontend/tsconfig.json @@ -16,8 +16,9 @@ "esModuleInterop": true, "allowSyntheticDefaultImports": true, "strict": true, + "baseUrl": "./src", "paths": { - "~/*": ["./src/*"] + "~/*": ["./*"] }, "importHelpers": true, "skipLibCheck": true From 21ed929f8a52089adae441233db918afe75aa7e5 Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Wed, 20 Sep 2023 12:35:20 -0400 Subject: [PATCH 22/23] Revert "Revamp empty state in Model Serving Global" This reverts commit 2b3949d7b8bb73f17c9f265f986ea0c1fbadaf2f. --- docs/architecture.md | 8 +++- .../integration/hooks/useFetchState.spec.ts | 17 ++++--- .../clusterSettings/ClusterSettings.spec.ts | 5 ++- .../modelServing/ModelServingGlobal.spec.ts | 42 +++++------------ .../ModelServingGlobal.stories.tsx | 45 ------------------- .../modelServing/ServingRuntimeList.spec.ts | 7 +-- .../pages/projects/ProjectDetails.spec.ts | 9 ++-- .../pages/projects/ProjectView.spec.ts | 13 ++++-- frontend/src/__tests__/integration/utils.ts | 2 - .../screens/global/EmptyModelServing.tsx | 26 ++++------- frontend/tsconfig.json | 3 +- 11 files changed, 61 insertions(+), 116 deletions(-) delete mode 100644 frontend/src/__tests__/integration/utils.ts diff --git a/docs/architecture.md b/docs/architecture.md index 7e10452388..564a3af313 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -196,7 +196,9 @@ export const EditModel = { import { test, expect } from '@playwright/test'; test('Create project', async ({ page }) => { - await page.goto(navigateToStory('projects-projectview', 'create-project')); + await page.goto( + './iframe.html?id=tests-stories-pages-projects-projectview--create-project&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Create data science project'); @@ -215,7 +217,9 @@ test('Create project', async ({ page }) => { To run storybook UI: `cd ./frontend && npm run storybook` ```ts -await page.goto(navigateToStory('projects-projectview', 'create-project')); +await page.goto( + './iframe.html?id=tests-stories-pages-projects-projectview--create-project&viewMode=story', +); ``` 6. Wait for the page to load and the story to settle before performing any assertions or actions. Use `page.waitForSelector()` to wait for a specific element to appear as an indication of the story being loaded. diff --git a/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts b/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts index eae447c826..940f18088e 100644 --- a/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts +++ b/frontend/src/__tests__/integration/hooks/useFetchState.spec.ts @@ -1,8 +1,9 @@ import { test, expect } from '@playwright/test'; -import { navigateToStory } from '~/__tests__/integration/utils'; test('Success', async ({ page }) => { - await page.goto(navigateToStory('hooks-usefetchstate', 'success')); + await page.goto( + './iframe.html?args=&id=tests-integration-hooks-usefetchstate--success&viewMode=story', + ); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -15,7 +16,9 @@ test('Success', async ({ page }) => { }); test('Failure', async ({ page }) => { - await page.goto(navigateToStory('hooks-usefetchstate', 'failure')); + await page.goto( + './iframe.html?args=&id=tests-integration-hooks-usefetchstate--failure&viewMode=story', + ); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -31,7 +34,9 @@ test('Failure', async ({ page }) => { }); test('Stable', async ({ page }) => { - await page.goto(navigateToStory('hooks-usefetchstate', 'stable')); + await page.goto( + './iframe.html?args=&id=tests-integration-hooks-usefetchstate--stable&viewMode=story', + ); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -71,7 +76,9 @@ test('Stable', async ({ page }) => { }); test('Refresh rate', async ({ page }) => { - await page.goto(navigateToStory('hooks-usefetchstate', 'refresh-rate')); + await page.goto( + './iframe.html?args=&id=tests-integration-hooks-usefetchstate--refresh-rate&viewMode=story', + ); // wait 2 seconds to settle await new Promise((resolve) => setTimeout(resolve, 2000)); diff --git a/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts b/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts index b4b861499a..0a68a8faca 100644 --- a/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts +++ b/frontend/src/__tests__/integration/pages/clusterSettings/ClusterSettings.spec.ts @@ -1,8 +1,9 @@ import { test, expect } from '@playwright/test'; -import { navigateToStory } from '~/__tests__/integration/utils'; test('Cluster settings', async ({ page }) => { - await page.goto(navigateToStory('pages-clustersettings-clustersettings', 'default')); + await page.goto( + './iframe.html?args=&id=tests-integration-pages-clustersettings-clustersettings--default&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Save changes'); const submitButton = page.locator('[data-id="submit-cluster-settings"]'); diff --git a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts index 7a8aa986e6..dee2ce86ae 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.spec.ts @@ -1,36 +1,10 @@ import { test, expect } from '@playwright/test'; -import { navigateToStory } from '~/__tests__/integration/utils'; -test('Empty State No Serving Runtime', async ({ page }) => { - await page.goto( - navigateToStory('pages-modelserving-modelservingglobal', 'empty-state-no-serving-runtime'), - ); - - // wait for page to load - await page.waitForSelector('text=No deployed models yet'); - - // Test that the button is enabled - await expect(page.getByRole('button', { name: 'Go to the Projects page' })).toBeTruthy(); -}); - -test('Empty State No Inference Service', async ({ page }) => { +test('Delete model', async ({ page }) => { await page.goto( - navigateToStory('pages-modelserving-modelservingglobal', 'empty-state-no-inference-service'), + './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--delete-model&viewMode=story', ); - // wait for page to load - await page.waitForSelector('text=No deployed models'); - - // Test that the button is enabled - await page.getByRole('button', { name: 'Deploy model' }).click(); - - // test that you can not submit on empty - await expect(await page.getByRole('button', { name: 'Deploy' })).toBeDisabled(); -}); - -test('Delete model', async ({ page }) => { - await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'delete-model')); - // wait for page to load await page.waitForSelector('text=Delete deployed model?'); @@ -45,7 +19,9 @@ test('Delete model', async ({ page }) => { }); test('Edit model', async ({ page }) => { - await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'edit-model')); + await page.goto( + './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--edit-model&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Deploy model'); @@ -77,7 +53,9 @@ test('Edit model', async ({ page }) => { }); test('Create model', async ({ page }) => { - await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'deploy-model')); + await page.goto( + './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--deploy-model&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Deploy model'); @@ -116,7 +94,9 @@ test('Create model', async ({ page }) => { }); test('Create model error', async ({ page }) => { - await page.goto(navigateToStory('pages-modelserving-modelservingglobal', 'deploy-model')); + await page.goto( + './iframe.html?args=&id=tests-integration-pages-modelserving-modelservingglobal--deploy-model&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Deploy model'); diff --git a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx index 348d608ea5..488b034124 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx +++ b/frontend/src/__tests__/integration/pages/modelServing/ModelServingGlobal.stories.tsx @@ -63,51 +63,6 @@ const Template: StoryFn = (args) => ( ); -export const EmptyStateNoServingRuntime: StoryObj = { - render: Template, - - parameters: { - msw: { - handlers: [ - rest.get( - 'api/k8s/apis/serving.kserve.io/v1alpha1/namespaces/test-project/servingruntimes', - (req, res, ctx) => res(ctx.json(mockK8sResourceList([]))), - ), - rest.get( - 'api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices', - (req, res, ctx) => res(ctx.json(mockK8sResourceList([]))), - ), - rest.get('/api/k8s/apis/project.openshift.io/v1/projects', (req, res, ctx) => - res(ctx.json(mockK8sResourceList([mockProjectK8sResource({})]))), - ), - ], - }, - }, -}; - -export const EmptyStateNoInferenceServices: StoryObj = { - render: Template, - - parameters: { - msw: { - handlers: [ - rest.get( - 'api/k8s/apis/serving.kserve.io/v1alpha1/namespaces/test-project/servingruntimes', - (req, res, ctx) => - res(ctx.json(mockK8sResourceList([mockServingRuntimeK8sResource({})]))), - ), - rest.get( - 'api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices', - (req, res, ctx) => res(ctx.json(mockK8sResourceList([]))), - ), - rest.get('/api/k8s/apis/project.openshift.io/v1/projects', (req, res, ctx) => - res(ctx.json(mockK8sResourceList([mockProjectK8sResource({})]))), - ), - ], - }, - }, -}; - export const EditModel: StoryObj = { render: Template, diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts index 585f794d0d..486bc8233c 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts @@ -1,8 +1,9 @@ import { test, expect } from '@playwright/test'; -import { navigateToStory } from '~/__tests__/integration/utils'; test('Deploy model', async ({ page }) => { - await page.goto(navigateToStory('pages-modelserving-servingruntimelist', 'deploy-model')); + await page.goto( + './iframe.html?args=&id=tests-integration-pages-modelserving-servingruntimelist--deploy-model&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Deploy model'); @@ -37,7 +38,7 @@ test('Deploy model', async ({ page }) => { test('Legacy Serving Runtime', async ({ page }) => { await page.goto( - navigateToStory('pages-modelserving-servingruntimelist', 'list-available-models'), + './iframe.html?args=&id=tests-integration-pages-modelserving-servingruntimelist--list-available-models&viewMode=story', ); // wait for page to load diff --git a/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts b/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts index a89ca6bf86..bb17017d8e 100644 --- a/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts +++ b/frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts @@ -1,8 +1,9 @@ import { test, expect } from '@playwright/test'; -import { navigateToStory } from '~/__tests__/integration/utils'; test('Empty project', async ({ page }) => { - await page.goto(navigateToStory('pages-projects-projectdetails', 'empty-details-page')); + await page.goto( + './iframe.html?args=&id=tests-integration-pages-projects-projectdetails--empty-details-page&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=No model servers'); @@ -15,7 +16,9 @@ test('Empty project', async ({ page }) => { }); test('Non-empty project', async ({ page }) => { - await page.goto(navigateToStory('pages-projects-projectdetails', 'default')); + await page.goto( + './iframe.html?id=tests-integration-pages-projects-projectdetails--default&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Test Notebook'); diff --git a/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts b/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts index 3ded1a4dd2..7f4800427b 100644 --- a/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts +++ b/frontend/src/__tests__/integration/pages/projects/ProjectView.spec.ts @@ -1,8 +1,9 @@ import { test, expect } from '@playwright/test'; -import { navigateToStory } from '~/__tests__/integration/utils'; test('Create project', async ({ page }) => { - await page.goto(navigateToStory('pages-projects-projectview', 'create-project')); + await page.goto( + './iframe.html?id=tests-integration-pages-projects-projectview--create-project&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Create data science project'); @@ -51,7 +52,9 @@ test('Create project', async ({ page }) => { }); test('Edit project', async ({ page }) => { - await page.goto(navigateToStory('pages-projects-projectview', 'edit-project')); + await page.goto( + './iframe.html?id=tests-integration-pages-projects-projectview--edit-project&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Edit data science project'); @@ -68,7 +71,9 @@ test('Edit project', async ({ page }) => { }); test('Delete project', async ({ page }) => { - await page.goto(navigateToStory('pages-projects-projectview', 'delete-project')); + await page.goto( + './iframe.html?id=tests-integration-pages-projects-projectview--delete-project&viewMode=story', + ); // wait for page to load await page.waitForSelector('text=Delete project?'); diff --git a/frontend/src/__tests__/integration/utils.ts b/frontend/src/__tests__/integration/utils.ts deleted file mode 100644 index 6c7a2a8f22..0000000000 --- a/frontend/src/__tests__/integration/utils.ts +++ /dev/null @@ -1,2 +0,0 @@ -export const navigateToStory = (folder: string, storyId: string) => - `./iframe.html?args=&id=tests-integration-${folder}--${storyId}&viewMode=story`; diff --git a/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx b/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx index afe0a54481..4bd7471bd9 100644 --- a/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx +++ b/frontend/src/pages/modelServing/screens/global/EmptyModelServing.tsx @@ -1,13 +1,6 @@ import * as React from 'react'; -import { - Button, - EmptyState, - EmptyStateBody, - EmptyStateIcon, - EmptyStateVariant, - Title, -} from '@patternfly/react-core'; -import { PlusCircleIcon, WrenchIcon } from '@patternfly/react-icons'; +import { Button, EmptyState, EmptyStateBody, EmptyStateIcon, Title } from '@patternfly/react-core'; +import { PlusCircleIcon } from '@patternfly/react-icons'; import { useNavigate } from 'react-router-dom'; import { ModelServingContext } from '~/pages/modelServing/ModelServingContext'; import ServeModelButton from './ServeModelButton'; @@ -20,17 +13,16 @@ const EmptyModelServing: React.FC = () => { if (servingRuntimes.length === 0) { return ( - - + + - No deployed models yet + No model servers - To get started, deploy a model from the Models and model servers section - of a project. + Before deploying a model, you must first configure a model server. - ); @@ -40,7 +32,7 @@ const EmptyModelServing: React.FC = () => { - No deployed models + No deployed models. To get started, use existing model servers to serve a model. diff --git a/frontend/tsconfig.json b/frontend/tsconfig.json index e584ca32d3..32c295a529 100644 --- a/frontend/tsconfig.json +++ b/frontend/tsconfig.json @@ -16,9 +16,8 @@ "esModuleInterop": true, "allowSyntheticDefaultImports": true, "strict": true, - "baseUrl": "./src", "paths": { - "~/*": ["./*"] + "~/*": ["./src/*"] }, "importHelpers": true, "skipLibCheck": true From 947207e9a1de7aeded72981e8bb071e51976ab5b Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Tue, 19 Sep 2023 16:09:41 -0400 Subject: [PATCH 23/23] Tech Debt Template --- .github/ISSUE_TEMPLATE/internal_techdebt.yml | 54 ++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/internal_techdebt.yml diff --git a/.github/ISSUE_TEMPLATE/internal_techdebt.yml b/.github/ISSUE_TEMPLATE/internal_techdebt.yml new file mode 100644 index 0000000000..a38291e37f --- /dev/null +++ b/.github/ISSUE_TEMPLATE/internal_techdebt.yml @@ -0,0 +1,54 @@ +name: (Internal) Tech Debt Template +description: Intended to help create internal tech debt change. +title: "[Tech Debt]: " +labels: ["kind/tech-debt"] +body: + - type: dropdown + id: deploy-type + attributes: + label: Type + description: What kind of tech debt is it? + multiple: true + options: + - General Tech Debt (eg. Improve the way something exists today) + - Dev Efficiency (eg. CI improvements, issue templates, etc) + - Infrastructure (eg. Add web sockets) + - Quality (eg. tests) + validations: + required: true + - type: input + id: source + attributes: + label: Source + description: Where did you find this issue? + placeholder: main / feature branch x / incubation / etc + validations: + required: false + - type: textarea + id: description + attributes: + label: Description + description: What should we improve? + validations: + required: + true + - type: textarea + id: why + attributes: + label: Why? + description: What value does this bring / what is it fixing? + validations: + required: + true + - type: textarea + id: anything-else + attributes: + label: Anything else? + description: | + Additional information. Suggested topics: + - Initial investigation + - Known impact this will cause + - Anything else you want to add + validations: + required: + false