diff --git a/frontend/src/components/NewRunParametersV2.test.tsx b/frontend/src/components/NewRunParametersV2.test.tsx index 284650a05df..4a81f38efee 100644 --- a/frontend/src/components/NewRunParametersV2.test.tsx +++ b/frontend/src/components/NewRunParametersV2.test.tsx @@ -48,14 +48,14 @@ describe('NewRunParametersV2', () => { , ); - expect(screen.getByText('Run parameters')); - expect(screen.getByText('Specify parameters required by the pipeline')); - expect(screen.getByText('strParam - STRING')); - expect(screen.getByDisplayValue('string value')); - expect(screen.getByText('boolParam - BOOLEAN')); - expect(screen.getByDisplayValue('true')); - expect(screen.getByText('intParam - NUMBER_INTEGER')); - expect(screen.getByDisplayValue('123')); + screen.getByText('Run parameters'); + screen.getByText('Specify parameters required by the pipeline'); + screen.getByText('strParam - STRING'); + screen.getByDisplayValue('string value'); + screen.getByText('boolParam - BOOLEAN'); + screen.getByDisplayValue('true'); + screen.getByText('intParam - NUMBER_INTEGER'); + screen.getByDisplayValue('123'); }); it('edits parameters', () => { @@ -116,6 +116,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ strParam: 'new string', }); + screen.getByDisplayValue('new string'); }); it('test convertInput function for string type without default value', () => { @@ -131,7 +132,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const strParam = screen.getByLabelText('strParam - STRING'); fireEvent.change(strParam, { target: { value: 'new string' } }); @@ -139,6 +140,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ strParam: 'new string', }); + screen.getByDisplayValue('new string'); }); it('test convertInput function for boolean type with default value', () => { @@ -155,7 +157,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const boolParam = screen.getByDisplayValue('true'); fireEvent.change(boolParam, { target: { value: 'false' } }); @@ -163,6 +165,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ boolParam: false, }); + screen.getByDisplayValue('false'); }); it('test convertInput function for boolean type without default value', () => { @@ -178,7 +181,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const boolParam = screen.getByLabelText('boolParam - BOOLEAN'); fireEvent.change(boolParam, { target: { value: 'true' } }); @@ -186,6 +189,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ boolParam: true, }); + screen.getByDisplayValue('true'); }); it('test convertInput function for boolean type with invalid input (Uppercase)', () => { @@ -201,7 +205,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const boolParam = screen.getByLabelText('boolParam - BOOLEAN'); fireEvent.change(boolParam, { target: { value: 'True' } }); @@ -209,6 +213,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ boolParam: null, }); + screen.getByDisplayValue('True'); }); it('test convertInput function for integer type with default value', () => { @@ -225,7 +230,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const intParam = screen.getByDisplayValue('123'); fireEvent.change(intParam, { target: { value: '456' } }); @@ -233,6 +238,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ intParam: 456, }); + screen.getByDisplayValue('456'); }); it('test convertInput function for integer type without default value', () => { @@ -248,7 +254,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const intParam = screen.getByLabelText('intParam - NUMBER_INTEGER'); fireEvent.change(intParam, { target: { value: '789' } }); @@ -256,6 +262,7 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ intParam: 789, }); + screen.getByDisplayValue('789'); }); it('test convertInput function for integer type with invalid input (float)', () => { @@ -271,7 +278,7 @@ describe('NewRunParametersV2', () => { handlePipelineRootChange: jest.fn(), handleParameterChange: handleParameterChangeSpy, }; - render(); + render(); const intParam = screen.getByLabelText('intParam - NUMBER_INTEGER'); fireEvent.change(intParam, { target: { value: '7.89' } }); @@ -279,6 +286,202 @@ describe('NewRunParametersV2', () => { expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ intParam: null, }); + screen.getByDisplayValue('7.89'); + }); + + it('test convertInput function for double type with default value', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + doubleParam: { + parameterType: ParameterType_ParameterTypeEnum.NUMBER_DOUBLE, + defaultValue: 1.23, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const doubleParam = screen.getByDisplayValue('1.23'); + fireEvent.change(doubleParam, { target: { value: '4.56' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + doubleParam: 4.56, + }); + screen.getByDisplayValue('4.56'); + }); + + it('test convertInput function for double type without default value', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + doubleParam: { + parameterType: ParameterType_ParameterTypeEnum.NUMBER_DOUBLE, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const doubleParam = screen.getByLabelText('doubleParam - NUMBER_DOUBLE'); + fireEvent.change(doubleParam, { target: { value: '7.89' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + doubleParam: 7.89, + }); + screen.getByDisplayValue('7.89'); + }); + + it('test convertInput function for LIST type with default value', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + listParam: { + parameterType: ParameterType_ParameterTypeEnum.LIST, + defaultValue: [1, 2, 3], + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const listParam = screen.getByDisplayValue('[1,2,3]'); + fireEvent.change(listParam, { target: { value: '[4,5,6]' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + listParam: [4, 5, 6], + }); + screen.getByDisplayValue('[4,5,6]'); + }); + + it('test convertInput function for LIST type without default value', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + listParam: { + parameterType: ParameterType_ParameterTypeEnum.LIST, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const listParam = screen.getByLabelText('listParam - LIST'); + fireEvent.change(listParam, { target: { value: '[4,5,6]' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + listParam: [4, 5, 6], + }); + screen.getByDisplayValue('[4,5,6]'); + }); + + it('test convertInput function for LIST type with invalid input (invalid JSON form)', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + listParam: { + parameterType: ParameterType_ParameterTypeEnum.LIST, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const listParam = screen.getByLabelText('listParam - LIST'); + fireEvent.change(listParam, { target: { value: '[4,5,6' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + listParam: null, + }); + screen.getByDisplayValue('[4,5,6'); + }); + + it('test convertInput function for STRUCT type with default value', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + structParam: { + parameterType: ParameterType_ParameterTypeEnum.STRUCT, + defaultValue: { A: 1, B: 2 }, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const structParam = screen.getByDisplayValue('{"A":1,"B":2}'); + fireEvent.change(structParam, { target: { value: '{"C":3,"D":4}' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + structParam: { C: 3, D: 4 }, + }); + screen.getByDisplayValue('{"C":3,"D":4}'); + }); + + it('test convertInput function for STRUCT type without default value', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + structParam: { + parameterType: ParameterType_ParameterTypeEnum.STRUCT, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const structParam = screen.getByLabelText('structParam - STRUCT'); + fireEvent.change(structParam, { target: { value: '{"A":1,"B":2}' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + structParam: { A: 1, B: 2 }, + }); + screen.getByDisplayValue('{"A":1,"B":2}'); + }); + + it('test convertInput function for STRUCT type with invalid input (invalid JSON form)', () => { + const handleParameterChangeSpy = jest.fn(); + const props = { + titleMessage: 'default Title', + pipelineRoot: 'defalut pipelineRoot', + specParameters: { + structParam: { + parameterType: ParameterType_ParameterTypeEnum.STRUCT, + }, + }, + handlePipelineRootChange: jest.fn(), + handleParameterChange: handleParameterChangeSpy, + }; + render(); + + const structParam = screen.getByLabelText('structParam - STRUCT'); + fireEvent.change(structParam, { target: { value: '"A":1,"B":2' } }); + expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); + expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ + structParam: null, + }); + screen.getByDisplayValue('"A":1,"B":2'); }); it('does not display any text fields if there are no parameters', () => { diff --git a/frontend/src/components/NewRunParametersV2.tsx b/frontend/src/components/NewRunParametersV2.tsx index d388514dec4..58591b77f71 100644 --- a/frontend/src/components/NewRunParametersV2.tsx +++ b/frontend/src/components/NewRunParametersV2.tsx @@ -71,6 +71,18 @@ function convertInput(paramStr: string, paramType: ParameterType_ParameterTypeEn return Number(paramStr); } return null; + case ParameterType_ParameterTypeEnum.NUMBER_DOUBLE: + if (!Number.isNaN(Number(paramStr))) { + return Number(paramStr); + } + return null; + case ParameterType_ParameterTypeEnum.LIST: + case ParameterType_ParameterTypeEnum.STRUCT: + try { + return JSON.parse(paramStr); + } catch (err) { + return null; + } default: // TODO: (jlyaoyuli) Validate if the type of parameters matches the value // If it doesn't throw an error message next to the TextField. @@ -89,7 +101,21 @@ function NewRunParametersV2(props: NewRunParametersProps) { Object.keys(props.specParameters).map(key => { if (props.specParameters[key].defaultValue) { // TODO(zijianjoy): Make sure to consider all types of parameters. - runtimeParametersWithDefault[key] = props.specParameters[key].defaultValue; + let defaultValStr; // Convert default to string type first to avoid error from convertInput + switch (props.specParameters[key].parameterType) { + case ParameterType_ParameterTypeEnum.STRUCT: + case ParameterType_ParameterTypeEnum.LIST: + defaultValStr = JSON.stringify(props.specParameters[key].defaultValue); + break; + case ParameterType_ParameterTypeEnum.BOOLEAN: + case ParameterType_ParameterTypeEnum.NUMBER_INTEGER: + case ParameterType_ParameterTypeEnum.NUMBER_DOUBLE: + defaultValStr = props.specParameters[key].defaultValue.toString(); + break; + default: + defaultValStr = props.specParameters[key].defaultValue; + } + runtimeParametersWithDefault[key] = defaultValStr; } }); setUpdatedParameters(runtimeParametersWithDefault); @@ -145,6 +171,7 @@ function NewRunParametersV2(props: NewRunParametersProps) { const param = { key: `${k} - ${ParameterType_ParameterTypeEnum[v.parameterType]}`, value: updatedParameters[k], + type: v.parameterType, }; return ( @@ -160,7 +187,7 @@ function NewRunParametersV2(props: NewRunParametersProps) { let parametersInRealType: RuntimeParameters = {}; Object.entries(nextUpdatedParameters).map(([k, v1]) => { parametersInRealType[k] = convertInput( - v1.toString(), + v1, props.specParameters[k].parameterType, ); }); @@ -182,6 +209,7 @@ export default NewRunParametersV2; interface Param { key: string; value: any; + type: ParameterType_ParameterTypeEnum; } interface ParamEditorProps { @@ -202,16 +230,23 @@ class ParamEditor extends React.Component { prevState: ParamEditorState, ): { isInJsonForm: boolean; isJsonField: boolean } { let isJson = true; - try { - const displayValue = JSON.parse(''); - // Nulls, booleans, strings, and numbers can all be parsed as JSON, but we don't care - // about rendering. Note that `typeOf null` returns 'object' - if (displayValue === null || typeof displayValue !== 'object') { - throw new Error('Parsed JSON was neither an array nor an object. Using default renderer'); - } - } catch (err) { - isJson = false; + let paramType = nextProps.param.type; + + switch (paramType) { + case ParameterType_ParameterTypeEnum.LIST: + case ParameterType_ParameterTypeEnum.STRUCT: + isJson = true; + break; + case ParameterType_ParameterTypeEnum.STRING: + case ParameterType_ParameterTypeEnum.BOOLEAN: + case ParameterType_ParameterTypeEnum.NUMBER_INTEGER: + case ParameterType_ParameterTypeEnum.NUMBER_DOUBLE: + isJson = false; + break; + default: + isJson = false; } + return { isInJsonForm: isJson, isJsonField: prevState.isJsonField || isJson, @@ -229,8 +264,22 @@ class ParamEditor extends React.Component { const onClick = () => { if (this.state.isInJsonForm) { + let paramType = param.type; + let displayValue; + switch (paramType) { + case ParameterType_ParameterTypeEnum.LIST: + displayValue = JSON.parse(param.value || '[]'); + break; + case ParameterType_ParameterTypeEnum.STRUCT: + displayValue = JSON.parse(param.value || '{}'); + break; + default: + // TODO(jlyaoyuli): If the type from PipelineSpec is either LIST or STURCT, + // but the user-input or default value is invalid JSON form, show error message. + displayValue = JSON.parse(''); + } + // TODO(zijianjoy): JSON format needs to be struct or list type. - const displayValue = JSON.parse(param.value?.string_value || ''); if (this.state.isEditorOpen) { onChange(JSON.stringify(displayValue) || ''); } else { @@ -249,8 +298,8 @@ class ParamEditor extends React.Component { id={id} disabled={this.state.isEditorOpen} variant='outlined' - // label={param.name} - // value={param.value || ''} + label={param.key} + value={param.value || ''} onChange={ev => onChange(ev.target.value || '')} className={classes(commonCss.textField, css.textfield)} InputProps={{