Skip to content

Commit d142fc4

Browse files
jeffspahrjeffspahr
authored
refactor(frontend): remove NewRunParameters mount effect (#13281)
* refactor(frontend): remove NewRunParameters mount effect Signed-off-by: Jeff Spahr <spahrj@gmail.com> * fix(frontend): remove unused NewRunParameters test import Signed-off-by: Jeff Spahr <spahrj@gmail.com> * test(frontend): cover literal parameter initial state Signed-off-by: Jeff Spahr <spahrj@gmail.com> --------- Signed-off-by: Jeff Spahr <spahrj@gmail.com>
1 parent 910fed3 commit d142fc4

2 files changed

Lines changed: 92 additions & 228 deletions

File tree

‎frontend/src/components/NewRunParametersV2.test.tsx‎

Lines changed: 91 additions & 206 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@
1616

1717
import { testBestPractices } from 'src/TestUtils';
1818
import { CommonTestWrapper } from 'src/TestWrapper';
19-
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
19+
import { fireEvent, render, screen } from '@testing-library/react';
2020
import { ParameterType_ParameterTypeEnum } from 'src/generated/pipeline_spec/pipeline_spec';
2121
import NewRunParametersV2 from 'src/components/NewRunParametersV2';
22+
import { getInitialParameterState } from 'src/lib/NewRunParametersUtils';
2223

2324
testBestPractices();
2425

@@ -90,6 +91,48 @@ describe('NewRunParametersV2', () => {
9091
expect(boolParam.closest('input').value).toEqual('false');
9192
});
9293

94+
it('computes valid initial state with default integer input', () => {
95+
const initialParameterState = getInitialParameterState({
96+
intParam: {
97+
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
98+
defaultValue: 123,
99+
},
100+
});
101+
102+
expect(initialParameterState.isValid).toBe(true);
103+
expect(initialParameterState.runtimeParameters).toEqual({ intParam: 123 });
104+
expect(initialParameterState.updatedParameters).toEqual({ intParam: '123' });
105+
expect(initialParameterState.errorMessages).toEqual({});
106+
});
107+
108+
it('computes invalid initial state with no default integer input', () => {
109+
const initialParameterState = getInitialParameterState({
110+
intParam: {
111+
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
112+
},
113+
});
114+
115+
expect(initialParameterState.isValid).toBe(false);
116+
expect(initialParameterState.runtimeParameters).toEqual({});
117+
expect(initialParameterState.updatedParameters).toEqual({});
118+
expect(initialParameterState.errorMessages).toEqual({ intParam: 'Missing parameter.' });
119+
});
120+
121+
it('computes invalid initial state for literal parameter with no default', () => {
122+
const initialParameterState = getInitialParameterState({
123+
env: {
124+
parameterType: ParameterType_ParameterTypeEnum.STRING,
125+
literals: ['dev', 'staging', 'prod'],
126+
isOptional: false,
127+
},
128+
});
129+
130+
expect(initialParameterState.isValid).toBe(false);
131+
expect(initialParameterState.runtimeParameters).toEqual({});
132+
expect(initialParameterState.updatedParameters).toEqual({});
133+
expect(initialParameterState.errorMessages).toEqual({ env: 'Missing parameter.' });
134+
});
135+
93136
it('call convertInput function for string type with default value', () => {
94137
const handleParameterChangeSpy = vi.fn();
95138
const props = {
@@ -512,48 +555,6 @@ describe('NewRunParametersV2', () => {
512555
screen.getByDisplayValue('"A":1,"B":2');
513556
});
514557

515-
it('set input as valid type with valid default integer input', () => {
516-
const setIsValidInputSpy = vi.fn();
517-
const props = {
518-
titleMessage: 'default Title',
519-
pipelineRoot: 'default pipelineRoot',
520-
specParameters: {
521-
intParam: {
522-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
523-
defaultValue: 123,
524-
},
525-
},
526-
clonedRuntimeConfig: {},
527-
handlePipelineRootChange: vi.fn(),
528-
handleParameterChange: vi.fn(),
529-
setIsValidInput: setIsValidInputSpy,
530-
};
531-
render(<NewRunParametersV2 {...props} />);
532-
533-
expect(setIsValidInputSpy).toHaveBeenLastCalledWith(true);
534-
screen.getByDisplayValue('123');
535-
});
536-
537-
it('set input as invalid type with no default integer input', () => {
538-
const setIsValidInputSpy = vi.fn();
539-
const props = {
540-
titleMessage: 'default Title',
541-
pipelineRoot: 'default pipelineRoot',
542-
specParameters: {
543-
intParam: {
544-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
545-
},
546-
},
547-
clonedRuntimeConfig: {},
548-
handlePipelineRootChange: vi.fn(),
549-
handleParameterChange: vi.fn(),
550-
setIsValidInput: setIsValidInputSpy,
551-
};
552-
render(<NewRunParametersV2 {...props} />);
553-
554-
expect(setIsValidInputSpy).toHaveBeenLastCalledWith(false);
555-
});
556-
557558
it('show error message for invalid integer input', () => {
558559
const setIsValidInputSpy = vi.fn();
559560
const props = {
@@ -932,58 +933,6 @@ describe('NewRunParametersV2', () => {
932933
expect(screen.queryAllByRole('textbox')).toHaveLength(0);
933934
});
934935

935-
// Test for fix: Default parameters not displayed in Compare Runs
936-
// https://github.com/kubeflow/pipelines/issues/12536
937-
it('calls handleParameterChange with default values on mount', () => {
938-
const handleParameterChangeSpy = vi.fn();
939-
const props = {
940-
titleMessage: 'default Title',
941-
pipelineRoot: 'default pipelineRoot',
942-
specParameters: {
943-
strParam: {
944-
parameterType: ParameterType_ParameterTypeEnum.STRING,
945-
defaultValue: 'default string',
946-
},
947-
intParam: {
948-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
949-
defaultValue: 42,
950-
},
951-
boolParam: {
952-
parameterType: ParameterType_ParameterTypeEnum.BOOLEAN,
953-
defaultValue: true,
954-
},
955-
listParam: {
956-
parameterType: ParameterType_ParameterTypeEnum.LIST,
957-
defaultValue: [1, 2, 3],
958-
},
959-
structParam: {
960-
parameterType: ParameterType_ParameterTypeEnum.STRUCT,
961-
defaultValue: { key: 'value' },
962-
},
963-
},
964-
clonedRuntimeConfig: {},
965-
handlePipelineRootChange: vi.fn(),
966-
handleParameterChange: handleParameterChangeSpy,
967-
};
968-
render(<NewRunParametersV2 {...props} />);
969-
970-
// Verify that handleParameterChange was called on mount with all default values
971-
expect(handleParameterChangeSpy).toHaveBeenCalledWith({
972-
strParam: 'default string',
973-
intParam: 42,
974-
boolParam: true,
975-
listParam: [1, 2, 3],
976-
structParam: { key: 'value' },
977-
});
978-
979-
// Verify that the default values are displayed in the UI
980-
screen.getByDisplayValue('default string');
981-
screen.getByDisplayValue('42');
982-
screen.getByDisplayValue('true');
983-
screen.getByDisplayValue('[1,2,3]');
984-
screen.getByDisplayValue('{"key":"value"}');
985-
});
986-
987936
it('renders provided initial state without firing mount callbacks', () => {
988937
const handleParameterChangeSpy = vi.fn();
989938
const setIsValidInputSpy = vi.fn();
@@ -1017,47 +966,36 @@ describe('NewRunParametersV2', () => {
1017966
});
1018967

1019968
describe('Bug Fix: Default Parameters in Compare Runs (#12536)', () => {
1020-
it('SCENARIO 1: User creates run with ALL default parameters (no changes)', () => {
1021-
const handleParameterChangeSpy = vi.fn();
1022-
1023-
const props = {
1024-
titleMessage: 'Specify parameters required by the pipeline',
1025-
specParameters: {
1026-
string_param: {
1027-
parameterType: ParameterType_ParameterTypeEnum.STRING,
1028-
defaultValue: 'default_string_value',
1029-
},
1030-
integer_param: {
1031-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
1032-
defaultValue: 42,
1033-
},
1034-
boolean_param: {
1035-
parameterType: ParameterType_ParameterTypeEnum.BOOLEAN,
1036-
defaultValue: true,
1037-
},
1038-
float_param: {
1039-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_DOUBLE,
1040-
defaultValue: 3.14,
1041-
},
1042-
list_param: {
1043-
parameterType: ParameterType_ParameterTypeEnum.LIST,
1044-
defaultValue: [1, 2, 3],
1045-
},
1046-
struct_param: {
1047-
parameterType: ParameterType_ParameterTypeEnum.STRUCT,
1048-
defaultValue: { key: 'value', nested: { data: 123 } },
1049-
},
969+
it('returns all default parameters in the initial state', () => {
970+
const initialParameterState = getInitialParameterState({
971+
string_param: {
972+
parameterType: ParameterType_ParameterTypeEnum.STRING,
973+
defaultValue: 'default_string_value',
1050974
},
1051-
clonedRuntimeConfig: {},
1052-
handleParameterChange: handleParameterChangeSpy,
1053-
};
1054-
1055-
// User does NOT interact - just renders form
1056-
render(<NewRunParametersV2 {...props} />);
975+
integer_param: {
976+
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
977+
defaultValue: 42,
978+
},
979+
boolean_param: {
980+
parameterType: ParameterType_ParameterTypeEnum.BOOLEAN,
981+
defaultValue: true,
982+
},
983+
float_param: {
984+
parameterType: ParameterType_ParameterTypeEnum.NUMBER_DOUBLE,
985+
defaultValue: 3.14,
986+
},
987+
list_param: {
988+
parameterType: ParameterType_ParameterTypeEnum.LIST,
989+
defaultValue: [1, 2, 3],
990+
},
991+
struct_param: {
992+
parameterType: ParameterType_ParameterTypeEnum.STRUCT,
993+
defaultValue: { key: 'value', nested: { data: 123 } },
994+
},
995+
});
1057996

1058-
// KEY ASSERTION: handleParameterChange called on mount (not 0!)
1059-
// ALL default parameters sent to API
1060-
expect(handleParameterChangeSpy).toHaveBeenCalledWith({
997+
expect(initialParameterState.isValid).toBe(true);
998+
expect(initialParameterState.runtimeParameters).toEqual({
1061999
string_param: 'default_string_value',
10621000
integer_param: 42,
10631001
boolean_param: true,
@@ -1089,16 +1027,8 @@ describe('Bug Fix: Default Parameters in Compare Runs (#12536)', () => {
10891027
clonedRuntimeConfig: {},
10901028
handleParameterChange: handleParameterChangeSpy,
10911029
};
1092-
10931030
render(<NewRunParametersV2 {...props} />);
10941031

1095-
// On mount, all defaults sent
1096-
expect(handleParameterChangeSpy).toHaveBeenCalledWith({
1097-
param_a: 'default_a',
1098-
param_b: 100,
1099-
param_c: false,
1100-
});
1101-
11021032
// User changes only param_a
11031033
const paramAInput = screen.getByDisplayValue('default_a');
11041034
handleParameterChangeSpy.mockClear();
@@ -1113,36 +1043,26 @@ describe('Bug Fix: Default Parameters in Compare Runs (#12536)', () => {
11131043
});
11141044

11151045
it('SCENARIO 3: Falsy default values (0, false, empty list) are included', () => {
1116-
const handleParameterChangeSpy = vi.fn();
1117-
1118-
const props = {
1119-
titleMessage: 'Test falsy defaults',
1120-
specParameters: {
1121-
zero_param: {
1122-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
1123-
defaultValue: 0,
1124-
},
1125-
false_param: {
1126-
parameterType: ParameterType_ParameterTypeEnum.BOOLEAN,
1127-
defaultValue: false,
1128-
},
1129-
zero_float: {
1130-
parameterType: ParameterType_ParameterTypeEnum.NUMBER_DOUBLE,
1131-
defaultValue: 0.0,
1132-
},
1133-
empty_list: {
1134-
parameterType: ParameterType_ParameterTypeEnum.LIST,
1135-
defaultValue: [],
1136-
},
1046+
const initialParameterState = getInitialParameterState({
1047+
zero_param: {
1048+
parameterType: ParameterType_ParameterTypeEnum.NUMBER_INTEGER,
1049+
defaultValue: 0,
11371050
},
1138-
clonedRuntimeConfig: {},
1139-
handleParameterChange: handleParameterChangeSpy,
1140-
};
1141-
1142-
render(<NewRunParametersV2 {...props} />);
1051+
false_param: {
1052+
parameterType: ParameterType_ParameterTypeEnum.BOOLEAN,
1053+
defaultValue: false,
1054+
},
1055+
zero_float: {
1056+
parameterType: ParameterType_ParameterTypeEnum.NUMBER_DOUBLE,
1057+
defaultValue: 0.0,
1058+
},
1059+
empty_list: {
1060+
parameterType: ParameterType_ParameterTypeEnum.LIST,
1061+
defaultValue: [],
1062+
},
1063+
});
11431064

1144-
// CRITICAL: Falsy values NOT omitted
1145-
expect(handleParameterChangeSpy).toHaveBeenCalledWith({
1065+
expect(initialParameterState.runtimeParameters).toEqual({
11461066
zero_param: 0,
11471067
false_param: false,
11481068
zero_float: 0.0,
@@ -1276,9 +1196,6 @@ describe('Literal Parameter Dropdown (#12603)', () => {
12761196
};
12771197
render(<NewRunParametersV2 {...props} />);
12781198

1279-
// On mount, default is propagated
1280-
expect(handleParameterChangeSpy).toHaveBeenCalledWith({ env: 'dev' });
1281-
12821199
// Open dropdown and select a different value
12831200
const selectButton = screen.getByText('dev');
12841201
fireEvent.mouseDown(selectButton);
@@ -1289,30 +1206,7 @@ describe('Literal Parameter Dropdown (#12603)', () => {
12891206
expect(handleParameterChangeSpy).toHaveBeenCalledWith({ env: 'staging' });
12901207
});
12911208

1292-
it('marks input as invalid when literal parameter has no default value', () => {
1293-
const setIsValidInputSpy = vi.fn();
1294-
const props = {
1295-
titleMessage: 'default Title',
1296-
specParameters: {
1297-
env: {
1298-
parameterType: ParameterType_ParameterTypeEnum.STRING,
1299-
literals: ['dev', 'staging', 'prod'],
1300-
isOptional: false,
1301-
description: '',
1302-
},
1303-
},
1304-
clonedRuntimeConfig: {},
1305-
handleParameterChange: vi.fn(),
1306-
setIsValidInput: setIsValidInputSpy,
1307-
};
1308-
render(<NewRunParametersV2 {...props} />);
1309-
1310-
// Without a default value, the form should be invalid (run button disabled)
1311-
expect(setIsValidInputSpy).toHaveBeenCalledWith(false);
1312-
});
1313-
13141209
it('pre-selects the correct value from cloned RuntimeConfig for literal parameter', () => {
1315-
const handleParameterChangeSpy = vi.fn();
13161210
const props = {
13171211
titleMessage: 'default Title',
13181212
specParameters: {
@@ -1325,17 +1219,14 @@ describe('Literal Parameter Dropdown (#12603)', () => {
13251219
},
13261220
},
13271221
clonedRuntimeConfig: { parameters: { env: 'prod' } },
1328-
handleParameterChange: handleParameterChangeSpy,
1222+
handleParameterChange: vi.fn(),
13291223
setIsValidInput: vi.fn(),
13301224
};
13311225
render(<NewRunParametersV2 {...props} />);
13321226

13331227
// The cloned value 'prod' should be displayed, not the default 'dev'
13341228
screen.getByText('prod');
13351229
screen.getByDisplayValue('prod');
1336-
1337-
// handleParameterChange should be called with the cloned value
1338-
expect(handleParameterChangeSpy).toHaveBeenCalledWith({ env: 'prod' });
13391230
});
13401231

13411232
it('marks input as valid after selecting a value from a no-default literal dropdown', () => {
@@ -1357,9 +1248,6 @@ describe('Literal Parameter Dropdown (#12603)', () => {
13571248
};
13581249
render(<NewRunParametersV2 {...props} />);
13591250

1360-
// Initially invalid
1361-
expect(setIsValidInputSpy).toHaveBeenCalledWith(false);
1362-
13631251
// Open dropdown - MUI v5 Select renders with role="combobox"
13641252
const selectElement = screen.getByRole('combobox');
13651253
fireEvent.mouseDown(selectElement);
@@ -1391,9 +1279,6 @@ describe('Literal Parameter Dropdown (#12603)', () => {
13911279
};
13921280
render(<NewRunParametersV2 {...props} />);
13931281

1394-
// Initially invalid (no default value)
1395-
expect(setIsValidInputSpy).toHaveBeenCalledWith(false);
1396-
13971282
// Open dropdown - MUI v5 Select renders with role="combobox"
13981283
const selectElement = screen.getByRole('combobox');
13991284
fireEvent.mouseDown(selectElement);

0 commit comments

Comments
 (0)