From 19d575cc307bd50c33cfeb1515ea830b5d22b985 Mon Sep 17 00:00:00 2001 From: Oleg Ivaniv Date: Fri, 6 Dec 2024 17:07:19 +0100 Subject: [PATCH] Improve unit tests & fix tag adding --- .../editor-ui/src/api/testDefinition.ee.ts | 12 +- .../EditDefinition/EvaluationHeader.vue | 15 +- .../EditDefinition/TagsInput.vue | 37 +-- .../composables/useTestDefinitionForm.ts | 174 +++++++------ .../TestDefinition/tests/MetricsInput.test.ts | 107 +++++++- .../tests/useTestDefinitionForm.test.ts | 238 +++++++++++++++--- .../src/components/TestDefinition/types.ts | 20 ++ .../TestDefinition/TestDefinitionEditView.vue | 19 +- .../tests/TestDefinitionEditView.test.ts | 226 ++++++++++------- 9 files changed, 583 insertions(+), 265 deletions(-) diff --git a/packages/editor-ui/src/api/testDefinition.ee.ts b/packages/editor-ui/src/api/testDefinition.ee.ts index e9f2c13fde..b9d907d1a5 100644 --- a/packages/editor-ui/src/api/testDefinition.ee.ts +++ b/packages/editor-ui/src/api/testDefinition.ee.ts @@ -1,5 +1,6 @@ import type { IRestApiContext } from '@/Interface'; import { makeRestApiRequest } from '@/utils/apiUtils'; + export interface TestDefinitionRecord { id: string; name: string; @@ -9,7 +10,9 @@ export interface TestDefinitionRecord { description?: string | null; updatedAt?: string; createdAt?: string; + annotationTag: string | null; } + interface CreateTestDefinitionParams { name: string; workflowId: string; @@ -22,16 +25,17 @@ export interface UpdateTestDefinitionParams { annotationTagId?: string | null; description?: string | null; } + export interface UpdateTestResponse { createdAt: string; updatedAt: string; id: string; name: string; workflowId: string; - description: string | null; - annotationTag: string | null; - evaluationWorkflowId: string | null; - annotationTagId: string | null; + description?: string | null; + annotationTag?: string | null; + evaluationWorkflowId?: string | null; + annotationTagId?: string | null; } const endpoint = '/evaluation/test-definitions'; diff --git a/packages/editor-ui/src/components/TestDefinition/EditDefinition/EvaluationHeader.vue b/packages/editor-ui/src/components/TestDefinition/EditDefinition/EvaluationHeader.vue index 3f21e6a399..58b8e7dc0f 100644 --- a/packages/editor-ui/src/components/TestDefinition/EditDefinition/EvaluationHeader.vue +++ b/packages/editor-ui/src/components/TestDefinition/EditDefinition/EvaluationHeader.vue @@ -1,18 +1,15 @@ @@ -50,12 +56,13 @@ function updateTags(tags: string[]) { :bold="false" size="small" > +
- + {{ locale.baseText('testDefinition.edit.selectTag') }}
+ + ; @@ -32,22 +12,21 @@ type FormRefs = { }; export function useTestDefinitionForm() { - // Stores const evaluationsStore = useTestDefinitionStore(); - const tagsStore = useAnnotationTagsStore(); - // Form state - const state = ref({ - description: '', + // State initialization + const state = ref({ name: { value: `My Test ${evaluationsStore.allTestDefinitions.length + 1}`, - isEditing: false, tempValue: '', + isEditing: false, }, tags: { + value: [], + tempValue: [], isEditing: false, - appliedTagIds: [], }, + description: '', evaluationWorkflow: { mode: 'list', value: '', @@ -56,45 +35,48 @@ export function useTestDefinitionForm() { metrics: [], }); - // Loading states const isSaving = ref(false); const fieldsIssues = ref>([]); - - // Field refs const fields = ref({} as FormRefs); - const tagIdToITag = (tagId: string) => { - return tagsStore.tagsById[tagId]; - }; - // Methods + // A computed mapping of editable fields to their states + // This ensures TS knows the exact type of each field. + const editableFields: ComputedRef<{ + name: EditableField; + tags: EditableField; + }> = computed(() => ({ + name: state.value.name, + tags: state.value.tags, + })); + + /** + * Load test data including metrics. + */ const loadTestData = async (testId: string) => { try { await evaluationsStore.fetchAll({ force: true }); const testDefinition = evaluationsStore.testDefinitionsById[testId]; if (testDefinition) { - // Fetch metrics for this test definition const metrics = await evaluationsStore.fetchMetrics(testId); - console.log('Loaded metrics:', metrics); - state.value = { - description: testDefinition.description ?? '', - name: { - value: testDefinition.name ?? '', - isEditing: false, - tempValue: '', - }, - tags: { - isEditing: false, - appliedTagIds: testDefinition.annotationTagId ? [testDefinition.annotationTagId] : [], - }, - evaluationWorkflow: { - mode: 'list', - value: testDefinition.evaluationWorkflowId ?? '', - __rl: true, - }, - metrics, // Use the fetched metrics + state.value.description = testDefinition.description ?? ''; + state.value.name = { + value: testDefinition.name ?? '', + isEditing: false, + tempValue: '', }; + state.value.tags = { + isEditing: false, + value: testDefinition.annotationTagId ? [testDefinition.annotationTagId] : [], + tempValue: [], + }; + state.value.evaluationWorkflow = { + mode: 'list', + value: testDefinition.evaluationWorkflowId ?? '', + __rl: true, + }; + state.value.metrics = metrics; } } catch (error) { console.error('Failed to load test data', error); @@ -108,17 +90,12 @@ export function useTestDefinitionForm() { fieldsIssues.value = []; try { - // Prepare parameters for creating a new test const params = { name: state.value.name.value, workflowId, description: state.value.description, }; - - const newTest = await evaluationsStore.create(params); - return newTest; - } catch (error) { - throw error; + return await evaluationsStore.create(params); } finally { isSaving.value = false; } @@ -130,9 +107,8 @@ export function useTestDefinitionForm() { }; const updateMetrics = async (testId: string) => { - const updatePromises = state.value.metrics.map(async (metric) => { + const promises = state.value.metrics.map(async (metric) => { if (!metric.name) return; - if (!metric.id) { const createdMetric = await evaluationsStore.createMetric({ name: metric.name, @@ -148,7 +124,7 @@ export function useTestDefinitionForm() { } }); - await Promise.all(updatePromises); + await Promise.all(promises); }; const updateTest = async (testId: string) => { @@ -158,64 +134,82 @@ export function useTestDefinitionForm() { fieldsIssues.value = []; try { - // Check if the test ID is provided if (!testId) { throw new Error('Test ID is required for updating a test'); } - // Prepare parameters for updating the existing test const params: UpdateTestDefinitionParams = { name: state.value.name.value, description: state.value.description, }; + if (state.value.evaluationWorkflow.value) { params.evaluationWorkflowId = state.value.evaluationWorkflow.value.toString(); } - const annotationTagId = state.value.tags?.[0]?.id; + const annotationTagId = state.value.tags.value[0]; if (annotationTagId) { params.annotationTagId = annotationTagId; } - // Update the existing test - const updatedTest = await evaluationsStore.update({ ...params, id: testId }); - return updatedTest; - } catch (error) { - throw error; + return await evaluationsStore.update({ ...params, id: testId }); } finally { isSaving.value = false; } }; - const startEditing = async (field: string) => { - if (field === 'name') { - state.value.name.tempValue = state.value.name.value; - state.value.name.isEditing = true; + /** + * Start editing an editable field by copying `value` to `tempValue`. + */ + function startEditing(field: T) { + const fieldObj = editableFields.value[field]; + if (fieldObj.isEditing) { + // Already editing, do nothing + return; } - }; - const saveChanges = (field: string) => { - if (field === 'name') { - state.value.name.value = state.value.name.tempValue; - state.value.name.isEditing = false; + if (Array.isArray(fieldObj.value)) { + fieldObj.tempValue = [...fieldObj.value]; + } else { + fieldObj.tempValue = fieldObj.value; } - }; + fieldObj.isEditing = true; + } + /** + * Save changes by copying `tempValue` back into `value`. + */ + function saveChanges(field: T) { + const fieldObj = editableFields.value[field]; + fieldObj.value = Array.isArray(fieldObj.tempValue) + ? [...fieldObj.tempValue] + : fieldObj.tempValue; + fieldObj.isEditing = false; + } - const cancelEditing = (field: string) => { - if (field === 'name') { - state.value.name.isEditing = false; - state.value.name.tempValue = ''; + /** + * Cancel editing and revert `tempValue` from `value`. + */ + function cancelEditing(field: T) { + const fieldObj = editableFields.value[field]; + if (Array.isArray(fieldObj.value)) { + fieldObj.tempValue = [...fieldObj.value]; + } else { + fieldObj.tempValue = fieldObj.value; } - }; + fieldObj.isEditing = false; + } - const handleKeydown = (event: KeyboardEvent, field: string) => { + /** + * Handle keyboard events during editing. + */ + function handleKeydown(event: KeyboardEvent, field: T) { if (event.key === 'Escape') { cancelEditing(field); } else if (event.key === 'Enter' && !event.shiftKey) { event.preventDefault(); saveChanges(field); } - }; + } return { state, diff --git a/packages/editor-ui/src/components/TestDefinition/tests/MetricsInput.test.ts b/packages/editor-ui/src/components/TestDefinition/tests/MetricsInput.test.ts index 43b2164521..c8eea1e3af 100644 --- a/packages/editor-ui/src/components/TestDefinition/tests/MetricsInput.test.ts +++ b/packages/editor-ui/src/components/TestDefinition/tests/MetricsInput.test.ts @@ -6,11 +6,11 @@ import userEvent from '@testing-library/user-event'; const renderComponent = createComponentRenderer(MetricsInput); describe('MetricsInput', () => { - let props: { modelValue: string[] }; + let props: { modelValue: Array<{ name: string }> }; beforeEach(() => { props = { - modelValue: ['Metric 1', 'Metric 2'], + modelValue: [{ name: 'Metric 1' }, { name: 'Metric 2' }], }; }); @@ -25,14 +25,18 @@ describe('MetricsInput', () => { it('should update a metric when typing in the input', async () => { const { getAllByPlaceholderText, emitted } = renderComponent({ props: { - modelValue: [''], + modelValue: [{ name: '' }], }, }); const inputs = getAllByPlaceholderText('Enter metric name'); await userEvent.type(inputs[0], 'Updated Metric 1'); - expect(emitted('update:modelValue')).toBeTruthy(); - expect(emitted('update:modelValue')).toEqual('Updated Metric 1'.split('').map((c) => [[c]])); + // Every character typed triggers an update event. Let's check the last emission. + const allEmits = emitted('update:modelValue'); + expect(allEmits).toBeTruthy(); + // The last emission should contain the fully updated name + const lastEmission = allEmits[allEmits.length - 1]; + expect(lastEmission).toEqual([[{ name: 'Updated Metric 1' }]]); }); it('should render correctly with no initial metrics', () => { @@ -47,10 +51,95 @@ describe('MetricsInput', () => { const { getByText, emitted } = renderComponent({ props }); const addButton = getByText('New metric'); - addButton.click(); - addButton.click(); - addButton.click(); + await userEvent.click(addButton); + await userEvent.click(addButton); + await userEvent.click(addButton); - expect(emitted('update:modelValue')).toHaveProperty('length', 3); + // Each click adds a new metric + const updateEvents = emitted('update:modelValue'); + expect(updateEvents).toHaveLength(3); + + // Check the structure of one of the emissions + // Initial: [{ name: 'Metric 1' }, { name: 'Metric 2' }] + // After first click: [{ name: 'Metric 1' }, { name: 'Metric 2' }, { name: '' }] + expect(updateEvents[0]).toEqual([[{ name: 'Metric 1' }, { name: 'Metric 2' }, { name: '' }]]); + }); + + it('should emit "deleteMetric" event when a delete button is clicked', async () => { + const { getAllByRole, emitted } = renderComponent({ props }); + + // Each metric row has a delete button, identified by "button" + const deleteButtons = getAllByRole('button', { name: '' }); + // Since these are icon buttons, if you have trouble querying by role/name, + // consider adding a test-id or using `getAllByTestId('evaluation-metric-item')` + // and navigate to its sibling button. + + expect(deleteButtons).toHaveLength(props.modelValue.length); + + // Click on the delete button for the second metric + await userEvent.click(deleteButtons[1]); + + expect(emitted('deleteMetric')).toBeTruthy(); + expect(emitted('deleteMetric')[0]).toEqual([{ name: 'Metric 2' }]); + }); + + it('should emit multiple update events as the user types and reflect the final name correctly', async () => { + const { getAllByPlaceholderText, emitted } = renderComponent({ + props: { + modelValue: [{ name: '' }], + }, + }); + const inputs = getAllByPlaceholderText('Enter metric name'); + await userEvent.type(inputs[0], 'ABC'); + + const allEmits = emitted('update:modelValue'); + expect(allEmits).toBeTruthy(); + // Each character typed should emit a new value + expect(allEmits.length).toBe(3); + expect(allEmits[2]).toEqual([[{ name: 'ABC' }]]); + }); + + it('should not break if metrics are empty and still allow adding a new metric', async () => { + props.modelValue = []; + const { queryAllByRole, getByText, emitted } = renderComponent({ props }); + + // No metrics initially + const inputs = queryAllByRole('textbox'); + expect(inputs).toHaveLength(0); + + const addButton = getByText('New metric'); + await userEvent.click(addButton); + + const updates = emitted('update:modelValue'); + expect(updates).toBeTruthy(); + expect(updates[0]).toEqual([[{ name: '' }]]); + + // After adding one metric, we should now have an input + const { getAllByPlaceholderText } = renderComponent({ + props: { modelValue: [{ name: '' }] }, + }); + const updatedInputs = getAllByPlaceholderText('Enter metric name'); + expect(updatedInputs).toHaveLength(1); + }); + + it('should handle deleting the first metric and still display remaining metrics correctly', async () => { + const { getAllByPlaceholderText, getAllByRole, rerender, emitted } = renderComponent({ + props, + }); + const inputs = getAllByPlaceholderText('Enter metric name'); + expect(inputs).toHaveLength(2); + + const deleteButtons = getAllByRole('button', { name: '' }); + await userEvent.click(deleteButtons[0]); + + // Verify the "deleteMetric" event + expect(emitted('deleteMetric')).toBeTruthy(); + expect(emitted('deleteMetric')[0]).toEqual([{ name: 'Metric 1' }]); + + // Re-render with the updated props (simulating parent update) + await rerender({ modelValue: [{ name: 'Metric 2' }] }); + const updatedInputs = getAllByPlaceholderText('Enter metric name'); + expect(updatedInputs).toHaveLength(1); + expect(updatedInputs[0]).toHaveValue('Metric 2'); }); }); diff --git a/packages/editor-ui/src/components/TestDefinition/tests/useTestDefinitionForm.test.ts b/packages/editor-ui/src/components/TestDefinition/tests/useTestDefinitionForm.test.ts index 952807d7cc..693a582cb4 100644 --- a/packages/editor-ui/src/components/TestDefinition/tests/useTestDefinitionForm.test.ts +++ b/packages/editor-ui/src/components/TestDefinition/tests/useTestDefinitionForm.test.ts @@ -12,18 +12,21 @@ const TEST_DEF_A: TestDefinitionRecord = { evaluationWorkflowId: '456', workflowId: '123', annotationTagId: '789', + annotationTag: null, }; const TEST_DEF_B: TestDefinitionRecord = { id: '2', name: 'Test Definition B', workflowId: '123', description: 'Description B', + annotationTag: null, }; const TEST_DEF_NEW: TestDefinitionRecord = { id: '3', workflowId: '123', name: 'New Test Definition', description: 'New Description', + annotationTag: null, }; beforeEach(() => { @@ -35,44 +38,79 @@ afterEach(() => { vi.clearAllMocks(); }); -describe('useTestDefinitionForm', async () => { - it('should initialize with default props', async () => { +describe('useTestDefinitionForm', () => { + it('should initialize with default props', () => { const { state } = useTestDefinitionForm(); - expect(state.value.description).toEqual(''); + expect(state.value.description).toBe(''); expect(state.value.name.value).toContain('My Test'); - expect(state.value.tags.appliedTagIds).toEqual([]); - expect(state.value.metrics).toEqual(['']); - expect(state.value.evaluationWorkflow.value).toEqual(''); + expect(state.value.tags.value).toEqual([]); + expect(state.value.metrics).toEqual([]); + expect(state.value.evaluationWorkflow.value).toBe(''); }); it('should load test data', async () => { const { loadTestData, state } = useTestDefinitionForm(); - const fetchSpy = vi.fn(); + const fetchSpy = vi.spyOn(useTestDefinitionStore(), 'fetchAll'); + const fetchMetricsSpy = vi.spyOn(useTestDefinitionStore(), 'fetchMetrics').mockResolvedValue([ + { + id: 'metric1', + name: 'Metric 1', + testDefinitionId: TEST_DEF_A.id, + }, + ]); const evaluationsStore = mockedStore(useTestDefinitionStore); - expect(state.value.description).toEqual(''); - expect(state.value.name.value).toContain('My Test'); evaluationsStore.testDefinitionsById = { [TEST_DEF_A.id]: TEST_DEF_A, [TEST_DEF_B.id]: TEST_DEF_B, }; - evaluationsStore.fetchAll = fetchSpy; await loadTestData(TEST_DEF_A.id); expect(fetchSpy).toBeCalled(); + expect(fetchMetricsSpy).toBeCalledWith(TEST_DEF_A.id); expect(state.value.name.value).toEqual(TEST_DEF_A.name); expect(state.value.description).toEqual(TEST_DEF_A.description); - expect(state.value.tags.appliedTagIds).toEqual([TEST_DEF_A.annotationTagId]); + expect(state.value.tags.value).toEqual([TEST_DEF_A.annotationTagId]); expect(state.value.evaluationWorkflow.value).toEqual(TEST_DEF_A.evaluationWorkflowId); + expect(state.value.metrics).toEqual([ + { id: 'metric1', name: 'Metric 1', testDefinitionId: TEST_DEF_A.id }, + ]); + }); + + it('should gracefully handle loadTestData when no test definition found', async () => { + const { loadTestData, state } = useTestDefinitionForm(); + const fetchSpy = vi.spyOn(useTestDefinitionStore(), 'fetchAll'); + const evaluationsStore = mockedStore(useTestDefinitionStore); + + evaluationsStore.testDefinitionsById = {}; + + await loadTestData('unknown-id'); + expect(fetchSpy).toBeCalled(); + // Should remain unchanged since no definition found + expect(state.value.description).toBe(''); + expect(state.value.name.value).toContain('My Test'); + expect(state.value.tags.value).toEqual([]); + expect(state.value.metrics).toEqual([]); + }); + + it('should handle errors while loading test data', async () => { + const { loadTestData } = useTestDefinitionForm(); + // const fetchSpy = vi.fn().mockRejectedValue(new Error('Fetch Failed')); + const fetchSpy = vi + .spyOn(useTestDefinitionStore(), 'fetchAll') + .mockRejectedValue(new Error('Fetch Failed')); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + await loadTestData(TEST_DEF_A.id); + expect(fetchSpy).toBeCalled(); + expect(consoleErrorSpy).toBeCalledWith('Failed to load test data', expect.any(Error)); + consoleErrorSpy.mockRestore(); }); it('should save a new test', async () => { const { createTest, state } = useTestDefinitionForm(); - const createSpy = vi.fn().mockResolvedValue(TEST_DEF_NEW); - const evaluationsStore = mockedStore(useTestDefinitionStore); - - evaluationsStore.create = createSpy; + const createSpy = vi.spyOn(useTestDefinitionStore(), 'create').mockResolvedValue(TEST_DEF_NEW); state.value.name.value = TEST_DEF_NEW.name; state.value.description = TEST_DEF_NEW.description ?? ''; @@ -86,12 +124,24 @@ describe('useTestDefinitionForm', async () => { expect(newTest).toEqual(TEST_DEF_NEW); }); + it('should handle errors when creating a new test', async () => { + const { createTest } = useTestDefinitionForm(); + const createSpy = vi + .spyOn(useTestDefinitionStore(), 'create') + .mockRejectedValue(new Error('Create Failed')); + + await expect(createTest('123')).rejects.toThrow('Create Failed'); + expect(createSpy).toBeCalled(); + }); + it('should update an existing test', async () => { const { updateTest, state } = useTestDefinitionForm(); - const updateSpy = vi.fn().mockResolvedValue(TEST_DEF_B); - const evaluationsStore = mockedStore(useTestDefinitionStore); - - evaluationsStore.update = updateSpy; + const updatedBTest = { + ...TEST_DEF_B, + updatedAt: '2022-01-01T00:00:00.000Z', + createdAt: '2022-01-01T00:00:00.000Z', + }; + const updateSpy = vi.spyOn(useTestDefinitionStore(), 'update').mockResolvedValue(updatedBTest); state.value.name.value = TEST_DEF_B.name; state.value.description = TEST_DEF_B.description ?? ''; @@ -102,75 +152,183 @@ describe('useTestDefinitionForm', async () => { name: TEST_DEF_B.name, description: TEST_DEF_B.description, }); - expect(updatedTest).toEqual(TEST_DEF_B); + expect(updatedTest).toEqual(updatedBTest); }); - it('should start editing a field', async () => { + it('should throw an error if no testId is provided when updating a test', async () => { + const { updateTest } = useTestDefinitionForm(); + await expect(updateTest('')).rejects.toThrow('Test ID is required for updating a test'); + }); + + it('should handle errors when updating a test', async () => { + const { updateTest, state } = useTestDefinitionForm(); + const updateSpy = vi + .spyOn(useTestDefinitionStore(), 'update') + .mockRejectedValue(new Error('Update Failed')); + + state.value.name.value = 'Test'; + state.value.description = 'Some description'; + + await expect(updateTest(TEST_DEF_A.id)).rejects.toThrow('Update Failed'); + expect(updateSpy).toBeCalled(); + }); + + it('should delete a metric', async () => { + const { state, deleteMetric } = useTestDefinitionForm(); + const evaluationsStore = mockedStore(useTestDefinitionStore); + const deleteMetricSpy = vi.spyOn(evaluationsStore, 'deleteMetric'); + + state.value.metrics = [ + { + id: 'metric1', + name: 'Metric 1', + testDefinitionId: '1', + }, + { + id: 'metric2', + name: 'Metric 2', + testDefinitionId: '1', + }, + ]; + + await deleteMetric('metric1', TEST_DEF_A.id); + expect(deleteMetricSpy).toBeCalledWith({ id: 'metric1', testDefinitionId: TEST_DEF_A.id }); + expect(state.value.metrics).toEqual([ + { id: 'metric2', name: 'Metric 2', testDefinitionId: '1' }, + ]); + }); + + it('should update metrics', async () => { + const { state, updateMetrics } = useTestDefinitionForm(); + const evaluationsStore = mockedStore(useTestDefinitionStore); + const updateMetricSpy = vi.spyOn(evaluationsStore, 'updateMetric'); + const createMetricSpy = vi + .spyOn(evaluationsStore, 'createMetric') + .mockResolvedValue({ id: 'metric_new', name: 'Metric 2', testDefinitionId: TEST_DEF_A.id }); + + state.value.metrics = [ + { + id: 'metric1', + name: 'Metric 1', + testDefinitionId: TEST_DEF_A.id, + }, + { + id: '', + name: 'Metric 2', + testDefinitionId: TEST_DEF_A.id, + }, // New metric that needs creation + ]; + + await updateMetrics(TEST_DEF_A.id); + expect(createMetricSpy).toHaveBeenCalledWith({ + name: 'Metric 2', + testDefinitionId: TEST_DEF_A.id, + }); + expect(updateMetricSpy).toHaveBeenCalledWith({ + name: 'Metric 1', + id: 'metric1', + testDefinitionId: TEST_DEF_A.id, + }); + expect(state.value.metrics).toEqual([ + { id: 'metric1', name: 'Metric 1', testDefinitionId: TEST_DEF_A.id }, + { id: 'metric_new', name: 'Metric 2', testDefinitionId: TEST_DEF_A.id }, + ]); + }); + + it('should start editing a field', () => { const { state, startEditing } = useTestDefinitionForm(); - await startEditing('name'); + startEditing('name'); expect(state.value.name.isEditing).toBe(true); expect(state.value.name.tempValue).toBe(state.value.name.value); - await startEditing('tags'); + startEditing('tags'); expect(state.value.tags.isEditing).toBe(true); + expect(state.value.tags.tempValue).toEqual(state.value.tags.value); }); - it('should save changes to a field', async () => { + it('should do nothing if startEditing is called while already editing', () => { + const { state, startEditing } = useTestDefinitionForm(); + state.value.name.isEditing = true; + state.value.name.tempValue = 'Original Name'; + + startEditing('name'); + // Should remain unchanged because it was already editing + expect(state.value.name.isEditing).toBe(true); + expect(state.value.name.tempValue).toBe('Original Name'); + }); + + it('should save changes to a field', () => { const { state, startEditing, saveChanges } = useTestDefinitionForm(); - await startEditing('name'); + // Name + startEditing('name'); state.value.name.tempValue = 'New Name'; saveChanges('name'); expect(state.value.name.isEditing).toBe(false); expect(state.value.name.value).toBe('New Name'); - await startEditing('tags'); - state.value.tags.appliedTagIds = ['123']; + // Tags + startEditing('tags'); + state.value.tags.tempValue = ['123']; saveChanges('tags'); expect(state.value.tags.isEditing).toBe(false); - expect(state.value.tags.appliedTagIds).toEqual(['123']); + expect(state.value.tags.value).toEqual(['123']); }); - it('should cancel editing a field', async () => { + it('should cancel editing a field', () => { const { state, startEditing, cancelEditing } = useTestDefinitionForm(); - await startEditing('name'); + const originalName = state.value.name.value; + startEditing('name'); state.value.name.tempValue = 'New Name'; cancelEditing('name'); expect(state.value.name.isEditing).toBe(false); - expect(state.value.name.tempValue).toBe(''); + expect(state.value.name.tempValue).toBe(originalName); - await startEditing('tags'); - state.value.tags.appliedTagIds = ['123']; + const originalTags = [...state.value.tags.value]; + startEditing('tags'); + state.value.tags.tempValue = ['123']; cancelEditing('tags'); expect(state.value.tags.isEditing).toBe(false); + expect(state.value.tags.tempValue).toEqual(originalTags); }); - it('should handle keydown - Escape', async () => { + it('should handle keydown - Escape', () => { const { state, startEditing, handleKeydown } = useTestDefinitionForm(); - await startEditing('name'); + startEditing('name'); handleKeydown(new KeyboardEvent('keydown', { key: 'Escape' }), 'name'); expect(state.value.name.isEditing).toBe(false); - await startEditing('tags'); + startEditing('tags'); handleKeydown(new KeyboardEvent('keydown', { key: 'Escape' }), 'tags'); expect(state.value.tags.isEditing).toBe(false); }); - it('should handle keydown - Enter', async () => { + it('should handle keydown - Enter', () => { const { state, startEditing, handleKeydown } = useTestDefinitionForm(); - await startEditing('name'); + startEditing('name'); state.value.name.tempValue = 'New Name'; handleKeydown(new KeyboardEvent('keydown', { key: 'Enter' }), 'name'); expect(state.value.name.isEditing).toBe(false); expect(state.value.name.value).toBe('New Name'); - await startEditing('tags'); - state.value.tags.appliedTagIds = ['123']; + startEditing('tags'); + state.value.tags.tempValue = ['123']; handleKeydown(new KeyboardEvent('keydown', { key: 'Enter' }), 'tags'); expect(state.value.tags.isEditing).toBe(false); + expect(state.value.tags.value).toEqual(['123']); + }); + + it('should not save changes when shift+Enter is pressed', () => { + const { state, startEditing, handleKeydown } = useTestDefinitionForm(); + + startEditing('name'); + state.value.name.tempValue = 'New Name With Shift'; + handleKeydown(new KeyboardEvent('keydown', { key: 'Enter', shiftKey: true }), 'name'); + expect(state.value.name.isEditing).toBe(true); + expect(state.value.name.value).not.toBe('New Name With Shift'); }); }); diff --git a/packages/editor-ui/src/components/TestDefinition/types.ts b/packages/editor-ui/src/components/TestDefinition/types.ts index 68a9d246a5..6e582e5bcb 100644 --- a/packages/editor-ui/src/components/TestDefinition/types.ts +++ b/packages/editor-ui/src/components/TestDefinition/types.ts @@ -1,3 +1,23 @@ +import type { TestMetricRecord } from '@/api/testDefinition.ee'; +import type { INodeParameterResourceLocator } from 'n8n-workflow'; + +export interface EditableField { + value: T; + tempValue: T; + isEditing: boolean; +} + +export interface EditableFormState { + name: EditableField; + tags: EditableField; +} + +export interface EvaluationFormState extends EditableFormState { + description: string; + evaluationWorkflow: INodeParameterResourceLocator; + metrics: TestMetricRecord[]; +} + export interface TestExecution { lastRun: string | null; errorRate: number | null; diff --git a/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue b/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue index 4ddade703e..583630541f 100644 --- a/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue +++ b/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue @@ -63,7 +63,8 @@ onMounted(async () => { await tagsStore.fetchAll(); if (testId.value) { await loadTestData(testId.value); - if (state.value.tags.appliedTagIds.length > 0) { + // Now tags are in state.tags.value instead of appliedTagIds + if (state.value.tags.value.length > 0) { await fetchSelectedExecutions(); } } else { @@ -105,23 +106,24 @@ async function onDeleteMetric(deletedMetric: Partial) { } async function fetchSelectedExecutions() { + // Use state.tags.value for the annotationTags const executionsForTags = await fetchExecutions({ - annotationTags: state.value.tags.appliedTagIds, + annotationTags: state.value.tags.value, }); matchedExecutions.value = executionsForTags.results; } -watch( - [() => state.value.name, () => state.value.description, () => state.value.evaluationWorkflow], - debounce(onSaveTest, { debounceTime: 400 }), - { deep: true }, -); +// Debounced watchers for auto-saving +watch([() => state.value.evaluationWorkflow], debounce(onSaveTest, { debounceTime: 400 }), { + deep: true, +}); watch( - [() => state.value.metrics], + () => state.value.metrics, debounce(async () => await updateMetrics(testId.value), { debounceTime: 400 }), { deep: true }, ); + async function handleCreateTag(tagName: string) { try { const newTag = await tagsStore.create(tagName); @@ -266,7 +268,6 @@ watch(() => state.value, debounce(onSaveTest, { debounceTime: 400 }), { deep: tr .panelBlock { max-width: var(--evaluation-edit-panel-width, 24rem); display: grid; - justify-items: end; } .panelIntro { diff --git a/packages/editor-ui/src/views/TestDefinition/tests/TestDefinitionEditView.test.ts b/packages/editor-ui/src/views/TestDefinition/tests/TestDefinitionEditView.test.ts index 9aeb70679b..c67776d09f 100644 --- a/packages/editor-ui/src/views/TestDefinition/tests/TestDefinitionEditView.test.ts +++ b/packages/editor-ui/src/views/TestDefinition/tests/TestDefinitionEditView.test.ts @@ -1,3 +1,4 @@ +import type { Mock } from 'vitest'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { createPinia, setActivePinia } from 'pinia'; import { createTestingPinia } from '@pinia/testing'; @@ -8,57 +9,73 @@ import { useToast } from '@/composables/useToast'; import { useTestDefinitionForm } from '@/components/TestDefinition/composables/useTestDefinitionForm'; import { useAnnotationTagsStore } from '@/stores/tags.store'; import { ref, nextTick } from 'vue'; +import { mockedStore } from '@/__tests__/utils'; +import { VIEWS } from '@/constants'; vi.mock('vue-router'); vi.mock('@/composables/useToast'); vi.mock('@/components/TestDefinition/composables/useTestDefinitionForm'); -vi.mock('@/stores/tags.store'); vi.mock('@/stores/projects.store'); describe('TestDefinitionEditView', () => { const renderComponent = createComponentRenderer(TestDefinitionEditView); + let createTestMock: Mock; + let updateTestMock: Mock; + let loadTestDataMock: Mock; + let deleteMetricMock: Mock; + let updateMetricsMock: Mock; + let showMessageMock: Mock; + let showErrorMock: Mock; + beforeEach(() => { setActivePinia(createPinia()); + // Default route mock: no testId vi.mocked(useRoute).mockReturnValue({ params: {}, - path: '/test-path', - name: 'test-route', + name: VIEWS.NEW_TEST_DEFINITION, } as ReturnType); vi.mocked(useRouter).mockReturnValue({ push: vi.fn(), + replace: vi.fn(), resolve: vi.fn().mockReturnValue({ href: '/test-href' }), } as unknown as ReturnType); + createTestMock = vi.fn().mockResolvedValue({ id: 'newTestId' }); + updateTestMock = vi.fn().mockResolvedValue({}); + loadTestDataMock = vi.fn(); + deleteMetricMock = vi.fn(); + updateMetricsMock = vi.fn(); + showMessageMock = vi.fn(); + showErrorMock = vi.fn(); + vi.mocked(useToast).mockReturnValue({ - showMessage: vi.fn(), - showError: vi.fn(), + showMessage: showMessageMock, + showError: showErrorMock, } as unknown as ReturnType); + vi.mocked(useTestDefinitionForm).mockReturnValue({ state: ref({ name: { value: '', isEditing: false, tempValue: '' }, description: '', - tags: { appliedTagIds: [], isEditing: false }, - evaluationWorkflow: { id: '1', name: 'Test Workflow' }, + tags: { value: [], tempValue: [], isEditing: false }, + evaluationWorkflow: { mode: 'list', value: '', __rl: true }, metrics: [], }), fieldsIssues: ref([]), isSaving: ref(false), - loadTestData: vi.fn(), - saveTest: vi.fn(), + loadTestData: loadTestDataMock, + createTest: createTestMock, + updateTest: updateTestMock, startEditing: vi.fn(), saveChanges: vi.fn(), cancelEditing: vi.fn(), handleKeydown: vi.fn(), + deleteMetric: deleteMetricMock, + updateMetrics: updateMetricsMock, } as unknown as ReturnType); - vi.mocked(useAnnotationTagsStore).mockReturnValue({ - isLoading: ref(false), - allTags: ref([]), - tagsById: ref({}), - fetchAll: vi.fn(), - } as unknown as ReturnType); vi.mock('@/stores/projects.store', () => ({ useProjectsStore: vi.fn().mockReturnValue({ @@ -76,133 +93,162 @@ describe('TestDefinitionEditView', () => { it('should load test data when testId is provided', async () => { vi.mocked(useRoute).mockReturnValue({ params: { testId: '1' }, - path: '/test-path', - name: 'test-route', + name: VIEWS.TEST_DEFINITION_EDIT, } as unknown as ReturnType); - const loadTestDataMock = vi.fn(); - vi.mocked(useTestDefinitionForm).mockReturnValue({ - ...vi.mocked(useTestDefinitionForm)(), - loadTestData: loadTestDataMock, - } as unknown as ReturnType); - renderComponent({ - pinia: createTestingPinia(), - }); + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + const { getByTestId } = renderComponent({ pinia }); await nextTick(); + expect(loadTestDataMock).toHaveBeenCalledWith('1'); }); it('should not load test data when testId is not provided', async () => { - const loadTestDataMock = vi.fn(); - vi.mocked(useTestDefinitionForm).mockReturnValue({ - ...vi.mocked(useTestDefinitionForm)(), - loadTestData: loadTestDataMock, - } as unknown as ReturnType); + // Here route returns no testId + vi.mocked(useRoute).mockReturnValue({ + params: {}, + name: VIEWS.NEW_TEST_DEFINITION, + } as unknown as ReturnType); - renderComponent({ - pinia: createTestingPinia(), - }); + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + renderComponent({ pinia }); await nextTick(); + expect(loadTestDataMock).not.toHaveBeenCalled(); }); - it('should save test and show success message on successful save', async () => { - const saveTestMock = vi.fn().mockResolvedValue({}); - const routerPushMock = vi.fn(); - const routerResolveMock = vi.fn().mockReturnValue({ href: '/test-href' }); - vi.mocked(useTestDefinitionForm).mockReturnValue({ - ...vi.mocked(useTestDefinitionForm)(), - createTest: saveTestMock, - } as unknown as ReturnType); + it('should create a new test and show success message on save if no testId is present', async () => { + vi.mocked(useRoute).mockReturnValue({ + params: {}, + name: VIEWS.NEW_TEST_DEFINITION, + } as ReturnType); - vi.mocked(useRouter).mockReturnValue({ - push: routerPushMock, - resolve: routerResolveMock, - } as unknown as ReturnType); + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); - const { getByTestId } = renderComponent({ - pinia: createTestingPinia(), - }); + const { getByTestId } = renderComponent({ pinia }); await nextTick(); const saveButton = getByTestId('run-test-button'); saveButton.click(); await nextTick(); - expect(saveTestMock).toHaveBeenCalled(); + expect(createTestMock).toHaveBeenCalled(); + expect(showMessageMock).toHaveBeenCalledWith({ + title: expect.any(String), + type: 'success', + }); }); - it('should show error message on failed save', async () => { - const saveTestMock = vi.fn().mockRejectedValue(new Error('Save failed')); - const showErrorMock = vi.fn(); - vi.mocked(useTestDefinitionForm).mockReturnValue({ - ...vi.mocked(useTestDefinitionForm)(), - createTest: saveTestMock, - } as unknown as ReturnType); - vi.mocked(useToast).mockReturnValue({ showError: showErrorMock } as unknown as ReturnType< - typeof useToast - >); + it('should update test and show success message on save if testId is present', async () => { + vi.mocked(useRoute).mockReturnValue({ + params: { testId: '1' }, + name: VIEWS.TEST_DEFINITION_EDIT, + } as unknown as ReturnType); - const { getByTestId } = renderComponent({ - pinia: createTestingPinia(), - }); + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + + const { getByTestId } = renderComponent({ pinia }); await nextTick(); const saveButton = getByTestId('run-test-button'); saveButton.click(); await nextTick(); - expect(saveTestMock).toHaveBeenCalled(); - expect(showErrorMock).toHaveBeenCalled(); + + expect(updateTestMock).toHaveBeenCalledWith('1'); + expect(showMessageMock).toHaveBeenCalledWith({ + title: expect.any(String), + type: 'success', + }); + }); + + it('should show error message on failed test creation', async () => { + createTestMock.mockRejectedValue(new Error('Save failed')); + + vi.mocked(useRoute).mockReturnValue({ + params: {}, + name: VIEWS.NEW_TEST_DEFINITION, + } as unknown as ReturnType); + + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + + const { getByTestId } = renderComponent({ pinia }); + await nextTick(); + const saveButton = getByTestId('run-test-button'); + saveButton.click(); + await nextTick(); + + expect(createTestMock).toHaveBeenCalled(); + expect(showErrorMock).toHaveBeenCalledWith(expect.any(Error), expect.any(String)); }); it('should display "Update Test" button when editing existing test', async () => { vi.mocked(useRoute).mockReturnValue({ params: { testId: '1' }, - path: '/test-path', - name: 'test-route', + name: VIEWS.TEST_DEFINITION_EDIT, } as unknown as ReturnType); - const { getByTestId } = renderComponent({ - pinia: createTestingPinia(), - }); + + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + + const { getByTestId } = renderComponent({ pinia }); await nextTick(); const updateButton = getByTestId('run-test-button'); - expect(updateButton.textContent).toContain('Update test'); + expect(updateButton.textContent?.toLowerCase()).toContain('update'); }); - it('should display "Run Test" button when creating new test', async () => { - const { getByTestId } = renderComponent({ - pinia: createTestingPinia(), - }); + it('should display "Save Test" button when creating new test', async () => { + vi.mocked(useRoute).mockReturnValue({ + params: {}, + name: VIEWS.NEW_TEST_DEFINITION, + } as unknown as ReturnType); + + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + + const { getByTestId } = renderComponent({ pinia }); await nextTick(); const saveButton = getByTestId('run-test-button'); - expect(saveButton).toBeTruthy(); + expect(saveButton.textContent?.toLowerCase()).toContain('run test'); }); it('should apply "has-issues" class to inputs with issues', async () => { vi.mocked(useTestDefinitionForm).mockReturnValue({ ...vi.mocked(useTestDefinitionForm)(), - fieldsIssues: ref([{ field: 'name' }, { field: 'tags' }]), + fieldsIssues: ref([ + { field: 'name', message: 'Name is required' }, + { field: 'tags', message: 'Tag is required' }, + ]), } as unknown as ReturnType); - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + + const { container } = renderComponent({ pinia }); await nextTick(); - expect(container.querySelector('.has-issues')).toBeTruthy(); + const issueElements = container.querySelectorAll('.has-issues'); + expect(issueElements.length).toBeGreaterThan(0); }); it('should fetch all tags on mount', async () => { - const fetchAllMock = vi.fn(); - vi.mocked(useAnnotationTagsStore).mockReturnValue({ - ...vi.mocked(useAnnotationTagsStore)(), - fetchAll: fetchAllMock, - } as unknown as ReturnType); - - renderComponent({ - pinia: createTestingPinia(), - }); + const pinia = createTestingPinia(); + setActivePinia(pinia); + mockedStore(useAnnotationTagsStore).fetchAll.mockResolvedValue([]); + renderComponent({ pinia }); await nextTick(); - expect(fetchAllMock).toHaveBeenCalled(); + expect(mockedStore(useAnnotationTagsStore).fetchAll).toHaveBeenCalled(); }); });