From 1614e1bc2c444fcb6e94db737fb59957d11762a1 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Fri, 1 Sep 2023 10:11:04 -0400 Subject: [PATCH 1/9] use generic object state on data connections and prevent resetting on edit --- .../projects/screens/spawner/SpawnerPage.tsx | 10 ++--- .../dataConnection/DataConnectionField.tsx | 45 +++++++++---------- .../useNotebookDataConnection.ts | 43 +++++++++--------- 3 files changed, 46 insertions(+), 52 deletions(-) diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx index cc12d854f3..d9f119f964 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx @@ -65,9 +65,9 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { const [storageDataWithoutDefault, setStorageData] = useStorageDataObject(existingNotebook); const storageData = useMergeDefaultPVCName(storageDataWithoutDefault, nameDesc.name); const [envVariables, setEnvVariables] = useNotebookEnvVariables(existingNotebook); - const [dataConnection, setDataConnection] = useNotebookDataConnection( - existingNotebook, + const [dataConnectionData, setDataConnectionData] = useNotebookDataConnection( dataConnections.data, + existingNotebook, ); const restartNotebooks = useWillNotebooksRestart([existingNotebook?.metadata.name || '']); @@ -205,8 +205,8 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { aria-label={SpawnerPageSectionTitles[SpawnerPageSectionID.DATA_CONNECTIONS]} > setDataConnection(connection)} + dataConnectionData={dataConnectionData} + setDataConnectionData={setDataConnectionData} /> @@ -235,7 +235,7 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { }} storageData={storageData} envVariables={envVariables} - dataConnection={dataConnection} + dataConnection={dataConnectionData} canEnablePipelines={canEnablePipelines} /> )} diff --git a/frontend/src/pages/projects/screens/spawner/dataConnection/DataConnectionField.tsx b/frontend/src/pages/projects/screens/spawner/dataConnection/DataConnectionField.tsx index 605559046a..478547912d 100644 --- a/frontend/src/pages/projects/screens/spawner/dataConnection/DataConnectionField.tsx +++ b/frontend/src/pages/projects/screens/spawner/dataConnection/DataConnectionField.tsx @@ -4,18 +4,19 @@ import { DataConnectionData, EnvironmentVariableType, SecretCategory, + UpdateObjectAtPropAndValue, } from '~/pages/projects/types'; import AWSField from '~/pages/projects/dataConnections/AWSField'; import { EMPTY_AWS_SECRET_DATA } from '~/pages/projects/dataConnections/const'; import ExistingDataConnectionField from './ExistingDataConnectionField'; type DataConnectionFieldProps = { - dataConnection: DataConnectionData; - setDataConnection: (dataConnection: DataConnectionData) => void; + dataConnectionData: DataConnectionData; + setDataConnectionData: UpdateObjectAtPropAndValue; }; const DataConnectionField: React.FC = ({ - dataConnection, - setDataConnection, + dataConnectionData, + setDataConnectionData, }) => ( = ({ name="enable-data-connection-checkbox" id="enable-data-connection-checkbox" label="Use a data connection" - isChecked={dataConnection.enabled} - onChange={() => setDataConnection({ ...dataConnection, enabled: !dataConnection.enabled })} + isChecked={dataConnectionData.enabled} + onChange={() => setDataConnectionData('enabled', !dataConnectionData.enabled)} body={ - dataConnection.enabled && ( + dataConnectionData.enabled && ( = ({ name="new-data-connection-radio" id="new-data-connection-radio" label="Create new data connection" - isChecked={dataConnection.type === 'creating'} - onChange={() => setDataConnection({ ...dataConnection, type: 'creating' })} + isChecked={dataConnectionData.type === 'creating'} + onChange={() => setDataConnectionData('type', 'creating')} body={ - dataConnection.type === 'creating' && ( + dataConnectionData.type === 'creating' && ( { - setDataConnection({ - ...dataConnection, - creating: { - type: EnvironmentVariableType.SECRET, - values: { category: SecretCategory.AWS, data: newEnvData }, - }, + setDataConnectionData('creating', { + type: EnvironmentVariableType.SECRET, + values: { category: SecretCategory.AWS, data: newEnvData }, }); }} /> @@ -60,18 +58,15 @@ const DataConnectionField: React.FC = ({ name="existing-data-connection-type-radio" id="existing-data-connection-type-radio" label="Use existing data connection" - isChecked={dataConnection.type === 'existing'} - onChange={() => setDataConnection({ ...dataConnection, type: 'existing' })} + isChecked={dataConnectionData.type === 'existing'} + onChange={() => setDataConnectionData('type', 'existing')} body={ - dataConnection.type === 'existing' && ( + dataConnectionData.type === 'existing' && ( - setDataConnection({ - ...dataConnection, - existing: { secretRef: { name: name ?? '' } }, - }) + setDataConnectionData('existing', { secretRef: { name: name ?? '' } }) } /> ) diff --git a/frontend/src/pages/projects/screens/spawner/dataConnection/useNotebookDataConnection.ts b/frontend/src/pages/projects/screens/spawner/dataConnection/useNotebookDataConnection.ts index 031fd2282b..84fa325645 100644 --- a/frontend/src/pages/projects/screens/spawner/dataConnection/useNotebookDataConnection.ts +++ b/frontend/src/pages/projects/screens/spawner/dataConnection/useNotebookDataConnection.ts @@ -1,6 +1,10 @@ -import * as React from 'react'; import { NotebookKind } from '~/k8sTypes'; -import { DataConnection, DataConnectionData } from '~/pages/projects/types'; +import { + DataConnection, + DataConnectionData, + UpdateObjectAtPropAndValue, +} from '~/pages/projects/types'; +import useGenericObjectState from '~/utilities/useGenericObjectState'; export const getNotebookDataConnection = ( notebook?: NotebookKind, @@ -15,25 +19,18 @@ export const getNotebookDataConnection = ( }; export const useNotebookDataConnection = ( + dataConnections: DataConnection[], notebook?: NotebookKind, - dataConnections: DataConnection[] = [], ): [ dataConnection: DataConnectionData, - setDataConnection: (connection: DataConnectionData) => void, + setDataConnection: UpdateObjectAtPropAndValue, + resetDefaults: () => void, ] => { - const [dataConnection, setDataConnection] = React.useState({ - type: 'creating', - enabled: false, - }); - - React.useEffect(() => { - if (notebook) { - // find data connection from env list - const notebookDataConnectionSecret = getNotebookDataConnection(notebook, dataConnections) - ?.data.metadata.name; - - if (notebookDataConnectionSecret) { - setDataConnection({ + const notebookDataConnectionSecret = getNotebookDataConnection(notebook, dataConnections)?.data + .metadata.name; + const createDataState = useGenericObjectState( + notebookDataConnectionSecret + ? { type: 'existing', enabled: true, existing: { @@ -41,10 +38,12 @@ export const useNotebookDataConnection = ( name: notebookDataConnectionSecret, }, }, - }); - } - } - }, [notebook, dataConnections]); + } + : { + type: 'creating', + enabled: false, + }, + ); - return [dataConnection, setDataConnection]; + return createDataState; }; From a58257522dd637b70dcdc696c3706b93cf01a459 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Thu, 7 Sep 2023 15:06:43 -0400 Subject: [PATCH 2/9] Add owner references to Elyra role binding when creating notebooks --- frontend/src/api/k8s/notebooks.ts | 22 ++---- frontend/src/api/k8s/roleBindings.ts | 18 +++++ .../src/concepts/pipelines/elyra/utils.ts | 68 ++++++++++++++++++- .../notebook/NotebookStatusToggle.tsx | 3 +- 4 files changed, 93 insertions(+), 18 deletions(-) diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index d7a86ba07e..c3cb94f31d 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -17,14 +17,13 @@ import { translateDisplayNameForK8s } from '~/pages/projects/utils'; import { getTolerationPatch, TolerationChanges } from '~/utilities/tolerations'; import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; import { + createElyraServiceAccountRoleBinding, ELYRA_VOLUME_NAME, - generateElyraServiceAccountRoleBinding, getElyraVolume, getElyraVolumeMount, getPipelineVolumeMountPatch, getPipelineVolumePatch, } from '~/concepts/pipelines/elyra/utils'; -import { createRoleBinding } from '~/api'; import { Volume, VolumeMount } from '~/types'; import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; @@ -207,8 +206,7 @@ export const stopNotebook = (name: string, namespace: string): Promise => { @@ -223,18 +221,12 @@ export const startNotebook = async ( if (enablePipelines) { patches.push(getPipelineVolumePatch()); patches.push(getPipelineVolumeMountPatch()); - await createRoleBinding(generateElyraServiceAccountRoleBinding(name, namespace)).catch((e) => { - // This is not ideal, but it shouldn't impact the starting of the notebook. Let us log it, and mute the error - // eslint-disable-next-line no-console - console.error( - `Could not patch rolebinding to service account for notebook, ${name}; Reason ${e.message}`, - ); - }); + await createElyraServiceAccountRoleBinding(notebook); } return k8sPatchResource({ model: NotebookModel, - queryOptions: { name, ns: namespace }, + queryOptions: { name: notebook.metadata.name, ns: notebook.metadata.namespace }, patches, }); }; @@ -252,9 +244,9 @@ export const createNotebook = ( }); if (canEnablePipelines) { - return createRoleBinding( - generateElyraServiceAccountRoleBinding(notebook.metadata.name, notebook.metadata.namespace), - ).then(() => notebookPromise); + return notebookPromise.then((notebook) => + createElyraServiceAccountRoleBinding(notebook).then(() => notebook), + ); } return notebookPromise; diff --git a/frontend/src/api/k8s/roleBindings.ts b/frontend/src/api/k8s/roleBindings.ts index bdb129193f..16c83997a5 100644 --- a/frontend/src/api/k8s/roleBindings.ts +++ b/frontend/src/api/k8s/roleBindings.ts @@ -1,4 +1,5 @@ import { + OwnerReference, k8sCreateResource, k8sDeleteResource, k8sGetResource, @@ -162,3 +163,20 @@ export const patchRoleBindingName = ( }, ], }); + +export const patchRoleBindingOwnerRef = ( + rbName: string, + namespace: string, + ownerReferences: OwnerReference[], +): Promise => + k8sPatchResource({ + model: RoleBindingModel, + queryOptions: { name: rbName, ns: namespace }, + patches: [ + { + op: 'replace', + path: '/metadata/ownerReferences', + value: ownerReferences, + }, + ], + }); diff --git a/frontend/src/concepts/pipelines/elyra/utils.ts b/frontend/src/concepts/pipelines/elyra/utils.ts index 028819fbde..bcd430d0cb 100644 --- a/frontend/src/concepts/pipelines/elyra/utils.ts +++ b/frontend/src/concepts/pipelines/elyra/utils.ts @@ -10,9 +10,13 @@ import { import { AWS_KEYS } from '~/pages/projects/dataConnections/const'; import { Volume, VolumeMount } from '~/types'; import { RUNTIME_MOUNT_PATH } from '~/pages/projects/pvc/const'; +import { createRoleBinding, getRoleBinding, patchRoleBindingOwnerRef } from '~/api'; export const ELYRA_VOLUME_NAME = 'elyra-dsp-details'; +export const getElyraServiceAccountRoleBindingName = (notebookName: string) => + `elyra-pipelines-${notebookName}`; + export const getElyraVolumeMount = (): VolumeMount => ({ name: ELYRA_VOLUME_NAME, mountPath: RUNTIME_MOUNT_PATH, @@ -25,6 +29,13 @@ export const getElyraVolume = (): Volume => ({ }, }); +export const getElyraRoleBindingOwnerRef = (notebookName: string, ownerUid: string) => ({ + apiVersion: 'kubeflow.org/v1beta1', + kind: 'Notebook', + name: notebookName, + uid: ownerUid, +}); + export const getPipelineVolumePatch = (): Patch => ({ path: '/spec/template/spec/volumes/-', op: 'add', @@ -83,15 +94,17 @@ export const generateElyraSecret = ( export const generateElyraServiceAccountRoleBinding = ( notebookName: string, namespace: string, + ownerUid: string, ): RoleBindingKind => ({ apiVersion: 'rbac.authorization.k8s.io/v1', kind: 'RoleBinding', metadata: { - name: `elyra-pipelines-${notebookName}`, + name: getElyraServiceAccountRoleBindingName(notebookName), namespace, labels: { [KnownLabels.DASHBOARD_RESOURCE]: 'true', }, + ownerReferences: [getElyraRoleBindingOwnerRef(notebookName, ownerUid)], }, roleRef: { apiGroup: 'rbac.authorization.k8s.io', @@ -105,3 +118,56 @@ export const generateElyraServiceAccountRoleBinding = ( }, ], }); + +export const createElyraServiceAccountRoleBinding = async ( + notebook: NotebookKind, +): Promise => { + const notebookName = notebook.metadata.name; + const namespace = notebook.metadata.namespace; + const notebookUid = notebook.metadata.uid; + + // Check if rolebinding is already exists for backward compatibility + const roleBinding = await getRoleBinding( + namespace, + getElyraServiceAccountRoleBindingName(notebookName), + ).catch((e) => { + // 404 is not an error + if (e.statusObject?.code !== 404) { + // eslint-disable-next-line no-console + console.error( + `Could not get rolebinding to service account for notebook, ${notebookName}; Reason ${e.message}`, + ); + } + return undefined; + }); + + if (notebookUid) { + if (roleBinding) { + const ownerReferences = roleBinding.metadata.ownerReferences || []; + if (!ownerReferences.find((ownerReference) => ownerReference.uid === notebookUid)) { + ownerReferences.push(getElyraRoleBindingOwnerRef(notebookName, notebookUid)); + } + return patchRoleBindingOwnerRef( + roleBinding.metadata.name, + roleBinding.metadata.namespace, + ownerReferences, + ).catch((e) => { + // This is not ideal, but it shouldn't impact the starting of the notebook. Let us log it, and mute the error + // eslint-disable-next-line no-console + console.error( + `Could not patch rolebinding to service account for notebook, ${notebookName}; Reason ${e.message}`, + ); + }); + } + return createRoleBinding( + generateElyraServiceAccountRoleBinding(notebookName, namespace, notebookUid), + ).catch((e) => { + // eslint-disable-next-line no-console + console.error( + `Could not create rolebinding to service account for notebook, ${notebookName}; Reason ${e.message}`, + ); + }); + } + + return undefined; +}; diff --git a/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx b/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx index cc63f9c3a6..c134229d58 100644 --- a/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx +++ b/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx @@ -99,8 +99,7 @@ const NotebookStatusToggle: React.FC = ({ notebookState.notebook, ); startNotebook( - notebookName, - notebookNamespace, + notebook, tolerationSettings, enablePipelines && !currentlyHasPipelines(notebook), ).then(() => { From 208854b00bc89fd2263433f4d9d7ce5ab48d4cb9 Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Fri, 29 Sep 2023 13:34:26 -0400 Subject: [PATCH 3/9] refactor hook test utils to extend jest matchers --- frontend/jest.config.js | 2 + frontend/src/__tests__/unit/jest.d.ts | 29 +++ frontend/src/__tests__/unit/jest.setup.ts | 62 +++++ .../__tests__/unit/testUtils/hooks.spec.ts | 206 ++++++++++++----- .../src/__tests__/unit/testUtils/hooks.ts | 218 +++++++----------- .../__tests__/useGroups.spec.ts | 78 ++++--- .../utilities/__tests__/useFetchState.spec.ts | 122 ++++++---- frontend/src/utilities/useFetchState.ts | 20 +- 8 files changed, 463 insertions(+), 274 deletions(-) create mode 100644 frontend/src/__tests__/unit/jest.d.ts create mode 100644 frontend/src/__tests__/unit/jest.setup.ts diff --git a/frontend/jest.config.js b/frontend/jest.config.js index 68a37f4e48..4a65fc93d5 100644 --- a/frontend/jest.config.js +++ b/frontend/jest.config.js @@ -30,4 +30,6 @@ module.exports = { // A list of paths to snapshot serializer modules Jest should use for snapshot testing snapshotSerializers: [], + + setupFilesAfterEnv: ['/src/__tests__/unit/jest.setup.ts'], }; diff --git a/frontend/src/__tests__/unit/jest.d.ts b/frontend/src/__tests__/unit/jest.d.ts new file mode 100644 index 0000000000..3a99872fcf --- /dev/null +++ b/frontend/src/__tests__/unit/jest.d.ts @@ -0,0 +1,29 @@ +declare namespace jest { + interface Expect { + isIdentityEqual(expected: T): T; + } + + interface Matchers { + hookToBe(expected: unknown): R; + hookToStrictEqual(expected: unknown): R; + hookToHaveUpdateCount(expected: number): R; + hookToBeStable< + V extends T extends Pick< + import('~/__tests__/unit/testUtils/hooks').RenderHookResultExt< + infer Result, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + any + >, + 'result' + > + ? import('~/__tests__/unit/testUtils/hooks').BooleanValues + : never, + >( + expected?: V, + ): R; + } + + interface Expect { + isIdentityEqual(expected: unknown): AsymmetricMatcher; + } +} diff --git a/frontend/src/__tests__/unit/jest.setup.ts b/frontend/src/__tests__/unit/jest.setup.ts new file mode 100644 index 0000000000..7a2b431ba4 --- /dev/null +++ b/frontend/src/__tests__/unit/jest.setup.ts @@ -0,0 +1,62 @@ +import { JestAssertionError } from 'expect'; +import { + BooleanValues, + RenderHookResultExt, + createComparativeValue, +} from '~/__tests__/unit/testUtils/hooks'; + +const tryExpect = (expectFn: () => void) => { + try { + expectFn(); + } catch (e) { + const { matcherResult } = e as JestAssertionError; + if (matcherResult) { + return { ...matcherResult, message: () => matcherResult.message }; + } + throw e; + } + return { + pass: true, + message: () => '', + }; +}; + +expect.extend({ + // custom asymmetric matchers + + /** + * Checks that a value is what you expect. + * It uses Object.is to check strict equality. + * + * Usage: + * expect.isIdentifyEqual(...) + */ + isIdentityEqual: (actual, expected) => ({ + pass: Object.is(actual, expected), + message: () => `expected ${actual} to be identity equal to ${expected}`, + }), + + // hook related custom matchers + hookToBe: (actual: RenderHookResultExt, expected) => + tryExpect(() => expect(actual.result.current).toBe(expected)), + + hookToStrictEqual: (actual: RenderHookResultExt, expected) => + tryExpect(() => expect(actual.result.current).toStrictEqual(expected)), + + hookToHaveUpdateCount: (actual: RenderHookResultExt, expected: number) => + tryExpect(() => expect(actual.getUpdateCount()).toBe(expected)), + + hookToBeStable: (actual: RenderHookResultExt, expected?: BooleanValues) => { + if (actual.getUpdateCount() <= 1) { + throw new Error('Cannot assert stability as the hook has not run at least 2 times.'); + } + if (typeof expected === 'undefined') { + return tryExpect(() => expect(actual.result.current).toBe(actual.getPreviousResult())); + } + return tryExpect(() => + expect(actual.result.current).toStrictEqual( + createComparativeValue(actual.getPreviousResult(), expected), + ), + ); + }, +}); diff --git a/frontend/src/__tests__/unit/testUtils/hooks.spec.ts b/frontend/src/__tests__/unit/testUtils/hooks.spec.ts index dac0f2897a..9b698ea1e0 100644 --- a/frontend/src/__tests__/unit/testUtils/hooks.spec.ts +++ b/frontend/src/__tests__/unit/testUtils/hooks.spec.ts @@ -1,5 +1,5 @@ import * as React from 'react'; -import { expectHook, renderHook, standardUseFetchState, testHook } from './hooks'; +import { createComparativeValue, renderHook, standardUseFetchState, testHook } from './hooks'; const useSayHello = (who: string, showCount = false) => { const countRef = React.useRef(0); @@ -18,34 +18,33 @@ const useSayHelloDelayed = (who: string, delay = 0) => { describe('hook test utils', () => { it('simple testHook', () => { - const renderResult = testHook((who: string) => `Hello ${who}!`, 'world'); - expectHook(renderResult).toBe('Hello world!').toHaveUpdateCount(1); + const renderResult = testHook((who: string) => `Hello ${who}!`)('world'); + expect(renderResult).hookToBe('Hello world!'); + expect(renderResult).hookToHaveUpdateCount(1); renderResult.rerender('world'); - expectHook(renderResult).toBe('Hello world!').toBeStable().toHaveUpdateCount(2); + expect(renderResult).hookToBe('Hello world!'); + expect(renderResult).hookToBeStable(); + expect(renderResult).hookToHaveUpdateCount(2); }); it('use testHook for rendering', () => { - const renderResult = testHook(useSayHello, 'world'); - expectHook(renderResult) - .toHaveUpdateCount(1) - .toBe('Hello world!') - .toStrictEqual('Hello world!'); + const renderResult = testHook(useSayHello)('world'); + expect(renderResult).hookToHaveUpdateCount(1); + expect(renderResult).hookToBe('Hello world!'); + expect(renderResult).hookToStrictEqual('Hello world!'); renderResult.rerender('world', false); - expectHook(renderResult) - .toHaveUpdateCount(2) - .toBe('Hello world!') - .toStrictEqual('Hello world!') - .toBeStable(); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBe('Hello world!'); + expect(renderResult).hookToStrictEqual('Hello world!'); + expect(renderResult).hookToBeStable(); renderResult.rerender('world', true); - expectHook(renderResult) - .toHaveUpdateCount(3) - .toBe('Hello world! x3') - .toStrictEqual('Hello world! x3') - .toBeStable(false); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBe('Hello world! x3'); + expect(renderResult).hookToStrictEqual('Hello world! x3'); }); it('use renderHook for rendering', () => { @@ -59,50 +58,47 @@ describe('hook test utils', () => { }, }); - expectHook(renderResult) - .toHaveUpdateCount(1) - .toBe('Hello world!') - .toStrictEqual('Hello world!'); + expect(renderResult).hookToHaveUpdateCount(1); + expect(renderResult).hookToBe('Hello world!'); + expect(renderResult).hookToStrictEqual('Hello world!'); renderResult.rerender({ who: 'world', }); - expectHook(renderResult) - .toHaveUpdateCount(2) - .toBe('Hello world!') - .toStrictEqual('Hello world!') - .toBeStable(); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBe('Hello world!'); + expect(renderResult).hookToStrictEqual('Hello world!'); renderResult.rerender({ who: 'world', showCount: true }); - expectHook(renderResult) - .toHaveUpdateCount(3) - .toBe('Hello world! x3') - .toStrictEqual('Hello world! x3') - .toBeStable(false); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBe('Hello world! x3'); + expect(renderResult).hookToStrictEqual('Hello world! x3'); }); it('should use waitForNextUpdate for async update testing', async () => { - const renderResult = testHook(useSayHelloDelayed, 'world'); - expectHook(renderResult).toHaveUpdateCount(1).toBe(''); + const renderResult = testHook(useSayHelloDelayed)('world'); + expect(renderResult).hookToHaveUpdateCount(1); + expect(renderResult).hookToBe(''); await renderResult.waitForNextUpdate(); - expectHook(renderResult).toHaveUpdateCount(2).toBe('Hello world!'); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBe('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); + expect(renderResult).hookToHaveUpdateCount(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); + expect(renderResult).hookToHaveUpdateCount(1); }); it('should not throw if waitForNextUpdate timeout is sufficient', async () => { @@ -112,43 +108,47 @@ describe('hook test utils', () => { renderResult.waitForNextUpdate({ timeout: 50, interval: 10 }), ).resolves.not.toThrow(); - expectHook(renderResult).toHaveUpdateCount(2); + expect(renderResult).hookToHaveUpdateCount(2); }); it('should assert stability of results using isStable', () => { let testValue = 'test'; const renderResult = renderHook(() => testValue); - expectHook(renderResult).toHaveUpdateCount(1); + expect(renderResult).hookToHaveUpdateCount(1); renderResult.rerender(); - expectHook(renderResult).toHaveUpdateCount(2).toBeStable(true); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable(); testValue = 'new'; renderResult.rerender(); - expectHook(renderResult).toHaveUpdateCount(3).toBeStable(false); + expect(renderResult).hookToHaveUpdateCount(3); renderResult.rerender(); - expectHook(renderResult).toHaveUpdateCount(4).toBeStable(true); + expect(renderResult).hookToHaveUpdateCount(4); + expect(renderResult).hookToBeStable(); }); - it('should assert stability of results using isStableArray', () => { - let testValue = 'test'; + it(`should assert stability of result using isStable 'array'`, () => { + 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); + const renderResult = renderHook(() => testValue); + expect(renderResult).hookToHaveUpdateCount(1); renderResult.rerender(); - expectHook(renderResult).toHaveUpdateCount(2).toBeStable(false); - expectHook(renderResult).toHaveUpdateCount(2).toBeStable([true]); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable(); + expect(renderResult).hookToBeStable([true]); - testValue = 'new'; + testValue = ['new']; renderResult.rerender(); - expectHook(renderResult).toHaveUpdateCount(3).toBeStable(false); - expectHook(renderResult).toHaveUpdateCount(3).toBeStable([false]); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([false]); renderResult.rerender(); - expectHook(renderResult).toHaveUpdateCount(4).toBeStable(false); - expectHook(renderResult).toHaveUpdateCount(4).toBeStable([true]); + expect(renderResult).hookToHaveUpdateCount(4); + expect(renderResult).hookToBeStable(); + expect(renderResult).hookToBeStable([true]); }); it('standardUseFetchState should return an array matching the state of useFetchState', () => { @@ -160,4 +160,102 @@ describe('hook test utils', () => { standardUseFetchState('test', false, new Error('error')), ); }); + + describe('createComparativeValue', () => { + it('should extract array values according to the boolean object', () => { + expect([1, 2, 3]).toStrictEqual(createComparativeValue([1, 2, 3], [true, true, true])); + expect([1, 2, 3]).toStrictEqual(createComparativeValue([1, 2, 3], [true, true, false])); + expect([1, 2, 3]).toStrictEqual(createComparativeValue([1, 2, 4], [true, true, false])); + expect([1, 2, 3]).toStrictEqual(createComparativeValue([1, 2, 4], [true, true])); + expect([1, 2, 3]).not.toStrictEqual(createComparativeValue([1, 4, 3], [true, true, true])); + }); + + it('should extract object values according to the boolean object', () => { + expect({ a: 1, b: 2, c: 3 }).toStrictEqual( + createComparativeValue({ a: 1, b: 2, c: 3 }, { a: true, b: true, c: true }), + ); + expect({ a: 1, b: 2, c: 3 }).toStrictEqual( + createComparativeValue({ a: 1, b: 2, c: 3 }, { a: true, b: true, c: true }), + ); + expect({ a: 1, b: 2, c: 3 }).toStrictEqual( + createComparativeValue({ a: 1, b: 2, c: 4 }, { a: true, b: true, c: false }), + ); + expect({ a: 1, b: 2, c: 3 }).toStrictEqual( + createComparativeValue({ a: 1, b: 2, c: 4 }, { a: true, b: true }), + ); + expect({ a: 1, b: 2, c: 3 }).not.toStrictEqual( + createComparativeValue({ a: 1, b: 4, c: 3 }, { a: true, b: true, c: true }), + ); + }); + + it('should extract nested values', () => { + const testValue = { + a: 1, + b: { + c: 2, + d: [{ e: 3 }, 'f', {}], + }, + }; + expect(testValue).toStrictEqual( + createComparativeValue( + { a: 10, b: { c: 2, d: [null, 'f'] } }, + { + b: { + c: true, + d: [false, true], + }, + }, + ), + ); + }); + + it('should extract objects for identity comparisons', () => { + const obj = {}; + const array: string[] = []; + const testValue = { + a: obj, + b: array, + c: { + d: obj, + e: array, + }, + }; + + expect(testValue).not.toStrictEqual( + createComparativeValue( + { + a: {}, + b: [], + c: { + d: {}, + e: [], + }, + }, + { + a: true, + b: true, + c: { d: true, e: true }, + }, + ), + ); + + expect(testValue).toStrictEqual( + createComparativeValue( + { + a: obj, + b: array, + c: { + d: obj, + e: array, + }, + }, + { + a: true, + b: true, + c: { d: true, e: true }, + }, + ), + ); + }); + }); }); diff --git a/frontend/src/__tests__/unit/testUtils/hooks.ts b/frontend/src/__tests__/unit/testUtils/hooks.ts index 4f13d7ee67..98e5136f3b 100644 --- a/frontend/src/__tests__/unit/testUtils/hooks.ts +++ b/frontend/src/__tests__/unit/testUtils/hooks.ts @@ -7,50 +7,25 @@ import { } 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; -}; +export type BooleanValues = T extends + | boolean + | number + | string + | null + | undefined + // eslint-disable-next-line @typescript-eslint/ban-types + | Function + ? boolean | undefined + : boolean | undefined | { [K in keyof T]?: BooleanValues }; /** * 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. + * Returns the previous result. */ - 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[]; + getPreviousResult: () => Result; /** * Get the update count for how many times the hook has been rendered. @@ -67,61 +42,6 @@ export type RenderHookResultExt = RenderHookResult 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. * @@ -131,9 +51,9 @@ export const expectHook = ( * * ``` * const renderResult = renderHook(({ who }: { who: string }) => useSayHello(who), { initialProps: { who: 'world' }}); - * expectHook(renderResult).toBe('Hello world!'); + * expect(renderResult).hookToBe('Hello world!'); * renderResult.rerender({ who: 'there' }); - * expectHook(renderResult).toBe('Hello there!'); + * expect(renderResult).hookToBe('Hello there!'); * ``` */ export const renderHook = < @@ -160,28 +80,8 @@ export const renderHook = < 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; - }, + getPreviousResult: () => + updateCount > 1 ? (prevResult as Result) : renderResult.result.current, getUpdateCount: () => updateCount, @@ -204,30 +104,34 @@ export const renderHook = < * Prefer this method of testing over `renderHook` for simplicity. * * ``` - * const renderResult = testHook(useSayHello, 'world'); + * 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 }), +export const testHook = + (hook: (...params: P) => Result) => + // not ideal to nest functions in terms of API but cannot find a better way to infer P from hook and not initialParams + ( + ...initialParams: P + ): Omit, 'rerender'> & { + rerender: (...params: typeof initialParams) => void; + } => { + const renderResult = renderHook( + ({ $params }) => hook(...$params), + { + initialProps: { + $params: initialParams, + }, + }, + ); + + return { + ...renderResult, + rerender: (...params) => renderResult.rerender({ $params: params }), + }; }; -}; /** * A helper function for asserting the return value of hooks based on `useFetchState`. @@ -251,3 +155,49 @@ export const standardUseFetchState = ( loadError: Error | undefined, refresh: () => Promise, ] => [data, loaded, error, expect.any(Function)]; + +/** + * Extracts a subset of values from the source that can be used to compare equality. + * + * Recursively traverses the `booleanTarget`. For every property or array index equal to `true`, + * adds the value of the source to the result wrapped in custom matcher `expect.isIdentityEqual`. + * If the entry is `false` or `undefined`, adds matcher `expect.anything()` to the result. + */ +export const createComparativeValue = (source: T, booleanTarget: BooleanValues) => + createComparativeValueRecursive(source, booleanTarget); + +const createComparativeValueRecursive = ( + source: unknown, + // eslint-disable-next-line @typescript-eslint/ban-types + booleanTarget: boolean | string | number | Function | BooleanValues, +) => { + if (typeof booleanTarget === 'boolean') { + return booleanTarget ? expect.isIdentityEqual(source) : expect.anything(); + } + if (Array.isArray(booleanTarget)) { + if (Array.isArray(source)) { + return expect.arrayContaining( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + booleanTarget.map((b, i): any => + b == null ? expect.anything() : createComparativeValueRecursive(source[i], b), + ), + ); + } + return undefined; + } + if ( + source == null || + typeof source === 'string' || + typeof source === 'number' || + typeof source === 'function' + ) { + return source; + } + const obj: { [k: string]: unknown } = {}; + const btObj = booleanTarget as { [k: string]: unknown }; + Object.keys(btObj).forEach((key) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + obj[key] = createComparativeValueRecursive((source as any)[key] as unknown, btObj[key] as any); + }); + return expect.objectContaining(obj); +}; diff --git a/frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts b/frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts index fcf404f1a4..21dd760c69 100644 --- a/frontend/src/pages/projects/projectSharing/__tests__/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 '~/__tests__/unit/testUtils/hooks'; +import { standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ k8sListResource: jest.fn(), @@ -17,23 +17,24 @@ describe('useGroups', () => { }; k8sListResourceMock.mockReturnValue(Promise.resolve(mockList)); - const renderResult = testHook(useGroups); + const renderResult = testHook(useGroups)(); expect(k8sListResourceMock).toHaveBeenCalledTimes(1); - expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(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]); + expect(renderResult).hookToStrictEqual(standardUseFetchState(mockList.items, true)); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([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]); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([false, true, true, true]); }); it('should handle 403 as an empty result', async () => { @@ -44,26 +45,25 @@ describe('useGroups', () => { }; k8sListResourceMock.mockReturnValue(Promise.reject(error)); - const renderResult = testHook(useGroups); + const renderResult = testHook(useGroups)(); expect(k8sListResourceMock).toHaveBeenCalledTimes(1); - expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); // wait for update await renderResult.waitForNextUpdate(); expect(k8sListResourceMock).toHaveBeenCalledTimes(1); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState([], true)) - .toHaveUpdateCount(2) - .toBeStable([false, false, true, true]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([], true)); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([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]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([], true)); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([false, true, true, true]); }); it('should handle 404 as an error', async () => { @@ -74,17 +74,19 @@ describe('useGroups', () => { }; k8sListResourceMock.mockReturnValue(Promise.reject(error)); - const renderResult = testHook(useGroups); + const renderResult = testHook(useGroups)(); expect(k8sListResourceMock).toHaveBeenCalledTimes(1); - expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(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(renderResult).hookToStrictEqual( + standardUseFetchState([], false, new Error('No groups found.')), + ); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([true, true, false, true]); expect(k8sListResourceMock).toHaveBeenCalledTimes(1); @@ -92,34 +94,34 @@ describe('useGroups', () => { 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]); + expect(renderResult).hookToStrictEqual( + standardUseFetchState([], false, new Error('No groups found.')), + ); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([true, true, false, true]); }); it('should handle other errors and rethrow', async () => { k8sListResourceMock.mockReturnValue(Promise.reject(new Error('error1'))); - const renderResult = testHook(useGroups); + const renderResult = testHook(useGroups)(); expect(k8sListResourceMock).toHaveBeenCalledTimes(1); - expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(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]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([], false, new Error('error1'))); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([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]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([], false, new Error('error2'))); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([true, true, false, true]); }); }); diff --git a/frontend/src/utilities/__tests__/useFetchState.spec.ts b/frontend/src/utilities/__tests__/useFetchState.spec.ts index f29f380a21..b8081a64f2 100644 --- a/frontend/src/utilities/__tests__/useFetchState.spec.ts +++ b/frontend/src/utilities/__tests__/useFetchState.spec.ts @@ -1,97 +1,125 @@ -import { act } from '@testing-library/react'; -import useFetchState from '~/utilities/useFetchState'; -import { expectHook, standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; +import { act, waitFor } from '@testing-library/react'; +import useFetchState, { FetchState } from '~/utilities/useFetchState'; +import { standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; jest.useFakeTimers(); describe('useFetchState', () => { it('should be successful', async () => { - const renderResult = testHook( - useFetchState, + const renderResult = testHook(useFetchState)( () => Promise.resolve('success-test-state'), 'default-test-state', ); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState('default-test-state')) - .toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState('default-test-state')); + expect(renderResult).hookToHaveUpdateCount(1); await renderResult.waitForNextUpdate(); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState('success-test-state', true)) - .toHaveUpdateCount(2) - .toBeStable([false, false, true, true]); + expect(renderResult).hookToStrictEqual(standardUseFetchState('success-test-state', true)); + expect(renderResult).hookToHaveUpdateCount(2); }); it('should fail', async () => { - const renderResult = testHook( - useFetchState, + const renderResult = testHook(useFetchState)( () => Promise.reject(new Error('error-test-state')), 'default-test-state', ); - - expectHook(renderResult) - .toStrictEqual(standardUseFetchState('default-test-state')) - .toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState('default-test-state')); + expect(renderResult).hookToHaveUpdateCount(1); await renderResult.waitForNextUpdate(); - expectHook(renderResult) - .toStrictEqual( - standardUseFetchState('default-test-state', false, new Error('error-test-state')), - ) - .toHaveUpdateCount(2) - .toBeStable([true, true, false, true]); + expect(renderResult).hookToStrictEqual( + standardUseFetchState('default-test-state', false, new Error('error-test-state')), + ); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([true, true, false, true]); }); it('should refresh', async () => { - const renderResult = testHook(useFetchState, () => Promise.resolve([1, 2, 3]), [], { + const renderResult = testHook(useFetchState)(() => Promise.resolve([1, 2, 3]), [], { refreshRate: 1000, }); - expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); await renderResult.waitForNextUpdate(); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState([1, 2, 3], true)) - .toHaveUpdateCount(2) - .toBeStable([false, false, true, true]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([1, 2, 3], true)); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([false, false, true, true]); await act(() => { jest.advanceTimersByTime(900); }); - expectHook(renderResult).toHaveUpdateCount(2); + expect(renderResult).hookToHaveUpdateCount(2); await act(async () => { jest.advanceTimersByTime(100); }); - expectHook(renderResult).toHaveUpdateCount(3); + expect(renderResult).hookToHaveUpdateCount(3); await renderResult.waitForNextUpdate(); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState([1, 2, 3], true)) - .toHaveUpdateCount(4) - .toBeStable([false, true, true, true]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([1, 2, 3], true)); + expect(renderResult).hookToHaveUpdateCount(4); + expect(renderResult).hookToBeStable([false, true, true, true]); }); it('should test stability', async () => { - const renderResult = testHook(useFetchState, () => Promise.resolve([1, 2, 3]), []); - expectHook(renderResult).toStrictEqual(standardUseFetchState([])).toHaveUpdateCount(1); + const renderResult = testHook(useFetchState)(() => Promise.resolve([1, 2, 3]), []); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); await renderResult.waitForNextUpdate(); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState([1, 2, 3], true)) - .toHaveUpdateCount(2) - .toBeStable([false, false, true, true]); + + expect(renderResult).hookToStrictEqual(standardUseFetchState([1, 2, 3], true)); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([false, false, true, true]); renderResult.rerender(() => Promise.resolve([1, 2, 4]), []); - expectHook(renderResult).toHaveUpdateCount(3).toBeStable([true, true, true, true]); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([true, true, true, true]); await renderResult.waitForNextUpdate(); - expectHook(renderResult) - .toStrictEqual(standardUseFetchState([1, 2, 4], true)) - .toHaveUpdateCount(4) - .toBeStable([false, true, true, true]); + expect(renderResult).hookToStrictEqual(standardUseFetchState([1, 2, 4], true)); + expect(renderResult).hookToHaveUpdateCount(4); + expect(renderResult).hookToBeStable([false, true, true, true]); + }); + + it('should have a stable default values when initialPromisePurity=true', async () => { + const oriDefaultValue = [10]; + const result: FetchState[] = []; + + const renderResult = testHook((...args: Parameters>) => { + // wrap useFetchState to capture all executions inbetween useEffects + const state = useFetchState(...args); + result.push(state); + return state; + })(() => Promise.resolve([1, 2, 3]), oriDefaultValue, { + initialPromisePurity: true, + }); + + expect(result[0][0]).toBe(oriDefaultValue); + expect(result[0][1]).toBe(false); + + await waitFor(() => expect(result).toHaveLength(2)); + expect(result[1][0]).toStrictEqual([1, 2, 3]); + expect(result[1][1]).toBe(true); + + // rerender but with a promise that doens't resolve + renderResult.rerender(() => new Promise(() => null), [11], { + initialPromisePurity: true, + }); + + expect(result).toHaveLength(4); + + // update immediately after hook completes but before useEffects are run + expect(result[2][0]).toBe(oriDefaultValue); + expect(result[2][1]).toBe(false); + + // final update after all useEffects are run + expect(result[3][0]).toBe(oriDefaultValue); + expect(result[3][1]).toBe(false); }); }); diff --git a/frontend/src/utilities/useFetchState.ts b/frontend/src/utilities/useFetchState.ts index 03c0c3da2f..a542c47eb8 100644 --- a/frontend/src/utilities/useFetchState.ts +++ b/frontend/src/utilities/useFetchState.ts @@ -111,7 +111,7 @@ type FetchOptions = { */ const useFetchState = ( /** React.useCallback result. */ - fetchCallbackPromise: FetchStateCallbackPromise | FetchStateCallbackPromiseAdHoc, + fetchCallbackPromise: FetchStateCallbackPromise>, /** * A preferred default states - this is ignored after the first render * Note: This is only read as initial value; changes do nothing. @@ -120,10 +120,13 @@ const useFetchState = ( /** Configurable features */ { refreshRate = 0, initialPromisePurity = false }: Partial = {}, ): FetchState => { + const initialDefaultStateRef = React.useRef(initialDefaultState); const [result, setResult] = React.useState(initialDefaultState); const [loaded, setLoaded] = React.useState(false); const [loadError, setLoadError] = React.useState(undefined); const abortCallbackRef = React.useRef<() => void>(() => undefined); + const changePendingRef = React.useRef(true); + /** Setup on initial hook a singular reset function. DefaultState & resetDataOnNewPromise are initial render states. */ const cleanupRef = React.useRef(() => { if (initialPromisePurity) { @@ -145,6 +148,7 @@ const useFetchState = ( const doRequest = () => fetchCallbackPromise({ signal: abortController.signal }) .then((r) => { + changePendingRef.current = false; if (alreadyAborted) { return undefined; } @@ -178,6 +182,7 @@ const useFetchState = ( return r; }) .catch((e) => { + changePendingRef.current = false; if (alreadyAborted) { return undefined; } @@ -190,6 +195,7 @@ const useFetchState = ( }); const unload = () => { + changePendingRef.current = false; if (alreadyAborted) { return; } @@ -201,6 +207,13 @@ const useFetchState = ( return [doRequest(), unload]; }, [fetchCallbackPromise]); + // Use a memmo to update the `changePendingRef` immediately on change. + React.useMemo(() => { + changePendingRef.current = true; + // React to changes to the `call` reference. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [call]); + React.useEffect(() => { let interval: ReturnType; @@ -234,6 +247,11 @@ const useFetchState = ( return callPromise; }, []); + // Return the default reset state if a change is pending and initialPromisePurity is true + if (initialPromisePurity && changePendingRef.current) { + return [initialDefaultStateRef.current, false, undefined, refresh]; + } + return [result, loaded, loadError, refresh]; }; From 302f7091e0c52764fc9841dd8c43f400d57ccbde Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 1 Oct 2023 09:23:31 +0000 Subject: [PATCH 4/9] Bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/pr-close-image-delete.yml | 2 +- .github/workflows/pull_request.yml | 2 +- .github/workflows/quay-tag.yml | 2 +- .github/workflows/vuln_scan.yml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pr-close-image-delete.yml b/.github/workflows/pr-close-image-delete.yml index a4026c45cd..e1b5589089 100644 --- a/.github/workflows/pr-close-image-delete.yml +++ b/.github/workflows/pr-close-image-delete.yml @@ -12,7 +12,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Git checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: fetch-depth: '0' - name: Install skopeo diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 97ad7b786d..d95072290d 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -10,7 +10,7 @@ jobs: matrix: node-version: [18.x] steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Setup Node.js ${{ matrix.node-version }} uses: actions/setup-node@v3.8.1 with: diff --git a/.github/workflows/quay-tag.yml b/.github/workflows/quay-tag.yml index 2905f5026b..aad1257d9f 100644 --- a/.github/workflows/quay-tag.yml +++ b/.github/workflows/quay-tag.yml @@ -16,7 +16,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Git checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: fetch-depth: '0' - name: Install skopeo diff --git a/.github/workflows/vuln_scan.yml b/.github/workflows/vuln_scan.yml index 910dc4a876..30405e857c 100644 --- a/.github/workflows/vuln_scan.yml +++ b/.github/workflows/vuln_scan.yml @@ -8,7 +8,7 @@ jobs: name: Scan Files runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Run Trivy vulnerability scanner for filesystem uses: aquasecurity/trivy-action@0.11.2 with: From 3c1efff044ef736a5bd72b28f821abcd4b2a11ca Mon Sep 17 00:00:00 2001 From: Deepak Chourasia Date: Tue, 3 Oct 2023 12:48:58 +0530 Subject: [PATCH 5/9] adding workflow to auto-add issues to project boards --- .../workflows/auto-add-issues-to-project.yaml | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 .github/workflows/auto-add-issues-to-project.yaml diff --git a/.github/workflows/auto-add-issues-to-project.yaml b/.github/workflows/auto-add-issues-to-project.yaml new file mode 100644 index 0000000000..afc36f4967 --- /dev/null +++ b/.github/workflows/auto-add-issues-to-project.yaml @@ -0,0 +1,24 @@ +name: Auto Add Issues to Tracking boards +on: + issues: + types: + - opened +jobs: + add-to-project: + name: Add issue to projects + runs-on: ubuntu-latest + steps: + - name: Generate github-app token + id: app-token + uses: getsentry/action-github-app-token@v2 + with: + app_id: ${{ secrets.DEVOPS_APP_ID }} + private_key: ${{ secrets.DEVOPS_APP_PRIVATE_KEY }} + - uses: actions/add-to-project@v0.5.0 + with: + project-url: https://github.com/orgs/opendatahub-io/projects/40 + github-token: ${{ secrets.GH_TOKEN }} + - uses: actions/add-to-project@v0.5.0 + with: + project-url: https://github.com/orgs/opendatahub-io/projects/45 + github-token: ${{ secrets.GH_TOKEN }} From c62aaa3c19b8522d669477417378657ce0391e29 Mon Sep 17 00:00:00 2001 From: Deepak Chourasia Date: Tue, 3 Oct 2023 13:23:26 +0530 Subject: [PATCH 6/9] adding workflow to auto-add issues to project boards --- .github/workflows/auto-add-issues-to-project.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/auto-add-issues-to-project.yaml b/.github/workflows/auto-add-issues-to-project.yaml index afc36f4967..8ad02516ca 100644 --- a/.github/workflows/auto-add-issues-to-project.yaml +++ b/.github/workflows/auto-add-issues-to-project.yaml @@ -17,8 +17,8 @@ jobs: - uses: actions/add-to-project@v0.5.0 with: project-url: https://github.com/orgs/opendatahub-io/projects/40 - github-token: ${{ secrets.GH_TOKEN }} + github-token: ${{ steps.app-token.outputs.token }} - uses: actions/add-to-project@v0.5.0 with: project-url: https://github.com/orgs/opendatahub-io/projects/45 - github-token: ${{ secrets.GH_TOKEN }} + github-token: ${{ steps.app-token.outputs.token }} From 09cf73c21c6c3012c80eec15c98ec91bb8654461 Mon Sep 17 00:00:00 2001 From: manaswinidas Date: Tue, 5 Sep 2023 17:32:23 +0530 Subject: [PATCH 7/9] Delete pipeline server from the Pipelines Section in Projects page --- .../pipeline/PipelineServerActions.tsx | 84 +++++++++++++++++++ .../global/pipelines/GlobalPipelines.tsx | 4 +- .../pipelines/PipelinesPageHeaderActions.tsx | 48 ----------- .../detail/pipelines/PipelinesSection.tsx | 6 ++ 4 files changed, 92 insertions(+), 50 deletions(-) create mode 100644 frontend/src/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineServerActions.tsx delete mode 100644 frontend/src/pages/pipelines/global/pipelines/PipelinesPageHeaderActions.tsx diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineServerActions.tsx b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineServerActions.tsx new file mode 100644 index 0000000000..689921b116 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineServerActions.tsx @@ -0,0 +1,84 @@ +import * as React from 'react'; +import { + Dropdown, + DropdownItem, + DropdownSeparator, + DropdownToggle, + KebabToggle, + Tooltip, +} from '@patternfly/react-core'; +import { DeleteServerModal, ViewServerModal } from '~/concepts/pipelines/context'; + +type PipelineServerActionsProps = { + variant?: 'kebab' | 'dropdown'; + isDisabled: boolean; +}; + +const PipelineServerActions: React.FC = ({ variant, isDisabled }) => { + const [open, setOpen] = React.useState(false); + const [deleteOpen, setDeleteOpen] = React.useState(false); + const [viewOpen, setViewOpen] = React.useState(false); + + const DropdownComponent = ( + setOpen(false)} + toggle={ + variant === 'kebab' ? ( + setOpen(!open)} /> + ) : ( + setOpen(!open)} + > + Pipeline server actions + + ) + } + isOpen={open} + position="right" + isPlain={variant === 'kebab'} + dropdownItems={[ + setViewOpen(true)}> + View pipeline server configuration + , + , + { + setDeleteOpen(true); + }} + key="delete-server" + > + Delete pipeline server + , + ]} + /> + ); + + if (isDisabled) { + return ( + + {DropdownComponent} + + ); + } + + return ( + <> + {DropdownComponent} + { + setDeleteOpen(false); + }} + /> + setViewOpen(false)} /> + + ); +}; + +export default PipelineServerActions; diff --git a/frontend/src/pages/pipelines/global/pipelines/GlobalPipelines.tsx b/frontend/src/pages/pipelines/global/pipelines/GlobalPipelines.tsx index d26e99d2a5..9dd02f7bb0 100644 --- a/frontend/src/pages/pipelines/global/pipelines/GlobalPipelines.tsx +++ b/frontend/src/pages/pipelines/global/pipelines/GlobalPipelines.tsx @@ -4,7 +4,7 @@ import { pipelinesPageDescription, pipelinesPageTitle, } from '~/pages/pipelines/global/pipelines/const'; -import PipelinesPageHeaderActions from '~/pages/pipelines/global/pipelines/PipelinesPageHeaderActions'; +import PipelineServerActions from '~/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineServerActions'; import PipelineCoreApplicationPage from '~/pages/pipelines/global/PipelineCoreApplicationPage'; import PipelinesView from '~/pages/pipelines/global/pipelines/PipelinesView'; import EnsureAPIAvailability from '~/concepts/pipelines/EnsureAPIAvailability'; @@ -16,7 +16,7 @@ const GlobalPipelines: React.FC = () => { } + headerAction={} getRedirectPath={(namespace) => `/pipelines/${namespace}`} > diff --git a/frontend/src/pages/pipelines/global/pipelines/PipelinesPageHeaderActions.tsx b/frontend/src/pages/pipelines/global/pipelines/PipelinesPageHeaderActions.tsx deleted file mode 100644 index 7c88fbaf72..0000000000 --- a/frontend/src/pages/pipelines/global/pipelines/PipelinesPageHeaderActions.tsx +++ /dev/null @@ -1,48 +0,0 @@ -import * as React from 'react'; -import { Dropdown, DropdownItem, DropdownSeparator, DropdownToggle } from '@patternfly/react-core'; -import { DeleteServerModal, ViewServerModal } from '~/concepts/pipelines/context'; - -const PipelinesPageHeaderActions: React.FC = () => { - const [open, setOpen] = React.useState(false); - const [deleteOpen, setDeleteOpen] = React.useState(false); - const [viewOpen, setViewOpen] = React.useState(false); - - return ( - <> - setOpen(false)} - toggle={ - setOpen(!open)}> - Pipeline server actions - - } - isOpen={open} - position="right" - dropdownItems={[ - setViewOpen(true)}> - View pipeline server configuration - , - , - { - setDeleteOpen(true); - }} - key="delete-server" - > - Delete pipeline server - , - ]} - /> - { - setDeleteOpen(false); - }} - /> - setViewOpen(false)} /> - - ); -}; - -export default PipelinesPageHeaderActions; diff --git a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx index d8039dde92..b3e865c791 100644 --- a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx +++ b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx @@ -8,6 +8,7 @@ import NoPipelineServer from '~/concepts/pipelines/NoPipelineServer'; import ImportPipelineButton from '~/concepts/pipelines/content/import/ImportPipelineButton'; import PipelinesList from '~/pages/projects/screens/detail/pipelines/PipelinesList'; import EnsureAPIAvailability from '~/concepts/pipelines/EnsureAPIAvailability'; +import PipelineServerActions from '~/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineServerActions'; const PipelinesSection: React.FC = () => { const { @@ -27,6 +28,11 @@ const PipelinesSection: React.FC = () => { key={`action-${ProjectSectionID.PIPELINES}`} variant="secondary" />, + , ]} isLoading={initializing} isEmpty={!installed} From 2969ea84b8d36d4964a5c1ac4d6a277de4f61123 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Tue, 3 Oct 2023 15:31:17 -0500 Subject: [PATCH 8/9] refresh accelerators after reset --- frontend/src/utilities/useAcceleratorState.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frontend/src/utilities/useAcceleratorState.ts b/frontend/src/utilities/useAcceleratorState.ts index 83a55cce42..08f9f0175d 100644 --- a/frontend/src/utilities/useAcceleratorState.ts +++ b/frontend/src/utilities/useAcceleratorState.ts @@ -31,7 +31,7 @@ const useAcceleratorState = ( }); const { dashboardNamespace } = useDashboardNamespace(); - const [accelerators, loaded, loadError] = useAccelerators(dashboardNamespace); + const [accelerators, loaded, loadError, refresh] = useAccelerators(dashboardNamespace); React.useEffect(() => { if (loaded && !loadError) { @@ -124,7 +124,12 @@ const useAcceleratorState = ( } }, [accelerators, loaded, loadError, resources, tolerations, existingAcceleratorName, setData]); - return [acceleratorState, setData, resetData]; + const resetDataAndRefresh = React.useCallback(() => { + resetData(); + refresh(); + }, [refresh, resetData]); + + return [acceleratorState, setData, resetDataAndRefresh]; }; export default useAcceleratorState; From 6f4ada7ad330c5ab23c9458d81478d3ffc4731ce Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 4 Oct 2023 10:56:49 +0000 Subject: [PATCH 9/9] Bump postcss from 8.4.23 to 8.4.31 in /frontend Bumps [postcss](https://github.com/postcss/postcss) from 8.4.23 to 8.4.31. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](https://github.com/postcss/postcss/compare/8.4.23...8.4.31) --- updated-dependencies: - dependency-name: postcss dependency-type: indirect ... Signed-off-by: dependabot[bot] --- frontend/package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 6fac01dca4..39c7cf06d4 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -25317,9 +25317,9 @@ } }, "node_modules/postcss": { - "version": "8.4.23", - "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.23.tgz", - "integrity": "sha512-bQ3qMcpF6A/YjR55xtoTr0jGOlnPOKAIMdOWiv0EIT6HVPEaJiJB4NLljSbiHoC2RX7DN5Uvjtpbg1NPdwv1oA==", + "version": "8.4.31", + "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.31.tgz", + "integrity": "sha512-PS08Iboia9mts/2ygV3eLpY5ghnUcfLV/EXTOW1E2qYxJKGGBUtNjN76FYHnMs36RmARn41bC0AZmn+rR0OVpQ==", "devOptional": true, "funding": [ {